chill added inline comments.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:1828
+ if (CodeGenOpts.BranchTargetEnforcement) {
+ FuncAttrs.addAttribute("branch-target-enforcement", "true");
+ }
----------------
danielkiss wrote:
> chill wrote:
> > I would really prefer to not set values "true" or "false" for the
> > attribute: we don't really have tri-state logic there
> > (absent/present-true/present-false), and those values just add some
> > not-very useful string processing.
> >
> the attribute will be "absent" for the runtime emitted function.
How about setting the attribute for LLVM created functions at the time of
creation, just like Clang created functions
get their attribute at the time of creation?
================
Comment at: clang/lib/CodeGen/CGCall.cpp:1831
+
+ auto RASignKind = CodeGenOpts.getSignReturnAddress();
+ if (RASignKind != CodeGenOptions::SignReturnAddressScope::None) {
----------------
danielkiss wrote:
> chill wrote:
> > What do we get from setting the PACBTI state in the default function
> > attributes? We still have
> > to do a per function processing, we can just as well avoid repeating the
> > logic, and spare us some
> > adding and potentially removing attributes churn.
> >
> in the new patch the per function processing will change the attribute only
> if really need.
Sure, but that's duplication of code/logic, it's a source of countless issues
"oh, here's the place I should fix that thing ... oh noes, turns out I have to
fix ten more ... hope I've found all ..." ;)
================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:200
+ if (!F.hasFnAttribute("branch-target-enforcement"))
+ return false;
+ Attribute A = F.getFnAttribute("branch-target-enforcement");
----------------
chill wrote:
> chill wrote:
> > This should be "true", although the comment might turn out moot.
> >
> > If we somehow end up with a function, that does not have that attribute, we
> > should clear the
> > ELF flag.
> >
> Oh, I see, those are the cases of sanitizer functions, created at LLVM level,
> that don't have the attribute.
> Please, leave a comment in that sense.
Or, as mentioned in the other comment, check if it's possible to set the
attribute at the time of creation (from module attributes?). Tri-state logic
is added complexity, if it's necessary, it's necessary, but if it's not, better
make it simpler.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D75181/new/
https://reviews.llvm.org/D75181
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
