Page MenuHomePhabricator

[AArch64] Handle BTI/PAC in case of generated functions.
AbandonedPublic

Authored by danielkiss on Feb 26 2020, 7:46 AM.

Details

Summary

Functions can be added not just by the fronted and PAC/BTI attributes
should be added to those too. The new pass manages the attributes.
After this change the all runtime created functions like _clang_call_terminate
or constructor that is emitted by a sanitizer will be handled for PAC/BTI if applicable.

Diff Detail

Event Timeline

danielkiss created this revision.Feb 26 2020, 7:46 AM
danielkiss retitled this revision from [Clang][AArch64] Add default arguments to runtime functions. to [AArch64] Handle BTI/PAC in case of generated functions..
danielkiss edited the summary of this revision. (Show Details)
danielkiss added a project: Restricted Project.

Previous version of the patch handled only the functions that are created in clang. Sanitizers can't see the codegen options therefore they also disables BTI.
This version of the patch is less invasive in my opinion, effects only aarch64.
branch-target-enforcement(BTI) and sign-return-address(PAC) are added to all function that comes from clang frontend.
If the backend encounters with the function without BTI attribute but the module is compiled with BTI then it assumes it should be made BTI compatible.

chill added a comment.Mar 2 2020, 6:53 AM

That said, my comments are not of the "over my dead body" kind ;)

clang/lib/CodeGen/CGCall.cpp
1827

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.

1830

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.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
200

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.

201–202

... that kind of string processing, here and elsewhere.

llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
61–65

Isn't there some redundancy with the two calls to getFnAttribute and to hasFnAttribute ?

chill added inline comments.Mar 2 2020, 6:56 AM
llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
200

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.

danielkiss updated this revision to Diff 248201.Mar 4 2020, 9:11 AM

Thanks for the comments, patch is improved
isStringAttribute() check removed, the attribute is always a string in this case or "null" so the check is not needed.
Function level the attribute is now only change when needed, so as the function level attribute is expected to be rare I hope the performance won't be impacted by the patch.
I kept the "tri-state" logic because of the emitted functions. Introducing a "branch-target-enforcement-disabled" attribute seems even more confusing for me.

danielkiss marked 4 inline comments as done.Mar 4 2020, 9:16 AM
danielkiss added inline comments.
clang/lib/CodeGen/CGCall.cpp
1827

the attribute will be "absent" for the runtime emitted function.

1830

in the new patch the per function processing will change the attribute only if really need.

llvm/lib/Target/AArch64/AArch64BranchTargets.cpp
62

emmited -> emitted

danielkiss updated this revision to Diff 248222.Mar 4 2020, 9:39 AM
danielkiss marked an inline comment as done.
danielkiss marked an inline comment as done.

Patch is rebased, test is updated.

chill added inline comments.Apr 1 2020, 12:16 AM
clang/lib/CodeGen/CGCall.cpp
1827

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?

1830

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 ..." ;)

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
200

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.

danielkiss added inline comments.Apr 1 2020, 2:42 AM
clang/lib/CodeGen/CGCall.cpp
1827

Attributes are not always set in clang as I see:
CodeGenModule::CreateRuntimeFunction() -> CodeGenModule::GetOrCreateLLVMFunction
CreateRuntimeFunction could be fixed but, the common location for LLVM created function is the llvm::Function::Create() where the CodeGenOpts and LangOpts are no available.
Adding target specific code there seems not right for me.

danielkiss marked an inline comment as done.Apr 2 2020, 6:49 AM
danielkiss added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
5149–5152

I'm going to rebase the patch. I add there a new attribute here "ignore-branch-target-enforcement"
so then the "branch-target-enforcement"="true"/"false" could be just "branch-target-enforcement".

danielkiss updated this revision to Diff 254761.Apr 3 2020, 5:54 AM

The patched is rebased and the hopefully the logic is now simpler.

tamas.petz added inline comments.Apr 3 2020, 9:00 AM
clang/lib/CodeGen/CGCall.cpp
1826–1827

LangOptions has the following functions:

  • hasSignReturnAddress()
  • isSignReturnAddressWithAKey()
  • isSignReturnAddressScopeAll()

With these functions some of these lines can be significantly reduced.

1836

Use getLangOpts(). isSignReturnAddressWithAKey()

clang/lib/CodeGen/CodeGenModule.cpp
595

Use new functions please.

danielkiss updated this revision to Diff 255347.Apr 6 2020, 8:05 AM
danielkiss added a reviewer: tamas.petz.

Fix review comments from Tamas.

danielkiss marked 3 inline comments as done.Apr 6 2020, 8:06 AM
chill added inline comments.Apr 7 2020, 9:23 AM
clang/lib/CodeGen/TargetInfo.cpp
5149–5152

TBH, that's worse, IMHO.

Ideally, I *think* we'd like *every* LLVM IR function that the backend sees,
regardless of how, why and by whom it is created, to have (or not have)
the three existing PACBTI attributes "sign-return-address", "sign-return-address-key", and "branch-target-enforcement", so the backend can generate code accordingly.

The module attributes are LLVM IR metadata, and AFAIK LLVM IR metadata is an optional extra,
it should not affect correctness.
Indeed, *module* metadata is a somwhat grey area, better not use it if there a way around it.

tamas.petz added inline comments.Apr 7 2020, 10:44 AM
clang/lib/CodeGen/TargetInfo.cpp
5149–5152

Which case are you trying to handle here?
Is this the case, for example, when -mbranch-protection=standard is set but a function has _attribute _((target("branch-protection=none")))?

danielkiss added inline comments.Apr 8 2020, 2:02 PM
clang/lib/CodeGen/TargetInfo.cpp
5149–5152

Chill: I think I have code that solve that for clang created functions, but functions created in llvm I don't have any idea.

Tamas: yes, that is the case when module is compiled with bti but the function is not ( "none" or "pac-ret" and so on. )

danielkiss updated this revision to Diff 268383.Jun 4 2020, 1:37 AM
danielkiss edited the summary of this revision. (Show Details)

Patch is refactored, the new pass smooth out the attribute issues.
This version fixes issues with _clang_call_terminate, sanitisers and the CFI.
New pass might need a new file, I'm not yet convinced myself.

chill added a reviewer: ab.Jul 31 2020, 3:02 AM
chill added a comment.Aug 4 2020, 4:29 AM

This approach looks way too hackish to me with multiple opposing attributes ("sign-return-address" vs. "ignore-sign-return-address")
and some convoluted logic to resolve the contradiction.

Would it be better to add a new value to "sign-return-address" as "none"? I don't see any other alternative option, I'm open to any other idea.

Would it be better to add a new value to "sign-return-address" as "none"? I don't see any other alternative option, I'm open to any other idea.

FWIW GCC has a "sign-return-address" function attribute with a default value of "none". It is considered deprecated, however, in favour of "branch-protection"

FWIW GCC has a "sign-return-address" function attribute with a default value of "none". It is considered deprecated, however, in favour of "branch-protection"

This is just the internal representation, the function attribute in C/C++ source is the "branch-protection".

Old version of the patch used attribute value to represent the "ignore"/disabled state https://reviews.llvm.org/D75181?id=247201 so I have no idea what would be the right solution.

chill added a comment.Aug 5 2020, 12:16 PM

I don't see any other alternative option, I'm open to any other idea.

My original idea was to pass options to LLVM. I'll come up with a patch in a day or two (if it works) and then we'll see.

danielkiss abandoned this revision.Aug 29 2020, 5:41 AM

Abandoning in favour of D85649.