Page MenuHomePhabricator

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

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



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

1828 ↗(On Diff #247201)

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.

1831 ↗(On Diff #247201)

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.

200 ↗(On Diff #247201)

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 ↗(On Diff #247201)

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


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

chill added inline comments.Mar 2 2020, 6:56 AM
200 ↗(On Diff #247201)

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.
1828 ↗(On Diff #247201)

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

1831 ↗(On Diff #247201)

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


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
1828 ↗(On Diff #247201)

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?

1831 ↗(On Diff #247201)

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

200 ↗(On Diff #247201)

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
1828 ↗(On Diff #247201)

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.

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
1827–1828 ↗(On Diff #254761)

LangOptions has the following functions:

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

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

1837 ↗(On Diff #254761)

Use getLangOpts(). isSignReturnAddressWithAKey()

596 ↗(On Diff #254761)

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

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

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

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.Thu, Jun 4, 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.