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

[Momchil Velikov via Phabricator via llvm-commits]

chill added inline comments.


================
Comment at: clang/lib/Basic/Targets/ARM.cpp:387
+
+  return a.isArmT32();
+}
----------------
For example Arm7-a defines the T32 instruction set, buy we still want a warning.
Maybe we need `return a.isArmT32() &&  a.isArmMClass()`.
I'm not actually sure whether the triple correctly reflects the target instruction set, e.g.
what are we going to get from `-target arm-eabi -march=armv7-a -mthumb`, so the approach with
the target triple might not be working.



================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:6402
+                 CGM.getLangOpts().hasSignReturnAddress() ||
+                 CGM.getLangOpts().isSignReturnAddressScopeAll()) {
+        // If the Branch Protection attribute is missing, validate the target
----------------
This condition `CGM.getLangOpts().isSignReturnAddressScopeAll()` is redundant.


================
Comment at: llvm/include/llvm/ADT/Triple.h:724
 
+  /// Tests whether the target is T32.
+  bool isArmT32() const {
----------------
This function does not look quite as expected.

`!isARM()` might be `isThumb()` but we're going to return false, isn't it ?

Then `isThumb()` might be true while we have, say, `armv6k`.

AFAICT, the test (and probably the whole function) ought to be

```
  switch (auto SubArch = getSubArch()) {
  case Triple::ARMSubArch_v8m_baseline,
  case Triple::ARMSubArch_v7s:
  case Triple::ARMSubArch_v7k:
  case Triple::ARMSubArch_v7ve:
  case Triple::ARMSubArch_v6:
  case Triple::ARMSubArch_v6m:
  case Triple::ARMSubArch_v6k:
  case Triple::ARMSubArch_v6t2:
  case Triple::ARMSubArch_v5:
  case Triple::ARMSubArch_v5te:
  case Triple::ARMSubArch_v4t:
        return false;
  default:
      return true;
   }
```

which is pretty future-proof.




================
Comment at: llvm/include/llvm/ADT/Triple.h:725
+  /// Tests whether the target is T32.
+  bool isArmT32() const {
+    if (!isARM())
----------------
In any case, if we're going to change the `Triple`, it should come with unit tests in `unittest/ADT/TripleTest.cpp`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115501/new/

https://reviews.llvm.org/D115501