Use IR rather than C for the clang test.
I was able to simplify the clang patch somewhat as a "lowering error" isn't any more revealing than just emitting a generic "backend error".

In D72194#1965731, @MaskRay wrote:

I am mostly fine with the LLVM CodeGen/MC side change, but the clang side change can probably be moved to a subsequent change. For the test/clang/CodeGen/ test, we may want to test .ll -> .s instead, not .c -> .s

Report error using LLVM diagnostics framework.

@nickdesaulniers and @MaskRay,

use reportError instead of report_fatal_error.
Split testing of error and output to defeat redirection race.

In rG8cedf0e2994c1a258902ed931abdec5f94725a55#891201, @arsenm wrote:

In rG8cedf0e2994c1a258902ed931abdec5f94725a55#891041, @ychen wrote:

In rG8cedf0e2994c1a258902ed931abdec5f94725a55#890965, @arsenm wrote:

Thanks for the comments, @arsenm. I expected this would cause concerns for practical reasons. I do think it is heading the right direction though.

I have a problem with this change. IMO the entire reason to use report_fatal_error is to not crash, or do anything that looks like a crash.

6c2d233e7a8993d90fd97703ed8331f2d6e80671 introduced this method. You could tell from the deleted code that it was trying to replace abort() call. MHO is that report_fatal_error should crash (or could since crash is its default behavior, it could be overriden) as its name suggests. It is for shortcutting error propagation where it is hard to assume the compilation process could exit cleanly. So exit may not be a good choice.

I should never see a backtrace after calling report_fatal_error, and it should be a clean exit.

This part is customized by tools using install_fatal_error_handler (clang does). llc uses InitLLVM which asks for a backtrace.

For example I should not see a backtrace on a legalization failure with -global-isel-abort=1

I think you mean in this case, the crash better not to trigger a backtrace? The option name suggests it should abort.

I think there are 3 levels of error handling:

1. User facing errors that can occur due to user error, that should use the LLVMContext error reporting
2. Clean errors that generally should generally not be end user facing, but also should be handled gracefully and in all builds. Most cases that can only be produced by writing unrealistic MIR tests should use this form of error since it's not really worth the effort to produce a user facing error
3. Asserts for situations that should never happen. This is what llvm_unreachable/assert, and can be deleted in release builds.
   
   This patch blobs uses 2 and 3 together. report_fatal_error is not equivalent to an assert.

Addressed header file order review comment

Addressed review comments.
Cosmetic test improvements.

Avoided use of ,unique, assembly feature when not using the integrated assembler.

Corrected silly mistake.

Added llvm_unreachable to getSectionPrefixForGlobal()

Rebased and removed changes to MCContext::renameELFSection().

Addressed review comments and rebased onto master.

In D72194#1861171, @MaskRay wrote:

