chill added inline comments.
================
Comment at: clang/lib/Basic/Targets/ARM.cpp:391-392
+ if (!Arch.empty() && !isBranchProtectionSupportedArch(Arch))
+ return false;
+
----------------
amilendra wrote:
> chill wrote:
> > On empty `Arch` it'd continue down the function, but we'd like to return
> > failure.
> I am having trouble getting the test `arm-branch-protection-attr-1.c` to work
> after these changes. `validateBranchProtection()` checks the combination of
> two parameters, the branch protection attribute and architecture.
> If the architecture is empty, like below, shouldn't the function to continue
> checking further than simply returning false?
> ```
> __attribute__((target("branch-protection=bti"))) void btionly() {}
> ```
> Or should I be using something else other than
> `CGM.getTarget().getTargetOpts().CPU` to get the architecture in
> `ARMTargetCodeGenInfo::setTargetAttributes`?
>
We shouldn't be getting an empty `Arch`, or rather we should definitely know
what we are generating code for.
If that cannot be reliably obtained via wherever the `Arch` parameter comes
from, maybe we could instead check
target features (`TargetOptions::Features`). It's conceptually //more
correct// too, even though in this particular instance
it probably does not matter much.
As a general note too, I think it'd be better to check for when PACBTI-M
instructions (NOP or not) are definitely *not* available
as architecture names where they are is likely to change with time.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115501/new/
https://reviews.llvm.org/D115501
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
