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.

It is a pity that the section name is not a valid C identifier otherwise start_ and stop_ would have been generated automatically.

In D82014#2180015, @MaskRay wrote:

Ping @kamleshbhalui

I hope your or some reviewer can test the buildability on ARM.

This looks good to me. Regardless of whether we allow --gc-sections it will be worth getting this right for /DISCARD/. Will be worth waiting to see if there are any further comments from other reviewers.

I guess if the use case for -r is a kernel module then garbage collection can make sense as the "object" is not relinked. What happens when there is no GC root? I think it will be worth detecting that and turning GC off as otherwise everything will get removed which is unlikely to be what was wanted.

In D72904#2157639, @jhenderson wrote:

Whilst the actual change seems fine, in that it does what it sets out to do, I believe, I'm struggling to follow why a SHF_LINK_ORDER section should have a sh_link of 0 at all. Surely the section should just then not have SHF_LINK_ORDER? What sections actually have sh_link=0 and SHF_LINK_ORDER?

I'll take a look over the weekend, also at D72904, apologies won't be able to get to it today.

What would happen if there was a single .text section containing multiple functions, would that produce multiple .gcc_except_table sections each with a SHF_LINK_ORDER dependency on the single .text section? I think that this is OK as there are no other sections that refer to .gcc_except_table and there doesn't need to be a total order. Ideally we could have one .gcc_except_table section per associated .text section.

I think an option makes sense. I have a small reservation about making users order their matches on the command line. We should make it more explicit in the help and the manual, or try and sort the matches using a heuristic order of specificity, for example no wildcards is more specific than wildcards, longer text is more specific than shorter. Other than that I think it looks OK.

LGTM it looks like a good simplification. Worth waiting a little bit to see if there are any objections.

LGTM, it looks like it is difficult to generate local dynamic from clang, although it is possible with GCC. I was able to make a test application, that also had the advantage of working for shared libraries and PIE as R_ABS does not in that case.

Thanks, will take a look over the weekend. Would like to make sure all is well running some TLS test cases on a native Arm machine, fully expect it to be OK.

I'm ok with this as is. I'll need to check over what should be happening on Arm, but I think that can be done in a separate change.

This looks OK to me, happy for George to approve . I'm not a Windows filesystem expert by any means, it seems like the rename then delete follows the pattern in https://boostgsoc13.github.io/boost.afio/doc/html/afio/FAQ/deleting_open_files.html though.

Looks good to me. Agree that symbol versioning should be passed through in a relocatable link.

Thanks for the update, I don't have any more comments. Happy when MaskRay is happy.

I think that this is highly likely to be a typo, it looks like it dates to the contribution of compiler-rt so there isn't any history we can use. I believe that compiler-rt builtins originated in Apple so there is a possibility that SOFTFP was used in the compiler at contribution time. If you wanted to be safe you could support both SOFTFP and SOFT_FP but I don't think that is necessary. I would like to think that for a soft-fp target the tests for these functions fail or are at least xfailed at the moment. Are you able to compile the tests for a soft-fp target to check?

I only partially followed the discussion on llvm-dev. The resolution sounds sensible to me. Will be good to get someone involved in the upstream discussion to check it over.

Looking good so far. Would be good to add a range extension thunk test case. For example:

 .section .text.1, "ax", %progbits
 .global _start
 .type _start, %function
_start: .word callee@PLT - .

That looks good to me too. Thanks for the updates.

I've moved the codes below the block of relocations representing TLS.

LGTM, I don't have a better idea.

Thanks for the comments. When I made the change I looked at:

Static relocation codes for ELF64 object files begin at (257); dynamic ones at (1024). Both (0) and (256) should be accepted as values of R_AARCH64_NONE, the null relocation.
Static relocation codes for ELF32 object files begin at [1]; dynamic ones at [180].

I couldn't find anything about a TLS range, but it looks like there is one being observed even if it isn't explicitly called out. Let me know if you've spotted it?

Thanks for the update. I don't have any more comments. Looks good to me.

In D81184#2074619, @MaskRay wrote:

I'm not sure unconditionally changing .word foo - . to use R_AARCH64_PLT32 is the appropriate change. Note that x86 R_X86_64_PC32 doesn't do this.
.word foo@plt - . would probably make more sense. For one reason, R_AARCH64_PLT32 is not supported by binutils, unconditionally using it can break -fno-integrated-as.
There is also no need to special case on visiblity: (ElfSym.getVisibility() == llvm::ELF::STV_HIDDEN) ? R_CLS(PREL32) : R_CLS(PLT32);, you can entirely defer the handling to the linker

Additionally, the lld changes should probably be moved to a separate patch. It is not tightly coupled with MC. The tests are also inadequate - you'd need a range extension thunk test. I can do this part if you don't mind.

Looks sensible to me. A couple of comment suggestions only.

LGTM, looks like a good idea. We may find other places where this form will be useful.

LGTM from an Arm person now that the Android changes have been made.

LGTM too, makes sense to keep as compatible with binutils as possible.

FWIW I'm happy for Clang to match GCC behaviour for Linux (non-Android) targets with respect to the frame pointer. Outside of Android I would expect most developers of linux applications to build with both GCC and clang so they should already have the -fno-omit-frame-pointer if they are using it.

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

Looks reasonable to me. Our documentation and help for dynamic-list is a bit sparse, it may be worth emphasising that it implies -bsymbolic (if I've understood correctly).

LGTM.

Just thinking if it would be possible to resolve this without having to read the verneed section of shared objects. I've not checked to see if this would make a simpler implementation though.

No objections from me, this does seem to be more consistent with the Coding Standards.

Thanks for the updates, I don't have any more comments. Happy if George is happy.

Looks good to me. The intention is that the little and big-endian table flags match, only the Offset is expected to change for big-endian, and this change does make sure that the flags match. Will be worth waiting a day or so to see if there are any further comments.

I don't have a lot to add. I think this change will work with LLD and the GNU tools, no objections to this change. Having LLD not recognise the section names without a suffix may become a problem if GNU tools still output them. My guess is that LLD is still most used with clang at the moment, but this may change over time.

Thanks for the review. I've applied the suggestion to remove the function.

LGTM too. Thanks for the update.

I definitely think we should do this for all new options. I agree that -o magic ought to be changed as I can see that happening in practice. The chance that someone intending -t hinlto-index-only rather than -thinlto-index-only is much lower. I'll defer to the users of thin-lto whether they think this is important.

I don't have a strong opinion here, the spaced bytes more closely match the output of the llvm-mc which can make disassembler tests easier to write. On the other hand it can be useful to see the structure of the disassembly of instructions when the mapping symbols exist.
Arm GNU objdump does the following:

address: 12345678 <spaces> <disassembly of arm instruction
address: 1234 5678 <spaces> for 32-bit Thumb instruction made up of two half words
address: 1234 <spaces> for 16-bit Thumb instruction made up a half word
address: 12345678 <spaces> for .word 0x12345678

I don't have a lot of experience of non Arm RISC architectures though so I don't know what looks reasonable for them.

Applied previous review comments. Added missing REQUIRES, added CHECK-NOT and simplified range checking code.

Thanks for the comments, will update diff shortly.

Thanks for the comments. I've updated the patch prior to commit.