Apologies for the delay in responding. Is there any reason why the ELF Header and Program Header need to be covered by a PT_LOAD region at all? For example if the ELF file is on flash somewhere. The RTOS loader reads the ELF header and program header from the file in flash. If the program headers are not in the address range covered by any PT_LOAD then there is no need to copy the program headers into the TCM or SDRAM. As I understand it with -nmagic (and no PHDRS) neither LLD or ld.bfd will attempt to put the ELF Header and Program Header into a PT_LOAD segment.

Apologies for the delay in responding. As I understand it, this would only cause a problem with PHDRS? Our implicit creation of program headers should already be in OutputSection order.

Not much to add over what Renato has said. I've put some context into the special meaning of MOVS with the PC as destination. I'm not entirely sure why the MOVS with immediate is unrestricted, my guess is that it is a historical quirk of the encoding and has likely never been used.

Code changes and test LGTM. The OSABI question is interesting, I don't have a particular good answer or suggestion, the whole area is a bit of a mess. Looking at what ELF says:

EI_OSABI
Byte e_ident[EI_OSABI] identifies the OS- or ABI-specific ELF extensions used by this file. Some fields in other ELF structures have flags and values that have operating system and/or ABI specific meanings; the interpretation of those fields is determined by the value of this byte. If the object file does not use any extensions, it is recommended that this byte be set to 0. If the value for this byte is 64 through 255, its meaning depends on the value of the e_machine header member. The ABI processor supplement for an architecture can define its own associated set of values for this byte in this range. If the processor supplement does not specify a set of values, one of the following values shall be used, where 0 can also be taken to mean unspecified.

The flag is in the OS specific range so it could clash with other OS specific flags, to be strictly compliant it should set an OSABI to avoid clashes. It is unfortunate that we can only set one value though. For example the Arm processor supplement defines a couple of OSABI flags. What is the combination of using a GNU OS specific flag? I think ELF is implying that OSABI should be set to GNU if there is no processor specific OSABI value, I guess a strictly compliant toolchain would not permit SHF_RETAIN if there is some other OSABI in action that doesn't also recognise SHF_RETAIN. Quite how practical that is, and how strictly it has been followed in the past is another matter though.

Thanks for the update and the link to D95199 . Thanks also for pointing out that this is reliant on the PHDR command, that wasn't clear from the description (I can see it in the test now). I'll try and take a deeper look at the weekend. Apologies I can only do this in my spare time and it is a busy week at the moment.

Looks like we need to make sure that the relocation is a static link time constant so that it can be used in PIE and Shared Libs. Otherwise LGTM.

This looks ok to me. The gotpage_lo15 matches the operator name in GNU assembler.

As I understand it, this is about what library call should be made when there is an eabi target such as arm-none-eabi which should target the Run Time ABI for the Arm Architecture https://github.com/ARM-software/abi-aa/blob/master/rtabi32/rtabi32.rst and other Arm targets such as Linux or Android. I think if we're arguing from the specification then clang is compliant with the eabi, so relocatable objects produced by clang should be compatible with eabi compliant toolchains that supply the eabi functions, this includes non GNU toolchains as well like IAR.

Thanks for working on this. It will be very useful for people using GCC yet using LLD as a linker on AArch64. Will try and take a look later today, if not then I should have some time tomorrow morning.

LGTM, may be worth waiting for any other comments.

No objections from me, looks like a sensible change.

The calculation looks good to me. It is logically equivalent to that given in the reference manual

Thanks for working on this. It looks like it implements the ABI correctly (see PR for links to the specification and GCC/Glibc details).

Looks good to me. Thanks for tidying this up.

Radio silence so far; I think no news is good news applies in this case. I'm happy to say no objections.

Personally I'm in favour of clang and gcc behaving the same way unless there is a good reason not to. I've shared the review internally to see if anyone has any concerns. May be worth informing the clang built linux community to see if they will need to make any alterations as a result.

No objections from me. Although unrelated to the patch, isLSDA might be better called fromFDE, or couldBeLSDA? Maybe one for a later follow up.

LGTM, with one suggestion of a name change. I've checked the GNU ld behaviour against the green test changes and LLD matches.

Assuming there are no problems with the bots or other downstream builds, please update the PR with a link to this review.

Now committed. Commit message has your name and email address.

Thanks for updating the comments. If the lexical ordering is going to work out the same I don't have any objections.

Thanks for the updates, LGTM too.

In D90698#2389872, @chaosdefinition wrote:

If there are no more comments, can someone please commit this for me? I don't have commit access to LLVM.

Will take another look tonight.For reference, I tried helping out someone last year with https://bugs.llvm.org/show_bug.cgi?id=44698 I think they had old objects, or a an old GCC on their system and it was mixing with a modern clang. Maybe worth a read through.

It took a lot of staring at that bit of code to work out what was going on. If I have understood it correctly. In particular I got confused with the sizeBefore and sizeLastSort with sizeBefore being the larger value. It took me a while to spot the continue. If I've understood correctly:

