Page MenuHomePhabricator

[clang][ARM] Emit warnings when PACBTI-M is used with unsupported architectures
ClosedPublic

Authored by amilendra on Dec 10 2021, 2:54 AM.

Details

Summary

Branch protection in M-class is supported by

  • Armv8.1-M.Main
  • Armv8-M.Main
  • Armv7-M

Attempting to enable this for other architectures, either by
command-line (e.g -mbranch-protection=bti) or by target attribute
in source code (e.g. attribute((target("branch-protection=..."))) )
will generate a warning.

In both cases function attributes related to branch protection will not
be emitted. Regardless of the warning, module level attributes related to
branch protection will be emitted when it is enabled via the command-line.

The following people also contributed to this patch:

  • Victor Campos

Diff Detail

Event Timeline

amilendra created this revision.Dec 10 2021, 2:54 AM
amilendra requested review of this revision.Dec 10 2021, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2021, 2:54 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
stuij added a subscriber: stuij.Dec 10 2021, 3:50 AM
danielkiss added inline comments.Dec 10 2021, 5:49 AM
clang/lib/Basic/Targets/ARM.cpp
376

I'd play safe and return false here.

amilendra updated this revision to Diff 393917.Dec 13 2021, 9:11 AM

Address review comment.

chill added inline comments.Dec 14 2021, 7:07 AM
clang/lib/Basic/Targets/AArch64.h
72–73

Would be nice to have parameter names, like in the adjacent declarations.

clang/lib/Basic/Targets/ARM.cpp
379–383
385
399–400

On empty Arch it'd continue down the function, but we'd like to return failure.

clang/lib/Basic/Targets/ARM.h
128–129

Likewise.

clang/test/CodeGen/arm_acle.c
1530

These look like random changes for the untrained eye

amilendra added inline comments.Dec 15 2021, 1:54 AM
clang/lib/Basic/Targets/ARM.cpp
399–400

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?

chill added inline comments.Dec 15 2021, 3:27 AM
clang/lib/Basic/Targets/ARM.cpp
399–400

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.

amilendra updated this revision to Diff 396671.Dec 30 2021, 7:27 AM

Address review comments.

Fix clang-format errors.

amilendra marked 4 inline comments as done.Dec 30 2021, 3:41 PM
amilendra updated this revision to Diff 398901.Jan 11 2022, 4:11 AM

Refactor the check conditions to a single function (isArmT32)

Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 4:11 AM
chill added inline comments.Jan 12 2022, 3:00 AM
clang/lib/Basic/Targets/ARM.cpp
387

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.

clang/lib/CodeGen/TargetInfo.cpp
6402

This condition CGM.getLangOpts().isSignReturnAddressScopeAll() is redundant.

llvm/include/llvm/ADT/Triple.h
724

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.

725

In any case, if we're going to change the Triple, it should come with unit tests in unittest/ADT/TripleTest.cpp

amilendra updated this revision to Diff 402659.Jan 24 2022, 2:03 PM

Add new interface isArmMClass() to the Triple class.
Use isArmT32() and isArmMClass() to emit PACBTI warnings for ARM.

amilendra marked 7 inline comments as done.Jan 26 2022, 3:23 AM
amilendra marked an inline comment as done.
chill accepted this revision.Jan 26 2022, 3:47 AM

LGTM, cheers!

This revision is now accepted and ready to land.Jan 26 2022, 3:47 AM
amilendra updated this revision to Diff 403801.Jan 27 2022, 2:15 PM

Fix clang-format errors.

The CI errors seem unrelated.

Failed Tests (9):
  libarcher :: races/critical-unrelated.c
  libarcher :: races/lock-nested-unrelated.c
  libarcher :: races/lock-unrelated.c
  libarcher :: races/parallel-simple.c
  libarcher :: races/task-dependency.c
  libarcher :: races/task-taskgroup-unrelated.c
  libarcher :: races/task-taskwait-nested.c
  libarcher :: races/task-two.c
  libarcher :: task/task_late_fulfill.c

So pushing these changes.

This revision was landed with ongoing or failed builds.Jan 28 2022, 2:00 AM
This revision was automatically updated to reflect the committed changes.