[PATCH] D115501: [clang][ARM] Emit warnings when PACBTI-M is used with unsupported architectures

[Momchil Velikov via Phabricator via cfe-commits]

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