This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][ELF][llvm-readobj] Support ELF AUTH constants
Needs ReviewPublic

Authored by kovdan01 on Aug 22 2023, 7:02 PM.

Details

Summary

This patch adds llvm-readobj support for:

Diff Detail

Event Timeline

kovdan01 created this revision.Aug 22 2023, 7:02 PM
Herald added a project: Restricted Project. · View Herald Transcript
kovdan01 requested review of this revision.Aug 22 2023, 7:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 22 2023, 7:02 PM
kovdan01 updated this revision to Diff 554806.Aug 30 2023, 12:46 PM

Rebase to current mainline

@MaskRay @psmith A kind reminder regarding the revision. Would be glad to see your comments.

I haven'ed looked closely, but a <numeric> issue makes this not buildable for me..

lld/ELF/Config.h
151 ↗(On Diff #554992)

They make the offsets of following variables larger. move to the end.
<xxx, 0> is more common in lld. The inline element doesn't save.

lld/ELF/Driver.cpp
2595 ↗(On Diff #554992)

move the check to the caller

2612 ↗(On Diff #554992)

No trailing period. Please check the existing convention.

2615 ↗(On Diff #554992)

Please check the existing convention.

lld/ELF/InputFiles.cpp
964 ↗(On Diff #554992)

move the check to the caller

lld/ELF/SyntheticSections.cpp
362 ↗(On Diff #554992)

this needs <numeric>

lld/test/ELF/aarch64-feature-pauth.s
3 ↗(On Diff #554992)

%tsplit makes temporary output filenames unnecessarily long. Just use RUN: rm -rf %t && split-file %s %t && cd %t

please check the existing convention.

59 ↗(On Diff #554992)

We omit messages before error: as the executable may have a different name, e.g. ld.lld.exe. Please follow the style of some recently added tests.

kovdan01 updated this revision to Diff 555770.Sep 4 2023, 10:35 AM
kovdan01 marked 8 inline comments as done.

Fix issues mentioned by @MaskRay

lld/ELF/Config.h
151 ↗(On Diff #554992)

Got it, fixed

lld/ELF/Driver.cpp
2595 ↗(On Diff #554992)

Done

2612 ↗(On Diff #554992)

Fixed, also changed "one. Either" to "one; either" - multiple sentences IMHO don't look nice in such context

lld/ELF/InputFiles.cpp
964 ↗(On Diff #554992)

Done

lld/ELF/SyntheticSections.cpp
362 ↗(On Diff #554992)

Fixed, thanks! It looks like this file is included implicitly from other files on my machine, so didn't mention it by myself.

lld/test/ELF/aarch64-feature-pauth.s
3 ↗(On Diff #554992)

Fixed, thanks, it definitely enhances readability

59 ↗(On Diff #554992)

Got it, fixed

The program properties are in the appendix as an alternative. Do we need to implement both for the same platform? If both are supported then we have the problem of what happens when one relocatable object has a notes section and the other a GNU properties section.

The ELF marking in the specification was very much a placeholder in the specification waiting for some implementation experience in prototypes. The notes section was used mostly as GNU properties are a Linux extension, and this may end up being used outside of a GNU platform. I strongly recommend choosing one or the other. Personally I would go with the note section as the properties are listed in an appendix as an alternative.

Experience from BTI has shown that assembler files, particularly those like crt0.s often creep into projects and are hard to find. Many of these assembler files will have no code pointers in them at all so giving a fatal error may be too harsh.

You may want to follow intel CET, which has a -z cet-report = [none|warn|error] for example -z pauth-report = [none|warn|error] which controls what happens when there are some objects without the note section.

I haven't checked through all the details in the implementation.

llvm/tools/llvm-readobj/ELFDumper.cpp
5367

I recommend printAArch64Note as ARM in all caps tends to refer to the ARM (AArch32 backend). Even if the vendor/owner name is ARM the company.

The program properties are in the appendix as an alternative. Do we need to implement both for the same platform? If both are supported then we have the problem of what happens when one relocatable object has a notes section and the other a GNU properties section.

The ELF marking in the specification was very much a placeholder in the specification waiting for some implementation experience in prototypes. The notes section was used mostly as GNU properties are a Linux extension, and this may end up being used outside of a GNU platform. I strongly recommend choosing one or the other. Personally I would go with the note section as the properties are listed in an appendix as an alternative.

The implementation proposed in this patch allows only one type of pauth mark at a time: if one of input files has .note.AARCH64-PAUTH-ABI-tag section and another one has GNU_PROPERTY_AARCH64_FEATURE_PAUTH in .note.gnu.property, the linker will emit an error: see lld/test/ELF/aarch64-feature-pauth.s:15:

error: input files contain both a .note.AARCH64-PAUTH-ABI-tag section and a GNU_PROPERTY_AARCH64_FEATURE_PAUTH feature in a .note.gnu.property section; all the input files must use the same way of specifying AArch64 PAuth compatibility info

I think we can leave this behavior since it's conservative enough, but allows some platforms to use an alternative marking way if they want to.
The compatibility check implemented is pretty generic (treat files as compatible if and only if their PAuth notes are identical), and it can be used as a skeleton if some platform wants something more complex.
Please let me know your thoughts on that - as for now, I don't see major downsides in leaving both ways implemented (if it's explicitly disallowed to mix them), but maybe you see some issues with that.

Experience from BTI has shown that assembler files, particularly those like crt0.s often creep into projects and are hard to find. Many of these assembler files will have no code pointers in them at all so giving a fatal error may be too harsh.

You may want to follow intel CET, which has a -z cet-report = [none|warn|error] for example -z pauth-report = [none|warn|error] which controls what happens when there are some objects without the note section.

Thanks! I'll definitely implement that, didn't know that such an option exists. I'll also add the notes manually to assembly files in libunwind though - just in case someone wants to link that with the option set to error or using a linker other than lld.

I am puzzled by the need of both the generic .note.AARCH64-PAUTH-ABI-tag and the GNU-centric .note.gnu.property.
If .note.AARCH64-PAUTH-ABI-tag is preferred, you can push forward .note.AARCH64-PAUTH-ABI-tag adoption and not bother with .note.gnu.property.
Why should the linker bear complexity related to .note.gnu.property and its interaction with .note.AARCH64-PAUTH-ABI-tag (error, but we could avoid this complexity but not support .note.gnu.property in the first place).

If there is not so such GNU hatred, I'd like that we only implement .note.gnu.property and only implement .note.AARCH64-PAUTH-ABI-tag when it is really needed, i.e. some ELF OSes using PAuth somehow uses .note.AARCH64-PAUTH-ABI-tag.
I'm not certain certain ELF OSes are present. The ABI can be written in a more natural way to favor .note.AARCH64-PAUTH-ABI-tag .

lld/ELF/Config.h
435 ↗(On Diff #555770)

Variables unrelated to command-line options are generally placed in Ctx.

lld/ELF/Driver.cpp
2600 ↗(On Diff #555770)

move f1 outside (prefer [0]. to front()->) and use ArrayRef(ctx.objectFiles).slice(1)

2605 ↗(On Diff #555770)

^ => !=

lld/ELF/InputFiles.cpp
968 ↗(On Diff #555770)

It's conventional to call errorOrWarn and continue parsing instead of calling fatal

reportFatal can be changed to return bool indicating whether the caller should early return.

974 ↗(On Diff #555770)

I am not sure the cast is a bad idea. This is the idiomatic way. Elf_Nhdr recognizes endianness and has packed data members.

lld/test/ELF/aarch64-feature-pauth.s
5 ↗(On Diff #555770)

Since we cd %t, all the %t prefix of temporary object files can be omitted. We ensure all temporary files are in %t now.

6 ↗(On Diff #555770)

cp gnu11.o gnu12.o

7 ↗(On Diff #555770)

a non-shared object can be named .so, but it's confusing. Suggest -shared

MaskRay requested changes to this revision.EditedSep 17 2023, 6:00 PM

The way D158574 and D156882 are organized looks awkward to me. I think the preferred way is

  • patch A: add constants to BinaryFormat and implement llvm-readobj, with tests in llvm/test/tools/llvm-readobj. Please study previous patches adding llvm-readobj support for new constants
  • patch B: full-fledged lld implementation
lld/ELF/SyntheticSections.cpp
375 ↗(On Diff #555770)

The section name convention is AArch64* instead of Aarch64*

384 ↗(On Diff #555770)

unneeded

This revision now requires changes to proceed.Sep 17 2023, 6:00 PM

I agree with MaskRay that for an Alpha specification there is no good reason to support both marking schemes in relocatable objects. I wish I had deleted the alternative one rather than moving it to an appendix. I'll note that the spec covers relocatable objects only, it is the platform's decision about executables and shared objects.

The encoding difference between the processor specific note section and the GNU property is minimal. Essentially the GNU property is just wrapping the contents of the NOTE section in a property. At the relocatable object this doesn't make a lot of difference as the (platform/vendor schema-version) does not fit well with any existing GNU property [*] so it would still need custom code to process. At the executable, shared-object level there is more of a benefit for GNU properties as the program header that identifies the program properties can make it easier for the dynamic linker to find the information. It is possible to use the raw notes in relocatable objects but use a program property in the executable.

Internally to Arm there is a discussion on how to do ELF marking in relocatable objects (not executables and shared-libraries), for example do we continue to use GNU properties or do we use something like Arm (or RISCV) build attributes. I hope that there will be a proposal that can be shared externally for public comment can be put on the Arm ABI github page at some point in the near future. As ABI work tends to move slowly I suspect it will be too late for this though.

  • X86 and AArch64 properties

X86 properties https://gitlab.com/x86-psABIs/x86-64-ABI/-/merge_requests/13/diffs?commit_id=b0b0c518c28be848e8494bf3dff2a97688e5fce0
Arch64 properties in https://github.com/ARM-software/abi-aa/blob/main/aaelf64/aaelf64.rst#62program-property

kovdan01 updated this revision to Diff 557429.Sep 27 2023, 5:28 PM
kovdan01 marked 11 inline comments as done.
kovdan01 retitled this revision from [AArch64][ELF] Support PAUTH ABI marking to [AArch64][ELF][llvm-readobj] Support ELF AUTH constants.
kovdan01 edited the summary of this revision. (Show Details)

TODO:

  • yaml2obj is for some reason broken when using AARCH64_DYNAMIC_TAG instead of DYNAMIC_TAG
  • more tests for new relocation types
lld/ELF/Config.h
435 ↗(On Diff #555770)

Done, thanks. I suppose we also might want to move andFeatures to Ctx - I initially placed this new field here because andFeatures was here. I'll submit a small subsequent patch regarding that.

lld/ELF/Driver.cpp
2600 ↗(On Diff #555770)

Regarding [0]. instead of front()-> - not relevant anymore after '-z pauth-report' implementation. We now use the first non-empty pauth abi tag as a reference instead of the first one.

Regarding `f11 - done, thanks.

2605 ↗(On Diff #555770)

Changed, thanks! I don't know why I used such a strange way of comparing booleans here :)

lld/ELF/InputFiles.cpp
968 ↗(On Diff #555770)

It's conventional to call errorOrWarn and continue parsing instead of calling fatal

Done, thanks

reportFatal can be changed to return bool indicating whether the caller should early return.

I'm not sure what you meant here. Please let me know if my changes are not what you expected.

974 ↗(On Diff #555770)

Yes, you a right, removed the comment. Thanks!

lld/ELF/SyntheticSections.cpp
375 ↗(On Diff #555770)

Changed, thanks

384 ↗(On Diff #555770)

Done, thanks

lld/test/ELF/aarch64-feature-pauth.s
5 ↗(On Diff #555770)

Ah, forgot to change that. Fixed, thanks

6 ↗(On Diff #555770)

Definitely better, fixed

7 ↗(On Diff #555770)

Fixed, thanks

llvm/tools/llvm-readobj/ELFDumper.cpp
5367

OK, it makes sense. Changed ARM->AArch64

@MaskRay Resolved the issues mentioned and moved the patch to https://github.com/llvm/llvm-project/pull/72713. Please consider reviewing it there. Thanks!