I saw this a couple of weeks ago. Apologies that I did not pay enough attention. (I complained that GNU as lacks features. Things are changing (I'm glad that my __patchable_function_entries complaints worked:) )

In D73999#1866680, @psmith wrote:

In D73999#1866168, @MaskRay wrote:

Address review comments.

I tend to agree with Alan Modra's arguement in
https://sourceware.org/ml/binutils/2020-02/msg00093.html

I don't think so. User assembly often gets section attributes wrong
or leaves them off entirely for special sections known to the
assembler. ie. the first .section .foo above is a built-in rather
than user input.

Add more folks for feedback.

Please see my binutils post. For a .section directive with the same name but a different field. If the field is:

• sh_flags or sh_type: warn
• sh_link due to SHF_LINK_ORDER: no warning. produce separate sections
• sh_entsize due to SHF_MERGE: still controversial

I agree with warning for sh_flags, I think there is too much legacy code out there that would behave differently.
I agree with sh_link producing separate sections. In an ideal world no-one writes this by hand in assembly.
For sh_entsize SHF_MERGE, this is informing the linker that it can produce an optimisation, which it is permitted to ignore. I tend towards an error message if it is implementable as it is a strong message to fix the assembler. The next best thing is a warning then clearing SHF_MERGE and setting sh_entsize to 0. I don't think a warning on its own is safe.

In D74006#1860074, @MaskRay wrote:

Another thing is whether we should use sh_flags/sh_type to differentiate sections. In addition, whether we should use sh_entsize.
If yes, the implementation can be a bit clumsy. We may have to add many Elf*_Shdr fields to ELFSectionKey.
If no (GNU as behavior), we should add warnings. D73999 will do that.

The section group name is part of the key in both MC and GNU as. By analogy, SHF_LINK_ORDER+sh_link should probably be part of the key.

Field not specified by the .section directive (sh_addrline,sh_addr,sh_offset,sh_info) should definitely not be part of the key.

I have a slight preference for "no", though I don't currently have a particular good reason why "sh_entsize" should not be part of a key. Maybe we are fine with current .rodata.cst16 naming.

I think that this patch removes the need for uniquing sections for symbols with associated symbols which are explicitly assigned to a section name e.g. via attribute((section name)).

updated the patch and description

I have updated the patch. It now handles both "default" sections and "implicit" sections ("implicit" sections are those that are created by MC normally as it encounters symbols). I have improved the test coverage, organised the tests and improved the comments for the test cases. Sorry it took a while to update the patch.. this was a more difficult change than I had anticipated. I have tried a few different approaches, I am not entirely happy with the current implementation but it is difficult to simply further.

In D72197#1825494, @MaskRay wrote:

In D72197#1825390, @pcc wrote:

In D72197#1825247, @MaskRay wrote:

In D72197#1825012, @pcc wrote:

It looks like this change caused us to start rejecting:

.thumb
.thumb_func
.globl foo
.hidden foo
foo:
adr lr, foo + 1

with:

test.s:6:1: error: unsupported relocation on symbol
adr lr, foo + 1
^

This is exposed by this Android ART code: https://cs.android.com/android/platform/superproject/+/master:art/runtime/arch/arm/quick_entrypoints_arm.S;l=1826

I see a couple of possible fixes for this:

1. We could go back to the previous behaviour for global hidden symbols.
2. We could teach MC and LLD about the R_ARM_THM_ALU_PREL_11_0 relocation required to relocate this instruction. Arguably the ART code shouldn't be adding the 1 for Thumb here (because R_ARM_THM_ALU_PREL_11_0 adds the 1 itself), so ART would then need to be fixed.

foo is a hidden definition in the current object file. Can the ART code be changed to use a local symbol (adr lr, .Lfoo) instead?

It could, but I don't really see a good argument for local symbols having different behaviour to strong hidden symbols.

Not special casing hidden in the general case has the nice property that: the assembler behavior is not affected by the visibility. Only the binding can.

We can thus postpone decisions to the linker, and all the logic can be implemented on the linker side. Both a hidden defined symbol and a local symbol is considered non-preemptive, thus they have the same behavior at the linker stage.

The downside is that we increase object file sizes. Hidden symbols are not that common so the downside may be acceptable.

I rebased the patch and addressed the review comments. I also noticed that there was a problem with the patch when explicitly placing non-mergeable sections into one of the "default" mergeable sections (e.g. .debug_str). As these "default" mergeable sections were not being added to the map it wasn't possible to explicitly place symbols into these even if the symbols entsize was compatible (as they would be put into a uniqued section instead). This is not acceptable for these "default" sections since for some of these like e.g. .debug_str, current binutils consumers make the assumption that there can only be one of these sections. The fix for this is WIP and I haven't added LIT tests for it yet; but, I have put up the patch in WIP state to get comments on the approach taken.

In D72194#1821072, @smeenai wrote:

Is this worth merging into 10.0 if it lands after the branch?

Thanks, I'll address the points that you have raised.
I tried updating the assert and rebasing the patch but the assert is now firing in the It tests. I will have to investigate this tomorrow and update the patch then.

In D72194#1818766, @nickdesaulniers wrote:

This is a partial fix for

Is this still considered a partial fix? Seems pretty complete to me, otherwise what's still missing?

In D68101#1806475, @nickdesaulniers wrote:

In D68101#1806213, @rjmccall wrote:

In D68101#1806135, @nickdesaulniers wrote:

In D68101#1802280, @rjmccall wrote:

I laid out a series of three options before:

• Emit different object-file sections for different mergeability settings.
• Only mark an object-file section as mergeable if all the symbols in it would have the same mergeability settings.
• Stop implicitly using mergeability for "ordinary" sections (i.e. sections other than the string section).

Of the above, I really think #2 is the only complete solution.

Can you explain why you think #1 is not "complete"? All three seem to establish correctness; I can see how giving up on the optimization (#3) is not a great solution, but #1 doesn't have that problem and actually preserves it in more places. To be clear, this is just taking advantage of the ability to have multiple sections with the same name in an ELF object file; they will still be assembled into a single section in the linked image.

My understanding is that changing the flags on an MCSection retroactively is pretty counter to the architecture, so I'm not sure how we'd actually achieve #2.

Ah, ok, I reread https://reviews.llvm.org/D72194 and see that it's creating non-contiguous sections (option 1), with unique entity sizes. That should be fine. Dissenting opinion retracted. We should prefer https://reviews.llvm.org/D72194 with the commit message updated.

Updated the diff to address review comments and expand test coverage.

In D72197#1806225, @MaskRay wrote:

In D72197#1805438, @bd1976llvm wrote:

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

Also, this code is missing a case for STB_GLOBAL and visibility != STV_DEFAULT. Actually, I have noticed before that this case is missing in various places in MC. As it stands this patch may cause a performance regression on platforms that use -fvisibility=hidden.

Visibility is irrelevant here. For a symbol defined in the same section as the instruction, the relocation record will reference a non-SECTION symbol instead of a STT_SECTION after this change. If the symbol is STV_HIDDEN, the linker will think the symbol is non-preemptible. So, no performance regression.

Thanks for the feedback. I'm sorry that I am being slow on this... v.busy at the moment. Will try to finish the patch in the next few days.

I wonder if is this really a bug or intentional. Clang and LLVM seem to ignore the possibility of symbol interposition (see: -fno-semantic-interposition). What is the LLVM policy for this?

In D68101#1802280, @rjmccall wrote:

The solution described in that comment is not acceptable. We are not going to tell users that they cannot assign symbols explicitly to sections because LLVM is unable to promise not to miscompile if they do. It is LLVM's responsibility to correctly compile valid code; enabling mergeability for a section containing unnamed_addr symbols is an optimization, and if it is not safe, it needs to be disabled until we can figure out a way to make it safe.

Sorry for the delay in updating this.

In D68101#1766359, @rjmccall wrote:

I can't speak for the pragma authors, but I strongly doubt the pragma is intended to force all affected globals to go into a single section unit, since the division into section units is purely an object-file representation issue.

The feature certainly could be implemented for COFF, so I think changing the name of the command line option is reasonable. The strings in the .deplibs sections map to libraries in an implementation defined manner, so I refer to them as "specifiers". In COFF dependent libraries are specified via "directives" in the object files. It might be worth naming the option something like: --dependent-lib-directives (as directives works for the entries in an ELF .deplibs sections equally as well as specifiers) ?

In D68101#1766151, @nickdesaulniers wrote:

In D68101#1765665, @rjmccall wrote:

Given all that, this patch seems far too aggressive. While mergeable sections can be useful for optimizing arbitrary code that might not use a section, they are also extremely useful for optimizing the sorts of global tables that programmers frequently use explicit sections for. It seems to me that the right fix is to find the place that ensures that we don't put mergeable and non-mergeable objects in the same section unit (or at least conservatively makes the section unit non-mergeable) and fix it to consider entry size as well. That should be straightforward unless that place doesn't exist, in which case we have very serious problems.

Disagree (but I spent all day thinking about this). Going back to https://bugs.llvm.org/show_bug.cgi?id=31828, I think I solved the problem there incorrectly; we should be not marking the explicit section merge-able.

In D68101#1689622, @SjoerdMeijer wrote:

Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time.

Hi All, Apologies that I missed the request for input here.

In D68101#1689622, @SjoerdMeijer wrote:

Hi Ben, this is not really my area of expertise, but it all starts to make some sense to me. I was expecting this transformation to happen earlier, but this is where the magic happens, and this probably where it belongs.
Just to make sure I haven't missed anything, I would like to take this patch and run some numbers, for which I need a little bit of time. If in the mean time someone with some more experience in this area here has a look too, that would be great....

Hi Sjoerd,

In D68101#1684828, @jmolloy wrote:

This feels like it could cause a pretty serious regression. This essentially disables global merging with -fdata-sections, which I know at least one linker relies upon for code size.

In D62935#1537096, @steven_wu wrote:

Thanks. I was not sure if the new functions in LTOModule.h/.cpp were in the right file - but I don't see a better place to put them.

This is fine. Do you think you might want to query symbols from InputFile instead of LTOModule in the future?

In D62935#1537096, @steven_wu wrote:

We should look at either merging the implementations or providing an api function that provides access to the "llvm.linker.options" named metadata via the IRSymtab. It may not be possible to merge the implementations because in COFF and MACHO the representation of dependent libraries is a string of command line options; whereas, the ELF representation is an array of library strings. This is because for COFF and MACHO there is basically only one linker for each file format so the command line format is fixed. For ELF there are a greater variety of linkers, so I just pass the library strings through to the linker, without processing them into command line arguments, and it is deferred to the linker to interpret the library strings.

It would be good if the API is flexible to return either. Not against having a different API to return linker options but I think that is less clean.

Thanks for the response.

Great to see someone wanting to use this. During the RFC we decided not to set SHF_EXCLUDE on the .deplibs ELF section that is used to store the strings from these pragmas. This means that other linkers that don't support dependent libraries will not remove the sections and they will end up in the output. This shouldn't cause any functional issues; however, it might be noticed by developers and commented on. Perhaps we should change this decision? Also, have you considered the other option of having the build system invoke the clang with the --dependent-library=<specifier> switch. This is an alternative approach where modifying the source code is not appropriate, an example use is: https://reviews.llvm.org/D61742.

@grimar - this patch causes a regression in behaviour for references to script symbols. I think https://reviews.llvm.org/D61298 is a reasonable fix. Could you take a look?

Addressed review comments

I am keen to keep this moving.

No longer shortening "dependent libraries" to "deplibs" except for the .deplibs section (as this takes up bytes on disk).

LGTM! Thanks for waiting for me to confirm. Comments inline:

Sorry for the delay in replying. I was trying to understand if the upgrade path was important. Comments inline:

Hi Steven,