• sizeBefore is really sizeOfRetBeforeNextSort
• sizeLastSort is really sizeOfRetAfterPreviousSort

If possible could we use Prev rather than Last as it is more specific. Last can mean the last one we went by, or the last one in the list. I strongly recommend adding something like Next to sizeBefore so that it is clear that sizeBefore >= sizeLastSort.

Looks OK to me. Will be best to see if George has any comments about how to do the parsing.

Apologies for being so quiet recently. It looks like this is changing .ctors/.dtors away from SORT_BY_NAME when they aren't mixed with .init_array.

This looks good to me. The __ARM_FP feature test macro is defined in the ACLE and 0x8 is the right flag to check for Double precision support. I've tested at least one single precision only build.

In D90108#2359539, @MaskRay wrote:

In D90108#2358482, @jhenderson wrote:

In D90108#2357342, @nickdesaulniers wrote:

In D90108#2357336, @MaskRay wrote:

We can surely loose it if it turns out to be a wide spreading problem.

The hard part is that regressions tend to get reported AFTER the release when it's too late to fix them. Maybe a warning for one release, then upgrade to hard error next release gives users more time to find and fix errors, and doesn't result in an unusable release version for anyone?

I don't have any specific evidence to suggest that we'd run into a problem doing this, but my personal thinking is that adding an option to allow disabling this error might be a good idea. I wouldn't be surprised to find code bases where fixing the code is non-trivial. Ideally, the option would not be made widely known unless actually asked for and/or heavily labelled with things saying something like "this is a temporary option that will be deleted in a future change unless a compelling reason is found not to".

If there are concerns, I can make .globl foo; .weak foo a warning instead of error. The others are still errors.

I've left a few small suggestions but overall no objections. Given that this is the default BFD behaviour and programs that have only been linked with LLD are not likely to use common; I think that if this behaviour is to be put under an option, it is on by default. This will permit users to use both GNU ld and lld with the same command line.

If we issue a warning instead, users will only notice the semantic difference when they use -Wa,--fatal-warnings. We can classify the potential risks on whether .local is used:

• .local is used: .local is a very rare directive.
• .local is not used: the issue is about .weak and .globl . This represents a semantic difference between MC and GNU ld. There may be a few more but I doubt there can be more than a few. Can we try error first and switch to a warning if it turns out to be a problem?

Personally I'd err on the side of consistency with binutils. If they are happy to go to an error then we can. If they aren't then I recommend that we warn rather than error as we risk breaking builds for working software and it isn't always easy to get source code fixed. Not got a strong opinion though so happy to go with the consensus.

LGTM too. Apologies for the delay. This looks like it is consistent with the ELF specification.

LGTM. If we are marking a section live it makes sense to mark its dependent sections live.

I don't have any objections. My comment in the thread was more along the lines that dependent libraries could interact with whole-archive in hard to predict ways. I'm happy to have dependent libraries ignore whole-archive. I'm fine with MaskRay accepting when he is happy.

Looks good to me. Thanks very much for adding the functionality.

LGTM. Agree that it should be bit31 that is sign extended. Apologies for not catching this last time.

What's here looks good to me to.

I'll try and take a look tomorrow.

Thanks for the update. LGTM. Happy to see other parts done in follow up commits

I think there may still be problems in a non-relocatable ELF file when there is more than one executable output section as there is only one .ARM.exidx output section and only one sh_link.

Looks good to me. Thanks for cleaning this up, it will be of great help to 32-bit bots.

LGTM too, thanks for adding the tests.

Thanks for the update I'm happy with the edits.

Will have to have a think. My initial reaction to "apply this file only when these conditions hold" is akin to moving part of the build system into the linker. It could be done, but I'm not sure it should be. For example if there were a linker script it would have to precisely match the inputs to make sense.

I've put some comments, mostly concentrating on the early part. I've not had a chance to go through the examples at the bottom in detail. I can't spot anything obviously wrong though.

I don't have any objections to the approval. Thanks for updating the comment.

Thanks for the update and sorry to take so long to comment. I can't see anything immediately wrong and I think limiting this to fragments in the same subsection makes sense.

I had a look in Levine's Linkers and Loaders book to see if I could find any source for back-references, all I could find was in 6.4 "Not all linkers do this; many just make a single sequential pass over the directory and miss any backwards depedencies from a file to another file earlier in the library. Tools like tsort and lorder can minimize the difficulty of using single-pass linkers but it's not uncommon for programmers to explicitly list the same library several times on the linker command line to force multiple passes and thus finally resolve all symbols."

Thanks for putting this together, looks good. I've added a few suggestions, and some typo spots.

LGTM too, I'm happy my comments were addressed, thanks.

