This patch make LLVM emit the processor specific program property types defined in AArch64 ELF spec https://developer.arm.com/docs/ihi0056/f/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation A file containing no functions gets both property flags. Otherwise, a property is set iff all the functions in the file have the corresponding attribute. Patch by Daniel Kiss and Momchil Velikov.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Apologies for the delay in responding. I've got a query about the alignment of the .note.gnu.property sections. As I understand it they should be 8-byte aligned for a 64-bit machine. I also think it would be helpful to warn that if at least one, but not all functions have the branch-target-enforcement attribute.
Other that that I think the change looks good.
Some links for other reviewers:
- for definition of .note.gnu.property https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf
- for AArch64 definitions of GNU_PROPERTY_AARCH64_FEATURE_1_PAC et-al. https://developer.arm.com/docs/ihi0056/f/elf-for-the-arm-64-bit-architecture-aarch64-abi-2019q2-documentation
- for X86_64 equivalent of BTI (IBT) https://github.com/hjl-tools/x86-psABI/wiki/x86-64-psABI-draft.pdf
| llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp | ||
|---|---|---|
| 190 | There is a similar implementation in X86AsmPrinter that uses FeatureFlagsAnd rather than Flags, it may be worth using a similar naming convention as these are equivalent features. This isn't a strong opinion by any means, just a suggestion. | |
| 193 | The case where at least one, but not all functions have the branch-target-enforcement attribute is somewhat problematic. At the moment BTI can only be enabled per link-unit, and if there is any object not marked with the .note.gnu.property section the linker will refuse to propagate the property into the image (without a command-line --force-bti). With that in mind clearing the property clears it for the entire link-unit, rather negating the point of using it in the first place. On the other hand not-clearing the property means that not all functions have branch-target-enforcement so claiming that they do could result in a run-time error if they are indirectly called. I think it would be worth a warning if at least one, but not all functions have branch-target-protection. The same case exists for PAC, below, however as I understand it, it is less important for the whole link-unit to be covered as it is with BTI. | |
| 214 | On 64-bit architectures SHT_NOTE sections have 8-byte alignment. For .note.gnu.property there has been some discussion about this as the actual section contents don't require 8-byte alignment. My understanding is that the consensus came out as 8-byte alignment. This is what has been implemented in the X86AsmPrinter::EmitStartOfAsmFile. The binutils AArch64 assembler tests have 8-byte alignment. see https://patches-gcc.linaro.org/patch/16268/ property-bti-pac1.s In summary I think we need to double check what GCC has done here, and add EmitAlignment directives if they are necessary. | |
| llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp | ||
|---|---|---|
| 190 | I don't see how that would be an improvement in readability. If the function manipulated several different kinds of flags, it'd be worth it renaming to, say, FeatureFlags, but then, if the function did more than one thing I'd separate the part that emitted the note section in a separate utility function ... and leave the name Flags. FeatureFlagsAnd is a rather unfortunate name, a) it is extremely long for a local variable name, and b) it makes me ask "... and what?" :D | |
Comment Actions
Looks good to me. Will be worth giving Tim or other reviewers a day for a last chance to comment before committing.
There is a similar implementation in X86AsmPrinter that uses FeatureFlagsAnd rather than Flags, it may be worth using a similar naming convention as these are equivalent features. This isn't a strong opinion by any means, just a suggestion.