Overall looking good. I think there may be an endianness problem with reinterpret_cast although I could easily be wrong without testing (I don't think we have a target that uses .note.gnu.property with big-endian support in LLD).

Had a chance to look over lunch. Thanks looks good to me too.

Not had a chance to look into the changes in detail yet, but on first glance it looks good. Given that the compiler uses STT_FUNC for the entry of a symbol I think it is reasonable to retain the type only for an alias.

In ELF the last paragraph of the section on STT_COMMON only mentions that a dynamic linker may change its resolution rules to prefer non-common over a common definition. I'm guessing extending this to static linker is something that some other linkers have done and there are some programs that currently depend on that behaviour.

Symbols with type STT_COMMON label uninitialized common blocks. In relocatable objects, these symbols are not allocated and must have the special section index SHN_COMMON (see below). In shared objects and executables these symbols must be allocated to some section in the defining object.

Given that it is possible to write a linker script that does work with *(pattern1 pattern2) then I don't have any objections. I've tentatively approved as it looks like was the most cautious here.

Thanks for the update. I'm happy with the changes. I've tentatively approved, James/Edd please shout if you have any objections?

In D85056#2217103, @MaskRay wrote:

In D85056#2188969, @psmith wrote:

I do have some sympathy with wanting to use a command line option to keep an individual section from the command line as it has been useful in Arm's proprietary linker, although this is mainly due to the convenience of not having to create or modify another file.

One observation I'd make about the proposed implementation is that it looks like it only implements a subset of the Linker Script KEEP command that supports precise matches of a section name. Quoting from the GNU linker manual:

When link-time garbage collection is in use (`--gc-sections'), it is often useful to mark sections that should not be eliminated. This is accomplished by surrounding an input section's wildcard entry with KEEP(), as in KEEP(*(.init)) or KEEP(SORT_BY_NAME(*)(.ctors)).

That permits the full power of the input section description to discriminate via object, as there can be many sections with the same name. Wildcards can sometimes be useful too.

I think it would be worth considering a richer interface for keep-sections. In Arm's proprietary linker we permitted a similar syntax as for the equivalent linker script. This did mean quoting or escaping parentheses but did provide equivalence in what could be achieved.

Having a non-INSERT-AFTER/BEFORE SECTIONS command is considered an external linker script and can change the default layout decisions. I can also feel sympathy with the users but I am also wary of adding these non-orthogonal features. The recent D76482 (__build_id_start = .) and this patch make me think of output section descriptions (a fragment of a SECTIONS command) which do not affect section layout.

@psmith @grimar I think we probably should start a conversation with binutils about such a feature. If they find needs as well, we will have a common ground, it'd be great. They need to be given the decision making opportunity to reduce the risk they create a similar but incompatible feature in the future.

One idea is:

OVERRIDE SECTIONS {
  .foo : { KEEP(*(.foo)) }
  .bar : { KEEP(*(.bar)) }
  sym = .;   // symbol assignments are disallowed
}

The output section descriptions will override .foo & .bar in the external linker script. If the external linker script does not describe .foo or .bar, the command will change the orphan sections.

Another syntax:

SECTIONS {
  .foo : { KEEP(*(.foo)) }
  .bar : { KEEP(*(.bar)) }
} REPLACE .foo;

Just to check I understand.

I think this should be ok for .ARM.exidx sections as they have a SHF_LINK_ORDER dependency which prevents them from being merged in relocatable links.

Looks good to me. Will be worth seeing if MaskRay has any comments.

Thanks for the update, I'm happy when George is.

Thanks for spotting that. I'd personally prefer to fix in the linker script, rather than make the code RW but I don't have a strong preference.

Simplification looks good to me. I'm not sure about the test changes as I would have expected the change to be NFC. If these are test improvements it will be worth altering separately.

I agree it's not what I'd like. I'm not sure how to react to MaskRays comment. The top of the ClangCommandLineReference.rst says:

-------------------------------------------------------------------
NOTE: This file is automatically generated by running clang-tblgen
-gen-opt-docs. Do not edit this file by hand!!
-------------------------------------------------------------------

This uses Options.td to generate the file. I don't know how true this is anymore given that both files are checked in. They may have been separated. I've seen at least one instance in the log where the whole file has been regenerated.

Yes. I think the consensus was to match the GCC behaviour, I'll abandon the revision.

FWIW this looks good to me too.

No objections from me either. I guess LGTM if George's last comment can be resolved.

uploaded diff with both Options.td and ClangCommandLineReference.rst

In D85239#2194604, @MaskRay wrote:

This file is auto generated. The real documentation should go to clang/include/clang/Driver/Options.td

I agree I think it is best to err on the side of correctness. As I understand it we could do better (merge more sections) but this would involve pulling apart .eh_frame sections to show equivalence. I think that could come later.

My apologies for not commenting earlier. I haven't had a lot of spare time and I only have a weak opinion, certainly far weaker than the other commenters.

I agree with MaskRay that if we do support the feature then we should make sure it works or document its limitations, even if our limitations are "Implemented to the degree that it supports the FreeBSD Kernel, we recommend producing an ELF file and using llvm-objcopy --output-target=binary."

I do have some sympathy with wanting to use a command line option to keep an individual section from the command line as it has been useful in Arm's proprietary linker, although this is mainly due to the convenience of not having to create or modify another file.