[MC][ELF] Ensure that mergeable globals with an explicit section are assigned to SHF_MERGE sections with compatible entsizes

[bd1976llvm]

Summary

Ensure that symbols explicitly* assigned a section name are placed into a section with a compatible entry size.

This is done by creating multiple sections with the same name** if incompatible symbols are explicitly given the name of an incompatible section, whilst:

• Avoiding using uniqued sections where possible (for readability and to maximize compatibly with assemblers).
• Creating as few SHF_MERGE sections as possible (for efficiency).

Given that each symbol is assigned to a section in a single pass, we must decide which section each symbol is assigned to without seeing the properties of all symbols. A stable and easy to understand assignment is desirable. The following rules facilitate this: The "generic" section for a given section name will be mergeable if the name is a mergeable "default" section name (such as .debug_str), a mergeable "implicit" section name (such as .rodata.str2.2), or MC has already created a mergeable "generic" section for the given section name (e.g. in response to a section directive in inline assembly). Otherwise, the "generic" section for a given name is non-mergeable; and, non-mergeable symbols are assigned to the "generic" section, while mergeable symbols are assigned to uniqued sections.

Terminology:

"default" sections are those always created by MC initially, e.g. .text or .debug_str.
"implicit" sections are those created normally by MC in response to the symbols that it encounters, i.e. in the absence of an explicit section name assignment on the symbol, e.g. a function foo might be placed into a .text.foo section.
"generic" sections are those that are referred to when a unique section ID is not supplied, e.g. if there are multiple unique .bob sections then ".quad .bob" will reference the generic .bob section. Typically, the generic section is just the first section of a given name to be created. Default sections are always generic.

* Typically, section names might be explicitly assigned in source code using a language extension e.g. a section attribute: _attribute_ ((section ("section-name"))) - https://clang.llvm.org/docs/AttributeReference.html

** I refer to such sections as unique/uniqued sections. In assembly the ", unique," assembly syntax is used to express such sections.

Fixes https://bugs.llvm.org/show_bug.cgi?id=43457.

See https://reviews.llvm.org/D68101 for previous discussions leading to this patch.

Some minor fixes were required to LLVM's tests, for tests had been using the old behavior - which allowed for explicitly assigning globals with incompatible entry sizes to a section.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

693

It is better to handle this here, associated symbols are handled in a similar manner.

llvm/test/CodeGen/X86/explict-section-mergeable.ll

12 ↗

(On Diff #236160)

Hi Nick,

The "unique 1" here is an LLVM extension to allow for multiple sections with the same name to be created and referenced in assembly.
The GNU assembler only allows the creation of a single section for a given name.. if you try to create a second section with the same name as a previous section but different properties then the assembler instead switches the current section back to the original section and ignores the properties you specified.

[bd1976llvm]

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

[rjmccall]

Okay. This seems conceptually reasonable to me, but I don't feel qualified to review the code, sorry.

[nickdesaulniers]

Thanks for the extensive tests. I might triple check the entity sizes to make sure they're correct, but the tests look pretty exhaustive to me and make sense.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

737

Right, shouldn't this be:

if (Flags & ELF::SHF_MERGE) {
  assert((Section->getFlags() & ELF::SHF_MERGE) &&
        "Merge-able section requested but not produced");
  assert(Section->getEntrySize() == getEntrySizeForKind(Kind) &&
        "Mismatched merge-able section entity sizes");
}

?

Also, it might be nice to hoist the multiple calls to getEntrySizeForKind(Kind) into one variable rather than have 3 calls at different scopes.

llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp

82

Are the ORC test changes related to this change?

[nickdesaulniers]

This is a partial fix for

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

[bd1976llvm]

llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp

82

Yes. In the original test the user has explicitly assigned a 32bit integer into the .debug_str section (GV->setSection(".debug_str")). The .debug_str section is a special section reserved for the system with fixed properties. MC creates a .debug_str section internally before looking at any of the globals as a mergeable section containing ASCII strings. The attempt to assign the integer foo to .debug_str ends up creating two .debug_str sections with the second one being a mergeable section with its entsize compatible with 32bit integers with the patch.

I traced the history of the test and I believe that using an integer is simply a mistake and that the original unit test author should have put strings into .debug_str. So, I updated the unit tests to do that.

[bd1976llvm]

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?

Having thought about it I am happy with this being the full fix. I was of the opinion that we should also add diagnostics for when a symbol has been explicitly assigned to an incompatible section. For example putting a function in the data section (.data). However, you can easily do this at link time and no diagnostic will be emitted. So I am now of the opinion that no diagnostics are actually required and I am happy to close the bug once this patch has landed.

[nickdesaulniers]

Having thought about it I am happy with this being the full fix

Me too. Please amend the patch description above to say so, rather than *partial fix*. Also, please address the two comments I've "bumped."

llvm/include/llvm/CodeGen/TargetLoweringObjectFileImpl.h

17 ↗

(On Diff #236160)

bump.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

737

bump

llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp

82

Thanks for the context, please add it to the patch description above so that others have context when looking at git log. It may also be worthwhile to subscribe the tests' authors to this change, which I've done (@lhames ).

[bd1976llvm]

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.

[lhames]

llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp

82

This test just needed to put something into a debug section so that the section would be forced though the JIT memory manager. No care was taken with types. ;)

This fix looks good to me.

[smeenai]

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

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

685

We should check Ctx.getAsmInfo()->useIntegratedAssembler() and not use the linkage ",unique" for -no-integrated-as.

I created a feature request a few days ago https://sourceware.org/bugzilla/show_bug.cgi?id=25380

[bd1976llvm]

In D72194#1821072, @smeenai wrote:

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

Yes please.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

685

@MaskRay - Thanks for the comments. This was my original design - see comments in https://reviews.llvm.org/D68101.

However, I now think that we should land this patch in its current state and file a bug for supporting -no-integrated-as properly. The ",unique" LLVM extension is used in other places e.g. for comdats (with no-unique-names) and for MD_associated. What do you think?

Also, I think that we should probably consider if the design of the ",unique" extension is correct. It seems a bit of a clumsy extension. Some ideas:

1. Maybe it it would be better to have an index scheme to select between sections with the same name rather than being able to apply a unique number to any section.
2. We could have an alias scheme so that you can use a unique name for a section in assembly but specify a real name when the section is emitted into an object file.
3. I think it might be nicer to have a new section directive e.g. .unique_section to support multiple sections with the same name rather than the current unique syntax?

[bd1976llvm]

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.

[nickdesaulniers]

llvm/include/llvm/MC/MCContext.h

278

are there getters/setters for this member? Might be better than exposing the raw member.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

115

can a range based for loop be used here?

119

i shadows another i from above, and also isn't used.

673

LLVM coding style is just to use unsigned (drop the optional int) as per the Flags variable above.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

685

,unique,2 does not look bad to me. It allows you to use a unified syntax for .section and .pushsection (see ELFAsmParser::ParseSectionArguments). The unique number can be potentially useful if we ever support a syntax to reference name+unique_id (this use case can usually be accomplished by a temporary local label).

However, I now think that we should land this patch in its current state and file a bug for supporting -no-integrated-as properly.

I think you mean .c ---clang--> .s ---GNU as---> .o

I am concerned that clang -no-integrated-as is not so uncommon. For example, ClangBuiltLinux uses it (assembly support is still a bit lacking). Hope you can add a check.

MD_associated is a bit different. It has a strong requirement: the global variable is in a @llvm.compiler.used
It can still work without garbage collection semantics of SHF_LINK_ORDER.

Please chime in the binutils feature requests https://sourceware.org/bugzilla/show_bug.cgi?id=25380 https://sourceware.org/bugzilla/show_bug.cgi?id=25381 https://sourceware.org/ml/binutils/2019-11/msg00266.html :)

[bd1976llvm]

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.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

685

I think you mean .c ---clang--> .s ---GNU as---> .o

Yes I did. My understanding was that this is the usecase for "clang -no-integrated-as". Are there other usecases?

I am concerned that clang -no-integrated-as is not so uncommon. For example, ClangBuiltLinux uses it (assembly support is still a bit lacking).
Hope you can add a check.

I can ad a check however the only fix that I have for -no-integrated-as is to remove the SHF_MERGE flag from sections that symbols have been explicitly assigned to as in the https://reviews.llvm.org/D68101 patch. Is this acceptable?

[bd1976llvm]

updated the patch and description

[MaskRay]

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:) )

https://bugs.llvm.org/show_bug.cgi?id=43457: If we did two passes in AsmPrinter, we probably could decide sh_entsize in the first pass.
In the second pass, we emit the two global variables to the same section. In reality we have just one pass, so we need to decide the section when we are about to emit a global variable, without knowing whether there will be another section with a different sh_entsize.
This is the conundrum faced by the -filetype=asm backend.

For the -filetype=obj backend, it is simple: when there are conflicting sh_entsize fields => change sh_entsize to 0 (disabling merging).
In GNU ld and gold, if input sections are of different sh_entsize, no merge is performed.
lld performs limited merging (https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/merge-entsize2.s) but I doubt it has much value. (I noticed that explict-section-mergeable.ll output can crash lld due to a linked-to section being a SHF_MERGE section. I'll try fixing it tomorrow.)

If we can't afford doing two passes for -filetype=asm. Another choice is to update AsmParser. When two .section directives with different sh_entsize are parsed, consider them the same section and update sh_entsize to 0.
This solves a problem and is very simple. The assembler just does some tasks that the linker will eventually do, so I'll probably not call it a hack.
However, if we want to do that, we may need to ask GNU as maintainers.

I realized that there are several assembly syntax issues we need to discuss with GNU as maintainers. <del>We should start a thread.</del> Created https://sourceware.org/ml/binutils/2020-02/msg00091.html

llvm/lib/MC/MCContext.cpp

487

Instead of find(), you can use try_emplace. The return value will tell you whether a new element is inserted. If yes, you can increase the unique ID.

492

error: conditional expression is ambiguous; 'pair<[...], typename __decay_and_strip<unsigned int>::__type>' can be converted to 'pair<[...], typename __decay_and_strip<int>::__type>' and vice versa

0 -> 0u

llvm/test/CodeGen/X86/explict-section-mergeable.ll

6 ↗

(On Diff #242492)

There are a plethora of CHECK-NOT patterns which make the test difficult to read.

FileCheck %s --implicit-check-not=.section may be useful

[bd1976llvm]

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:) )

I have been not updated this patch as I was waiting to see how the related assembler work landed. Now that https://reviews.llvm.org/D73999 has landed I think we can proceed with this patch.

https://bugs.llvm.org/show_bug.cgi?id=43457: If we did two passes in AsmPrinter, we probably could decide sh_entsize in the first pass.
In the second pass, we emit the two global variables to the same section. In reality we have just one pass, so we need to decide the section when we are about to emit a global variable, without knowing whether there will be another section with a different sh_entsize.
This is the conundrum faced by the -filetype=asm backend.

I am presuming that being single pass is important. In terms of the linker doing merging it is better to emit symbols into sections with matching entry sizes rather than modifying the enty size.

For the -filetype=obj backend, it is simple: when there are conflicting sh_entsize fields => change sh_entsize to 0 (disabling merging).

I might have misunderstood this part of your comment. I don't think we want the filetype=obj backend to operate differently. Don't we want .ll -> .s ->.o to produce equivalent output to .ll -> .o, no?

In GNU ld and gold, if input sections are of different sh_entsize, no merge is performed.
lld performs limited merging (https://github.com/llvm/llvm-project/blob/master/lld/test/ELF/merge-entsize2.s) but I doubt it has much value. (I noticed that explict-section-mergeable.ll output can crash lld due to a linked-to section being a SHF_MERGE section. I'll try fixing it tomorrow.)

I would rather just create output that allows the linker to do merging - rather than make a judgement call as to whether merging is important or not.

I have updated the patch to address your specific review comments.

Can you take another look?

llvm/lib/MC/MCContext.cpp

487

I don't think can use try_emplace because I don't want to modify the map here just query it. However, I can improve the code by using an optional - I have done that.

[MaskRay]

The latest Diff does not apply on master cleanly, but it is generally looking good.

The basic mechanism is to create multiple sections with the same name, using LLVM's ", unique," assembly extension (I refer to such sections as unique/uniqued sections) if incompatible symbols are explicitly given the same section name.

unique linkage. assembly extension can be deleted because GNU as>=2.35 supports it as well.

Update: There will be a similar issue for associated symbols once https://reviews.llvm.org/D74006 lands.

This sentence can be deleted now.

Symbols with a an explicit assignment

a an -> an

The summary is very long. Can you simplify it?

llvm/include/llvm/MC/MCContext.h

323

excess parentheses

332

__attribute__

340

DenseSet

llvm/lib/MC/MCContext.cpp

462

The variable IsGenericSection can be omitted.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

614

if (Optional<StringRef> Prefix = F->getSectionPrefix())

621

/*MayAlwaysUsePrivate=*/true

[bd1976llvm]

Addressed review comments and rebased onto master.

In D72194#1918639, @MaskRay wrote:

The summary is very long. Can you simplify it?

I have rewritten the summary.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

621

Not done yet.

[MaskRay]

llvm/include/llvm/MC/MCContext.h

340

MCContext::renameELFSection is only used by GNU-style .zdebug_* (GNU-style is obsoleted by SHF_COMPRESSED).

I think we should not add code just to make .zdebug_* happy. Can we simplify code if .zdebug_str is not a big issue?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

774

Accidentally indented.

[bd1976llvm]

llvm/include/llvm/MC/MCContext.h

340

Thanks for pointing this out. Sorry for the slow response - I had COVID-19 and zlib problems to contend with before I could get back to you about this :)

.debug_str is mergeable and MC can produce .zdebug_str.

I'm not sure what reasonable behaviour should be with GNU style debug section compression. Two cases I have thought about are:

1. If a compatible symbol has a n explicit section of .debug_str.
2. If a compatible symbol has an explicit section of .zdebug_str.

I think 1. probably should result in the symbol appearing in .zdebug_str in the output since the section compression can be thought of as a post processing step. I think that 2. should probably be an error.

I don't really want to add extra logic and more LIT testing to support this if this is a corner case that no one will actually ever hit.

I would be happy to remove the code from MCContext::renameELFSection and leave the interaction with GNU-style debug section compression as a later enhancement to be added if needed.

What do you think?

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

621

Sorry - lost the change rebasing somehow,

774

Thanks!

[MaskRay]

llvm/include/llvm/MC/MCContext.h

340

I don't really want to add extra logic and more LIT testing to support this if this is a corner case that no one will actually ever hit.

Agree. That was why I asked whether we can simplify code and tests if .zdebug_* support can be dropped.

.zdebug* is an obsolete feature that may even be deleted in a future lld..

[bd1976llvm]

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

[nickdesaulniers]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

590

is this section prefixing specific to ELF, or common between all object file formats? Should the function name contain ELF if specific to ELF?

Is there a more specific guard we can use for ".data.rel.ro", returning an empty string otherwise?

[bd1976llvm]

Added llvm_unreachable to getSectionPrefixForGlobal()

llvm/include/llvm/MC/MCContext.h

340

I have removed the code. No tests simplification as the changes to MCContext::renameELFSection were not being exercised.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

590

is this section prefixing specific to ELF, or common between all object file formats? Should the function name contain ELF if specific to ELF?

This function is also used by web assembly which is why it doesn't have ELF in the name.

Is there a more specific guard we can use for ".data.rel.ro", returning an empty string otherwise?

An llvm_unreachable would make this code clearer. I'll add one.

[nickdesaulniers]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

590

This looks funny; an assert, followed by an indented return, followed by an llvm_unreachable? Was the assert supposed to be an if statement?

[nickdesaulniers]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

685

Note that while binutils added support for this in 2.35 (https://sourceware.org/bugzilla/show_bug.cgi?id=25380), we need to continue to support not emitting these extensions when the integrated assembler is set, otherwise we will regress all Linux kernel builds (most distro's don't ship 2.35 yet).

I can ad a check however the only fix that I have for -no-integrated-as is to remove the SHF_MERGE flag from sections that symbols have been explicitly assigned to as in the https://reviews.llvm.org/D68101 patch. Is this acceptable?

Yes. That's a good idea for a fix. Mergeable sections is an optimization opportunity; not doing it is the safe thing to do in that case. Please add a test and code to handle that case.

[bd1976llvm]

Corrected silly mistake.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

590

Stupid mistake when I was in a hurry. Thanks for spotting this.

685

Just wanted to question whether this is really required. By the time people are building with an LLVM with this patch distors will have been updated to 2.35, no? No problem doing this as long as it's really requried.

[nickdesaulniers]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

685

There are multiple disparate CI systems covering Linux kernel builds with ToT LLVM, and using released versions of binutils (pre-2.35). If this patch lands as is, they will all go red. And when the CI systems are red, it's an opportunity for multiple bugs to pile up, which can be difficult to dig your way out of.

With support for that case added, I'm ready to accept this patch, but we can't regress Linux kernel builds with ToT LLVM.

[bd1976llvm]

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

[nickdesaulniers]

Cool thanks for addition; finding and fixing things in code review is better than having patches reverted.

I plan on running this patch through some more extensive testing, but I'm happy with the change now.

@MaskRay please speak up today if you'd like to block submission on your review. Otherwise, I hope to sign off of on this by EOD after my testing.

[nickdesaulniers]

Patch looks good with everything I was able to throw at it; let's get that test fixed up so we don't land new tests that are red, and we should be good to go here.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

755

If we have a way of producing a non-fatal warning, that may be preferred to crashing the compiler, which encourages users to send us a bug report. In this case, we don't care about their buggy code with impossible to solve constraints, and the text you provide here should be enough information for them to solve their code.

Maybe a warning, but proceed with codegen? The warning can encourage them that referencing those variables is undefined behavior.

llvm/test/CodeGen/X86/explict-section-mergeable.ll

291 ↗

(On Diff #253031)

Running the unit tests with this diff version Diff 253031, this added test case fails. Please triple check by running:

$ llvm-lit -vv llvm/test/CodeGen/X86/explict-section-mergeable.ll

It seems like there's something tricky going on with output redirection, as

$ llc < llvm/test/CodeGen/X86/explict-section-mergeable.ll -mtriple=x86_64-linux-gnu --no-integrated-as 2>&1| less

produces no output related to checks, but

$ llc < llvm/test/CodeGen/X86/explict-section-mergeable.ll -mtriple=x86_64-linux-gnu --no-integrated-as 2>&1

does.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

721

Delete stray versions.

Delete the last sentence Until support is widespread...

In MC, there are several other places such workarounds exist, no need for stating this verbosely.

[MaskRay]

llvm/include/llvm/MC/MCContext.h

20

ELF.h should be after Dwarf.h

llvm/test/CodeGen/X86/explict-section-mergeable.ll

1 ↗

(On Diff #253031)

Nit: -linux-gnu can be deleted.

1 ↗

(On Diff #253031)

I just realized the file name made a typo. It should be explicit

16 ↗

(On Diff #253031)

Delete trailing whitespace

46 ↗

(On Diff #253031)

Changing 2 x i16 to 1 x i32 can enhance the test.

62 ↗

(On Diff #253031)

explicit_basic_5 does not test string vs non-string.

64 ↗

(On Diff #253031)

Change to 1 x i64 to enhance the test.

70 ↗

(On Diff #253031)

This can be deleted.

If you want to test a non-unnamed_addr constant, this should be 2 x i32 and the comment should be fixed.

108 ↗

(On Diff #253031)

This test does not seem correct.

.debug_str is a pre-allocated non-SHF_ALLOC section. A constant (SHF_ALLOC) cannot reuse the section.

114 ↗

(On Diff #253031)

This non-unnamed_addr test does not add coverage.

[MaskRay]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

735

The two assert may not be that useful. They can probably be deleted.

755

This code path is possible, which means we should use MCContext::reportError rather than report_fatal_error. In a bugfree world, report_fatal_error should not ever be called.

llvm/test/CodeGen/X86/explict-section-mergeable.ll

266 ↗

(On Diff #253031)

Changing to 1 x i32 can enhance the test.

273 ↗

(On Diff #253031)

explicit_asm_5 and explicit_asm_6 can be deleted. As a replacement, you can add another constant following explicit_asm_2 which is compatible with explicit_asm_[12]

280 ↗

(On Diff #253031)

.note.GNU-stack is unrelated to the test.

284 ↗

(On Diff #253031)

Delete not --crash. llc --no-integrated-as should not crash.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

735

These mirror the assert for associated symbols. If these are not useful then that can probably go as well?

755

report_fatal_error is used in similar circumstances in this file (which is why I chose it). See for example:

TargetLoweringObjectFileMachO::getExplicitSectionGlobal(
...
  if (!ErrorCode.empty()) {
    // If invalid, report the error with report_fatal_error.
    report_fatal_error("Global variable '" + GO->getName() +
                       "' has an invalid section specifier '" +
                       GO->getSection() + "': " + ErrorCode + ".");

In this file there are no current uses of reportError everything uses report_fatal_error so I wonder if what report_fatal_error -> MCContext::reportError is a more general refactor that should be done in a separate change?

I tried using MCContext::reportError but it didn't produce any output because llc does not seem to setup a SrcMgr correctly to allow for warning and errors. I have been unable to determine where to modify llc to setup a SrcMgr correctly today. I will look at this problem again tomorrow.

755

I think a warning is appropriate as the codegen is correct if the linker does not optimise the mergeable sections. Unfortunately, I can't get a warning to be emitted because llc does not seem to setup a SrcMgr correctly to allow for warning and errors. I have been unable to determine where to modify llc to setup a SrcMgr correctly. I will look at this problem again tomorrow.

llvm/test/CodeGen/X86/explict-section-mergeable.ll

1 ↗

(On Diff #253031)

Thanks! I'll make this change at the last moment because I think that a rename might mess up the phabricator comments?

46 ↗

(On Diff #253031)

I think that it is more obvious to a reader that 2 x i16 -> .rodata.cst4 and 2 x i32 -> .rodata.cst8, no?

62 ↗

(On Diff #253031)

explicit_basic_5 and explicit_basic_6 are both treated as strings which is why they get put into ".section .explicit_basic,"aMS",@progbits,4,unique,5" rather than ".section .explicit_basic,"aM",@progbits,8,unique,4".

This was part of the original internal bug report that led me to look at this problem originally.

The reader of the test should be able to see from the testing of the implicit section assignment (before any testing of explicitly assigned globals) how each symbol is treated by MC. For example near the top of the test we have:

;; Test implicit section assignment for symbols to ensure that the symbols
;; have the expected properties.
; CHECK: .section .rodata,"a",@progbits,unique,2
; CHECK: implicit_nonmergeable:
; CHECK: .section .rodata.cst4,"aM",@progbits,4
; CHECK: implicit_rodata_cst4:
; CHECK: .section .rodata.cst8,"aM",@progbits,8
; CHECK: implicit_rodata_cst8:
; CHECK: .section .rodata.str4.4,"aMS",@progbits,4
; CHECK: implicit_rodata_str4_4:

@implicit_nonmergeable  =              constant [2 x i16] [i16 1, i16 1]
@implicit_rodata_cst4   = unnamed_addr constant [2 x i16] [i16 1, i16 1]
@implicit_rodata_cst8   = unnamed_addr constant [2 x i32] [i32 1, i32 1]
@implicit_rodata_str4_4 = unnamed_addr constant [2 x i32] [i32 1, i32 0]

The test is careful to stick to symbol definitions that are as similar as possible, for readability.

64 ↗

(On Diff #253031)

It won't be considered as a string if I make this change.

70 ↗

(On Diff #253031)

This is testing that a non-mergeable constant is assigned to the "generic" .explicit_basic section when sections with that name have already been created by the explicit assignment of mergeable symbols to that section name. "constant [2 x i16] [i16 1, i16 1]" is established to be non-mergeable by the test for the implicit assignment of @implicit_nonmergeable earlier. I think it aids readability that the definition of non-mergeable global @explicit_basic_7 is very similar to the definition of @explicit_basic_1. No?

108 ↗

(On Diff #253031)

This is the current behaviour. You can also do this sort of thing using a linker script. This patch is only fixing entsize with explicit section assignment. Initially, I was going to handle *any* incompatibilities between the symbol properties and the properties of the section that the symbol is assigned to to fix https://bugs.llvm.org/show_bug.cgi?id=43457. However, I decided that if you can use a linker script to force incompatible symbols together then explicit section assignment can allow you to do it as well.

114 ↗

(On Diff #253031)

This test is here as a guard in case the implementation of the "default" sections changes in the future. Currently, it doesn't add any coverage over the similar test for an "implicit" section - but it might in the future.

266 ↗

(On Diff #253031)

These need to be strings. See other comments.

273 ↗

(On Diff #253031)

Done. Also, these test cases didn't match the comments so I updated them.

280 ↗

(On Diff #253031)

I don't see how to avoid generating it. I need a check for it as I am using --implicit-check-not=.section.

284 ↗

(On Diff #253031)

This seems to be the common pattern to test for report_fatal_error, see e.g. llvm/test/CodeGen/X86/macho-comdat.ll; llc does seem to crash if errors are emitted.

291 ↗

(On Diff #253031)

Thanks for this report. The test passes for me on windows. I will investigate further tomorrow. If we move to emitting a warning or non-fatal error then this problem may be bypasssed.

[bd1976llvm]

Addressed review comments.
Cosmetic test improvements.

[bd1976llvm]

Addressed header file order review comment

[bd1976llvm]

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

[bd1976llvm]

@nickdesaulniers and @MaskRay,

With regards to the diagnostic. The whole thing is a bit of a mess. Since I originally authored this patch the behaviour of report_fatal_error has been changed, see comments on: https://reviews.llvm.org/rG8cedf0e2994c1a258902ed931abdec5f94725a55. Currently, it is not possible to emit a warning or error using MCContext::reportWarning/Error. I have changed the code to use reportError. For now, this results in a call to report_fatal_error, which results in a call to abort, which necessitates the use of "not --crash" in the LIT test.

As @nickdesaulniers had reported the test was broken on Linux (although it did work on Windows). Looking at this failure I don't think there is a guarantee of how much stdout will be printed if report_fatal_error is called. Therefore, I have split the original testcase into two; first the error message is checked, second the suppression of mergeable section generation is checked.

With regards to whether we should emit a warning or an error. IMO It should be an error as: there is no way to turn off optimizing mergeable sections (in most linkers), warnings are often ignored, and we don't want to create broken output after linking.

[nickdesaulniers]

In D72194#1953100, @bd1976llvm wrote:

Split testing of error and output to defeat redirection race.

Heh, I was thinking about this this morning; how could we see output when not redirecting, but not see output when piped (to less or FileCheck). The only thing I could think of was a race, or rather forgetting to flush outs(), errs(), and dbgs() prior to exit. Could this be fixed with a call to raw_ostream::flush somewhere? Probably, but that's not your problem to solve (and adding flush calls explicitly here would be duct tape for a larger architectural issue in LLVM).

In D72194#1953155, @bd1976llvm wrote:

With regards to the diagnostic. The whole thing is a bit of a mess. Since I originally authored this patch the behaviour of report_fatal_error has been changed, see comments on: https://reviews.llvm.org/rG8cedf0e2994c1a258902ed931abdec5f94725a55.

Bad luck.

llvm/test/CodeGen/X86/explict-section-mergeable.ll

290 ↗

(On Diff #253957)

Would it be better to make this a separate test file, rather using echo, but then passing this file to filecheck? I think a good general rule of thumb is that %s is used as both what generates input to Filecheck over the pipe, AND input to Filecheck itself. The check doesn't rely on anything else from this file IIUC. But this is only one test case. @MaskRay , thoughts?

[nickdesaulniers]

Anyways, tests are now green for me, and I'm happy with the patch. Thanks for all of the work that went into this @bd1976llvm . It doesn't really matter to me which way we resolve the above comment about echo.

[efriedma]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

755

MCContext::reportError is intended for assembler errors. In CodeGen, you probably want to be calling LLVMContext::diagnose.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

755

Hi @efriedma, thanks for suggesting this. Sorry I couldn't look into it yesterday - I can take a look today (uk time).

I don't have a good handle on LLVM diagnostics. From poking/looking around I got the impression that in LLVM report_fatal_error was commonly used and that recently https://reviews.llvm.org/rG8cedf0e2994c1a258902ed931abdec5f94725a55 changed the behaviour so that now this aborts. I did see that MCContext::reportError/Warning was only setup when assembling; however, I was under the impression that this should be "the" way to report errors (from MC at least) and that it not being setup to do so outside of assembling was an oversight that hasn't be recognized until now as people were happy with report_fatal_error (before the recent behaviour change).

Do you have any code pointers/insight into the intended design here or thoughts on how LLVM should be moving w.r.t diagnostics .

Is there anyone else who would be good to bring into this?

Any input would be great. Thanks!

[efriedma]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

755

The reason the infrastructure here is, and has remained, sort of shaky, is that we generate very few diagnostics this way in practice. For IR generated by clang, outside of inline asm, it's almost always a frontend bug if we trigger an error in the backend.

If the backend triggers an assembler diagnostic outside of inline asm, that's 100% a bug. We should not be generating invalid assembly; that would imply the backend doesn't understand the code its generating. But TargetLoweringObjectFileELF is part of LLVM codegen, not MC, despite the fact that that it works almost exclusively with MC objects. So we shouldn't consider this an "assembler" diagnostic.

Generally, the reason people tend to use report_fatal_error is that it's easy: you don't have to worry about formatting the diagnostic, or the implications of what happens after the error is triggered, or a location for the diagnostic. But really, we want to avoid that if possible. The documentation here could be improved, though.

[bd1976llvm]

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

755

@efriedma - really appreciate you taking the time to explain this. I have run out of time to update this patch this week. On Monday I will look into using LLVMContext::diagnose as you suggested. Thanks!

[bd1976llvm]

Report error using LLVM diagnostics framework.

[nickdesaulniers]

interdiff LGTM, please wait for @efriedma 's feedback, too before landing. Thanks for this improvement!

[MaskRay]

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

[efriedma]

New diagnostics code looks fine. (I didn't try to review the new section logic closely).

[bd1976llvm]

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".

[bd1976llvm]

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

I didn't understand the terminology here. I presume "interdiff" refers to this diff now having clang and llvm changes in one diff? Is this a problem? I was under the impression that with the mono-repository now cross-project diffs are ok? To be clear I don't have a problem moving the clang error handling to another review if that's not correct/desirable. I just don't understand the benefit.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

755

I have tried to plum in the minimum to report errors using LLVM and Clangs diagnostic stuff. Please take a look.

[MaskRay]

In D72194#1968249, @bd1976llvm wrote:

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

I didn't understand the terminology here. I presume "interdiff" refers to this diff now having clang and llvm changes in one diff? Is this a problem? I was under the impression that with the mono-repository now cross-project diffs are ok? To be clear I don't have a problem moving the clang error handling to another review if that's not correct/desirable. I just don't understand the benefit.

For clang stuff you may need another groups of folks working more actively on that area. My intuition says they may have more opinions on how the diagnostics should be approached:/ For that group people, they may not really want to see the SHF_MERGE ELF things.

I will also accept the patch if you split off the clang changes.

[bd1976llvm]

• Removed Clang parts of the diff. Error behaviour from llc and clang is still acceptable without the Clang parts. A generic "error: <reason>" is issued rather than a "backend" or "lowering" error as we had before... but the message contents should still allow for easy diagnosis. I will put up the Clang parts as a separate diff.
• Rebased the patch.

[bd1976llvm]

Thanks for the interdiff rationale. Please take another look!

[MaskRay]

Thanks! clang/test/CodeGen/cfstring-elf-sections-x86_64.c needs to be deleted as well.

[bd1976llvm]

I would like to keep this as part of this patch.

In D72194#1969772, @MaskRay wrote:

Thanks! clang/test/CodeGen/cfstring-elf-sections-x86_64.c needs to be deleted as well.

I don't think that I can remove clang/test/CodeGen/cfstring-elf-sections-x86_64.c because if I do then I can't commit this as it will break that Clang test?

[MaskRay]

In D72194#1970292, @bd1976llvm wrote:

I would like to keep this as part of this patch.

In D72194#1969772, @MaskRay wrote:

Thanks! clang/test/CodeGen/cfstring-elf-sections-x86_64.c needs to be deleted as well.

I don't think that I can remove clang/test/CodeGen/cfstring-elf-sections-x86_64.c because if I do then I can't commit this as it will break that Clang test?

Ah, yes. Thanks for the clarification.

[bd1976llvm]

Thanks to everyone for reviewing/guiding me though this change. When I started this I hadn't looked at lowering in LLVM in any detail so thanks for sticking with reviewing the code when I was basically learning on the job.

Due to COVID disruption I have waited until today to push as I wanted to be sure that I had the bandwidth to deal with any disruption this causes.

I believe that we have made a change that has a good likelihood of being acceptable whilst also offering consistent and understandable behaviour. Nevertheless, this change could easily cause problems. For example, examining the change in behaviour in "clang/test/CodeGen/cfstring-elf-sections-x86_64.c " it is possible that some toolchains may reject SHF_MERGE .rodata sections. We agreed, when starting to look at this, to "make a reasonable change and see what breaks".. so here goes.

clang/test/CodeGen/cfstring-elf-sections-x86_64.c

 // REQUIRES: x86-registered-target
 
 // RUN: %clang_cc1 -triple x86_64-elf -S %s -o - | FileCheck %s -check-prefix CHECK-ELF-DATA-SECTION
 
 typedef struct __CFString *CFStringRef;
 const CFStringRef one = (CFStringRef)__builtin___CFStringMakeConstantString("one");
 const CFStringRef two = (CFStringRef)__builtin___CFStringMakeConstantString("\xef\xbf\xbd\x74\xef\xbf\xbd\x77\xef\xbf\xbd\x6f");
 
 // CHECK-ELF-DATA-SECTION: .type .L.str,@object
-// CHECK-ELF-DATA-SECTION: .section .rodata,"a",@progbits
+// CHECK-ELF-DATA-SECTION: .section .rodata,"aMS",@progbits,1,unique,1
 // CHECK-ELF-DATA-SECTION: .L.str:
 // CHECK-ELF-DATA-SECTION: .asciz "one"
 
 // CHECK-ELF-DATA-SECTION: .type .L.str.1,@object
-// CHECK-ELF-DATA-SECTION: .section .rodata,"a",@progbits
+// CHECK-ELF-DATA-SECTION: .section .rodata,"aMS",@progbits,2,unique,2
 // CHECK-ELF-DATA-SECTION: .L.str.1:
 // CHECK-ELF-DATA-SECTION: .short 65533
 // CHECK-ELF-DATA-SECTION: .short 116
 // CHECK-ELF-DATA-SECTION: .short 65533
 // CHECK-ELF-DATA-SECTION: .short 119
 // CHECK-ELF-DATA-SECTION: .short 65533
 // CHECK-ELF-DATA-SECTION: .short 111
 // CHECK-ELF-DATA-SECTION: .short 0

llvm/include/llvm/IR/DiagnosticInfo.h

 
 /// Defines the different supported kind of a diagnostic.
 /// This enum should be extended with a new ID for each added concrete subclass.
 enum DiagnosticKind {
   DK_InlineAsm,
   DK_ResourceLimit,
   DK_StackSize,
   DK_Linker,
+  DK_Lowering,
   DK_DebugMetadataVersion,
   DK_DebugMetadataInvalid,
   DK_ISelFallback,
   DK_SampleProfile,
   DK_OptimizationRemark,
   DK_OptimizationRemarkMissed,
   DK_OptimizationRemarkAnalysis,
   DK_OptimizationRemarkAnalysisFPCommute,

llvm/include/llvm/MC/MCContext.h

 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/BinaryFormat/Dwarf.h"
+#include "llvm/BinaryFormat/ELF.h"
 #include "llvm/BinaryFormat/XCOFF.h"
 #include "llvm/MC/MCAsmMacro.h"
 #include "llvm/MC/MCDwarf.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/MC/MCTargetOptions.h"
 #include "llvm/MC/SectionKind.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Compiler.h"
struct XCOFFSectionKey {
 
       bool operator<(const XCOFFSectionKey &Other) const {
         return std::tie(SectionName, MappingClass) <
                std::tie(Other.SectionName, Other.MappingClass);
       }
     };
 
     StringMap<MCSectionMachO *> MachOUniquingMap;
     std::map<ELFSectionKey, MCSectionELF *> ELFUniquingMap;
     std::map<COFFSectionKey, MCSectionCOFF *> COFFUniquingMap;
     std::map<WasmSectionKey, MCSectionWasm *> WasmUniquingMap;
     std::map<XCOFFSectionKey, MCSectionXCOFF *> XCOFFUniquingMap;
     StringMap<bool> RelSecNames;
 
     SpecificBumpPtrAllocator<MCSubtargetInfo> MCSubtargetAllocator;
 
     /// Do automatic reset in destructor
MCSectionELF *createELFSectionImpl(StringRef Section, unsigned Type,
                                        unsigned EntrySize,
                                        const MCSymbolELF *Group,
                                        unsigned UniqueID,
                                        const MCSymbolELF *LinkedToSym);
 
     /// Map of currently defined macros.
     StringMap<MCAsmMacro> MacroMap;
 
+    struct ELFEntrySizeKey {
+      std::string SectionName;
+      unsigned Flags;
+      unsigned EntrySize;
+
+      ELFEntrySizeKey(StringRef SectionName, unsigned Flags, unsigned EntrySize)
+          : SectionName(SectionName), Flags(Flags), EntrySize(EntrySize) {}
+
+      bool operator<(const ELFEntrySizeKey &Other) const {
+        if (SectionName != Other.SectionName)
+          return SectionName < Other.SectionName;
+        if ((Flags & ELF::SHF_STRINGS) != (Other.Flags & ELF::SHF_STRINGS))
+          return Other.Flags & ELF::SHF_STRINGS;
+        return EntrySize < Other.EntrySize;
+      }
+    };
+
+    // Symbols must be assigned to a section with a compatible entry
+    // size. This map is used to assign unique IDs to sections to
+    // distinguish between sections with identical names but incompatible entry
+    // sizes. This can occur when a symbol is explicitly assigned to a
+    // section, e.g. via __attribute__((section("myname"))).
+    std::map<ELFEntrySizeKey, unsigned> ELFEntrySizeMap;
+
+    // This set is used to record the generic mergeable section names seen.
+    // These are sections that are created as mergeable e.g. .debug_str. We need
+    // to avoid assigning non-mergeable symbols to these sections. It is used
+    // to prevent non-mergeable symbols being explicitly assigned  to mergeable
+    // sections (e.g. via _attribute_((section("myname")))).
+    DenseSet<StringRef> ELFSeenGenericMergeableSections;
+
   public:
     explicit MCContext(const MCAsmInfo *MAI, const MCRegisterInfo *MRI,
                        const MCObjectFileInfo *MOFI,
                        const SourceMgr *Mgr = nullptr,
                        MCTargetOptions const *TargetOpts = nullptr,
                        bool DoAutoReset = true);
     MCContext(const MCContext &) = delete;
     MCContext &operator=(const MCContext &) = delete;
MCSectionELF *createELFRelSection(const Twine &Name, unsigned Type,
                                       unsigned Flags, unsigned EntrySize,
                                       const MCSymbolELF *Group,
                                       const MCSectionELF *RelInfoSection);
 
     void renameELFSection(MCSectionELF *Section, StringRef Name);
 
     MCSectionELF *createELFGroupSection(const MCSymbolELF *Group);
 
+    void recordELFMergeableSectionInfo(StringRef SectionName, unsigned Flags,
+                                       unsigned UniqueID, unsigned EntrySize);
+
+    bool isELFImplicitMergeableSectionNamePrefix(StringRef Name);
+
+    bool isELFGenericMergeableSection(StringRef Name);
+
+    Optional<unsigned> getELFUniqueIDForEntsize(StringRef SectionName,
+                                                unsigned Flags,
+                                                unsigned EntrySize);
+
     MCSectionCOFF *getCOFFSection(StringRef Section, unsigned Characteristics,
                                   SectionKind Kind, StringRef COMDATSymName,
                                   int Selection,
                                   unsigned UniqueID = GenericSectionID,
                                   const char *BeginSymName = nullptr);
 
     MCSectionCOFF *getCOFFSection(StringRef Section, unsigned Characteristics,
                                   SectionKind Kind,

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp

 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineModuleInfo.h"
 #include "llvm/CodeGen/MachineModuleInfoImpls.h"
 #include "llvm/IR/Comdat.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/IR/DiagnosticPrinter.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalObject.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/Mangler.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"

 void TargetLoweringObjectFileELF::Initialize(MCContext &Ctx,
                                              const TargetMachine &TgtM) {
   TargetLoweringObjectFile::Initialize(Ctx, TgtM);
   TM = &TgtM;
 
   CodeModel::Model CM = TgtM.getCodeModel();
   InitializeELF(TgtM.Options.UseInitArray);
 
   switch (TgtM.getTargetTriple().getArch()) {
   case Triple::arm:
   case Triple::armeb:
   case Triple::thumb:
   case Triple::thumbeb:
     if (Ctx.getAsmInfo()->getExceptionHandlingType() == ExceptionHandling::ARM)
       break;
     // Fallthrough if not using EHABI
     LLVM_FALLTHROUGH;
   case Triple::ppc:
   case Triple::x86:
     PersonalityEncoding = isPositionIndependent()
                               ? dwarf::DW_EH_PE_indirect |
else {
     // We shouldn't have mergeable C strings or mergeable constants that we
     // didn't handle above.
     assert(!Kind.isMergeableCString() && "unknown string width");
     assert(!Kind.isMergeableConst() && "unknown data width");
     return 0;
   }
 }
 
+/// Return the section prefix name used by options FunctionsSections and
+/// DataSections.
+static StringRef getSectionPrefixForGlobal(SectionKind Kind) {
+  if (Kind.isText())
+    return ".text";
+  if (Kind.isReadOnly())
+    return ".rodata";
+  if (Kind.isBSS())
+    return ".bss";
+  if (Kind.isThreadData())
+    return ".tdata";
+  if (Kind.isThreadBSS())
+    return ".tbss";
+  if (Kind.isData())
+    return ".data";
+  if (Kind.isReadOnlyWithRel())
+    return ".data.rel.ro";
+  llvm_unreachable("Unknown section kind");
+}
+
+static SmallString<128>
+getELFSectionNameForGlobal(const GlobalObject *GO, SectionKind Kind,
+                           Mangler &Mang, const TargetMachine &TM,
+                           unsigned EntrySize, bool UniqueSectionName) {
+  SmallString<128> Name;
+  if (Kind.isMergeableCString()) {
+    // We also need alignment here.
+    // FIXME: this is getting the alignment of the character, not the
+    // alignment of the global!
+    unsigned Align = GO->getParent()->getDataLayout().getPreferredAlignment(
+        cast<GlobalVariable>(GO));
+
+    std::string SizeSpec = ".rodata.str" + utostr(EntrySize) + ".";
+    Name = SizeSpec + utostr(Align);
+  } else if (Kind.isMergeableConst()) {
+    Name = ".rodata.cst";
+    Name += utostr(EntrySize);
+  } else {
+    Name = getSectionPrefixForGlobal(Kind);
+  }
+
+  if (const auto *F = dyn_cast<Function>(GO)) {
+    if (Optional<StringRef> Prefix = F->getSectionPrefix())
+      Name += *Prefix;
+  }
+
+  if (UniqueSectionName) {
+    Name.push_back('.');
+    TM.getNameWithPrefix(Name, GO, Mang, /*MayAlwaysUsePrivate*/true);
+  }
+  return Name;
+}
+
+namespace {
+class LoweringDiagnosticInfo : public DiagnosticInfo {
+  const Twine &Msg;
+
+public:
+  LoweringDiagnosticInfo(const Twine &DiagMsg,
+                         DiagnosticSeverity Severity = DS_Error)
+      : DiagnosticInfo(DK_Lowering, Severity), Msg(DiagMsg) {}
+  void print(DiagnosticPrinter &DP) const override { DP << Msg; }
+};
+}
+
 MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
     const GlobalObject *GO, SectionKind Kind, const TargetMachine &TM) const {
   StringRef SectionName = GO->getSection();
 
   // Check if '#pragma clang section' name is applicable.
   // Note that pragma directive overrides -ffunction-section, -fdata-section
   // and so section name is exactly as user specified and not uniqued.
   const GlobalVariable *GV = dyn_cast<GlobalVariable>(GO);
MCSection *TargetLoweringObjectFileELF::getExplicitSectionGlobal(
 
   StringRef Group = "";
   unsigned Flags = getELFSectionFlags(Kind);
   if (const Comdat *C = getELFComdat(GO)) {
     Group = C->getName();
     Flags |= ELF::SHF_GROUP;
   }
 
+  unsigned EntrySize = getEntrySizeForKind(Kind);
+
   // A section can have at most one associated section. Put each global with
   // MD_associated in a unique section.
   unsigned UniqueID = MCContext::GenericSectionID;
   const MCSymbolELF *LinkedToSym = getLinkedToSymbol(GO, TM);
   if (LinkedToSym) {
     UniqueID = NextUniqueID++;
     Flags |= ELF::SHF_LINK_ORDER;
+  } else {
+    if (getContext().getAsmInfo()->useIntegratedAssembler()) {
+      // Symbols must be placed into sections with compatible entry
+      // sizes. Generate unique sections for symbols that have not
+      // been assigned to compatible sections.
+      if (Flags & ELF::SHF_MERGE) {
+        auto maybeID = getContext().getELFUniqueIDForEntsize(SectionName, Flags,
+                                                             EntrySize);
+        if (maybeID)
+          UniqueID = *maybeID;
+        else {
+          // If the user has specified the same section name as would be created
+          // implicitly for this symbol e.g. .rodata.str1.1, then we don't need
+          // to unique the section as the entry size for this symbol will be
+          // compatible with implicitly created sections.
+          SmallString<128> ImplicitSectionNameStem = getELFSectionNameForGlobal(
+              GO, Kind, getMangler(), TM, EntrySize, false);
+          if (!(getContext().isELFImplicitMergeableSectionNamePrefix(
+                    SectionName) &&
+                SectionName.startswith(ImplicitSectionNameStem)))
+            UniqueID = NextUniqueID++;
+        }
+      } else {
+        // We need to unique the section if the user has explicity
+        // assigned a non-mergeable symbol to a section name for
+        // a generic mergeable section.
+        if (getContext().isELFGenericMergeableSection(SectionName)) {
+          auto maybeID = getContext().getELFUniqueIDForEntsize(
+              SectionName, Flags, EntrySize);
+          UniqueID = maybeID ? *maybeID : NextUniqueID++;
+        }
+      }
+    } else {
+      // If two symbols with differing sizes end up in the same mergeable
+      // section that section can be assigned an incorrect entry size. To avoid
+      // this we usually put symbols of the same size into distinct mergeable
+      // sections with the same name. Doing so relies on the ",unique ,"
+      // assembly feature. This feature is not avalible until bintuils
+      // version 2.35 (https://sourceware.org/bugzilla/show_bug.cgi?id=25380).
+      Flags &= ~ELF::SHF_MERGE;
+      EntrySize = 0;
+    }
   }
 
   MCSectionELF *Section = getContext().getELFSection(
       SectionName, getELFSectionType(SectionName, Kind), Flags,
-      getEntrySizeForKind(Kind), Group, UniqueID, LinkedToSym);
+      EntrySize, Group, UniqueID, LinkedToSym);
   // Make sure that we did not get some other section with incompatible sh_link.
   // This should not be possible due to UniqueID code above.
   assert(Section->getLinkedToSymbol() == LinkedToSym &&
          "Associated symbol mismatch between sections");
-  return Section;
+
+  if (!getContext().getAsmInfo()->useIntegratedAssembler()) {
+    // If we are not using the integrated assembler then this symbol might have
+    // been placed in an incompatible mergeable section. Emit an error if this
+    // is the case to avoid creating broken output.
+    if ((Section->getFlags() & ELF::SHF_MERGE) &&
+        (Section->getEntrySize() != getEntrySizeForKind(Kind)))
+      GO->getContext().diagnose(LoweringDiagnosticInfo(
+          "Symbol '" + GO->getName() + "' from module '" +
+          (GO->getParent() ? GO->getParent()->getSourceFileName() : "unknown") +
+          "' required a section with entry-size=" +
+          Twine(getEntrySizeForKind(Kind)) + " but was placed in section '" +
+          SectionName + "' with entry-size=" + Twine(Section->getEntrySize()) +
+          ": Explicit assignment by pragma or attribute of an incompatible "
+          "symbol to this section?"));
   }
 
-/// Return the section prefix name used by options FunctionsSections and
-/// DataSections.
-static StringRef getSectionPrefixForGlobal(SectionKind Kind) {
-  if (Kind.isText())
-    return ".text";
-  if (Kind.isReadOnly())
-    return ".rodata";
-  if (Kind.isBSS())
-    return ".bss";
-  if (Kind.isThreadData())
-    return ".tdata";
-  if (Kind.isThreadBSS())
-    return ".tbss";
-  if (Kind.isData())
-    return ".data";
-  assert(Kind.isReadOnlyWithRel() && "Unknown section kind");
-  return ".data.rel.ro";
+  return Section;
 }
 
 static MCSectionELF *selectELFSectionForGlobal(
     MCContext &Ctx, const GlobalObject *GO, SectionKind Kind, Mangler &Mang,
     const TargetMachine &TM, bool EmitUniqueSection, unsigned Flags,
     unsigned *NextUniqueID, const MCSymbolELF *AssociatedSymbol) {
 
   StringRef Group = "";
   if (const Comdat *C = getELFComdat(GO)) {
     Flags |= ELF::SHF_GROUP;
     Group = C->getName();
   }
 
   // Get the section entry size based on the kind.
   unsigned EntrySize = getEntrySizeForKind(Kind);
 
-  SmallString<128> Name;
-  if (Kind.isMergeableCString()) {
-    // We also need alignment here.
-    // FIXME: this is getting the alignment of the character, not the
-    // alignment of the global!
-    unsigned Align = GO->getParent()->getDataLayout().getPreferredAlignment(
-        cast<GlobalVariable>(GO));
-
-    std::string SizeSpec = ".rodata.str" + utostr(EntrySize) + ".";
-    Name = SizeSpec + utostr(Align);
-  } else if (Kind.isMergeableConst()) {
-    Name = ".rodata.cst";
-    Name += utostr(EntrySize);
-  } else {
-    Name = getSectionPrefixForGlobal(Kind);
-  }
-
-  if (const auto *F = dyn_cast<Function>(GO)) {
-    const auto &OptionalPrefix = F->getSectionPrefix();
-    if (OptionalPrefix)
-      Name += *OptionalPrefix;
-  }
-
+  bool UniqueSectionName = false;
   unsigned UniqueID = MCContext::GenericSectionID;
   if (EmitUniqueSection) {
     if (TM.getUniqueSectionNames()) {
-      Name.push_back('.');
-      TM.getNameWithPrefix(Name, GO, Mang, true /*MayAlwaysUsePrivate*/);
+      UniqueSectionName = true;
     } else {
       UniqueID = *NextUniqueID;
       (*NextUniqueID)++;
     }
   }
+  SmallString<128> Name = getELFSectionNameForGlobal(
+      GO, Kind, Mang, TM, EntrySize, UniqueSectionName);
+
   // Use 0 as the unique ID for execute-only text.
   if (Kind.isExecuteOnly())
     UniqueID = 0;
   return Ctx.getELFSection(Name, getELFSectionType(Name, Kind), Flags,
                            EntrySize, Group, UniqueID, AssociatedSymbol);
 }
 
 MCSection *TargetLoweringObjectFileELF::SelectSectionForGlobal(

llvm/lib/MC/MCContext.cpp

void MCContext::reset() {
   CVContext.reset();
 
   MachOUniquingMap.clear();
   ELFUniquingMap.clear();
   COFFUniquingMap.clear();
   WasmUniquingMap.clear();
   XCOFFUniquingMap.clear();
 
+  ELFEntrySizeMap.clear();
+  ELFSeenGenericMergeableSections.clear();
+
   NextID.clear();
   AllowTemporaryLabels = true;
   DwarfLocSeen = false;
   GenDwarfForAssembly = false;
   GenDwarfFileNumber = 0;
 
   HadError = false;
 }
else if (Flags & ELF::SHF_EXECINSTR)
     Kind = SectionKind::getText();
   else
     Kind = SectionKind::getReadOnly();
 
   MCSectionELF *Result =
       createELFSectionImpl(CachedName, Type, Flags, Kind, EntrySize, GroupSym,
                            UniqueID, LinkedToSym);
   Entry.second = Result;
+
+  recordELFMergeableSectionInfo(Result->getSectionName(), Result->getFlags(),
+                                Result->getUniqueID(), Result->getEntrySize());
+
   return Result;
 }
 
 MCSectionELF *MCContext::createELFGroupSection(const MCSymbolELF *Group) {
   return createELFSectionImpl(".group", ELF::SHT_GROUP, 0,
                               SectionKind::getReadOnly(), 4, Group,
                               MCSection::NonUniqueID, nullptr);
 }
 
+void MCContext::recordELFMergeableSectionInfo(StringRef SectionName,
+                                              unsigned Flags, unsigned UniqueID,
+                                              unsigned EntrySize) {
+  bool IsMergeable = Flags & ELF::SHF_MERGE;
+  if (IsMergeable && (UniqueID == GenericSectionID))
+    ELFSeenGenericMergeableSections.insert(SectionName);
+
+  // For mergeable sections or non-mergeable sections with a generic mergeable
+  // section name we enter their Unique ID into the ELFEntrySizeMap so that
+  // compatible globals can be assigned to the same section.
+  if (IsMergeable || isELFGenericMergeableSection(SectionName)) {
+    ELFEntrySizeMap.insert(std::make_pair(
+        ELFEntrySizeKey{SectionName, Flags, EntrySize}, UniqueID));
+  }
+}
+
+bool MCContext::isELFImplicitMergeableSectionNamePrefix(StringRef SectionName) {
+  return SectionName.startswith(".rodata.str") ||
+         SectionName.startswith(".rodata.cst");
+}
+
+bool MCContext::isELFGenericMergeableSection(StringRef SectionName) {
+  return isELFImplicitMergeableSectionNamePrefix(SectionName) ||
+         ELFSeenGenericMergeableSections.count(SectionName);
+}
+
+Optional<unsigned> MCContext::getELFUniqueIDForEntsize(StringRef SectionName,
+                                                       unsigned Flags,
+                                                       unsigned EntrySize) {
+  auto I = ELFEntrySizeMap.find(
+      MCContext::ELFEntrySizeKey{SectionName, Flags, EntrySize});
+  return (I != ELFEntrySizeMap.end()) ? Optional<unsigned>(I->second) : None;
+}
+
 MCSectionCOFF *MCContext::getCOFFSection(StringRef Section,
                                          unsigned Characteristics,
                                          SectionKind Kind,
                                          StringRef COMDATSymName, int Selection,
                                          unsigned UniqueID,
                                          const char *BeginSymName) {
   MCSymbol *COMDATSymbol = nullptr;
   if (!COMDATSymName.empty()) {
     COMDATSymbol = getOrCreateSymbol(COMDATSymName);

llvm/test/CodeGen/X86/explicit-section-mergeable.ll

+; RUN: llc < %s -mtriple=x86_64 -unique-section-names=0 -data-sections 2>&1 \
+; RUN:     | FileCheck %s
+
+;; Several sections are created via inline assembly. We add checks
+;; for these lines as we want to use --implicit-check-not to reduce the
+;; number of checks in this file.
+; CHECK: .section .asm_mergeable1,"aMS",@progbits,2
+; CHECK-NEXT: .section .asm_nonmergeable1,"a",@progbits
+; CHECK-NEXT: .section .asm_mergeable2,"aMS",@progbits,2
+; CHECK-NEXT: .section .asm_nonmergeable2,"a",@progbits
+
+;; Test implicit section assignment for symbols
+; CHECK: .section .data,"aw",@progbits,unique,1
+; CHECK: uniquified:
+
+;; Create a uniquified symbol (as -unique-section-names=0) to test the uniqueID
+;; interaction with mergeable symbols.
+@uniquified = global i32 1
+
+;; Test implicit section assignment for symbols to ensure that the symbols
+;; have the expected properties.
+; CHECK: .section .rodata,"a",@progbits,unique,2
+; CHECK: implicit_nonmergeable:
+; CHECK: .section .rodata.cst4,"aM",@progbits,4
+; CHECK: implicit_rodata_cst4:
+; CHECK: .section .rodata.cst8,"aM",@progbits,8
+; CHECK: implicit_rodata_cst8:
+; CHECK: .section .rodata.str4.4,"aMS",@progbits,4
+; CHECK: implicit_rodata_str4_4:
+
+@implicit_nonmergeable  =              constant [2 x i16] [i16 1, i16 1]
+@implicit_rodata_cst4   = unnamed_addr constant [2 x i16] [i16 1, i16 1]
+@implicit_rodata_cst8   = unnamed_addr constant [2 x i32] [i32 1, i32 1]
+@implicit_rodata_str4_4 = unnamed_addr constant [2 x i32] [i32 1, i32 0]
+
+;; Basic checks that mergeable globals are placed into multiple distinct
+;; sections with the same name and a compatible entry size.
+
+; CHECK: .section .explicit_basic,"aM",@progbits,4,unique,3
+; CHECK: explicit_basic_1:
+; CHECK: explicit_basic_2:
+
+;; Assign a mergeable global to a non-existing section.
+@explicit_basic_1 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_basic"
+;; Assign a compatible mergeable global to the previous section.
+@explicit_basic_2 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_basic"
+
+; CHECK: .section .explicit_basic,"aM",@progbits,8,unique,4
+; CHECK: explicit_basic_3:
+; CHECK: explicit_basic_4:
+
+;; Assign a symbol with an incompatible entsize (different size) to a section with the same name.
+@explicit_basic_3 = unnamed_addr constant [2 x i32] [i32 1, i32 1], section ".explicit_basic"
+;; Assign a compatible mergeable global to the previous section.
+@explicit_basic_4 = unnamed_addr constant [2 x i32] [i32 1, i32 1], section ".explicit_basic"
+
+; CHECK: .section .explicit_basic,"aMS",@progbits,4,unique,5
+; CHECK: explicit_basic_5:
+; CHECK: explicit_basic_6:
+
+;; Assign a symbol with an incompatible entsize (string vs non-string) to a section with the same name.
+@explicit_basic_5 = unnamed_addr constant [2 x i32] [i32 1, i32 0], section ".explicit_basic"
+;; Assign a compatible mergeable global to the previous section.
+@explicit_basic_6 = unnamed_addr constant [2 x i32] [i32 1, i32 0], section ".explicit_basic"
+
+; CHECK: .section .explicit_basic,"a",@progbits
+; CHECK: explicit_basic_7:
+
+;; Assign a symbol with an incompatible entsize (non-mergeable) to a mergeable section created explicitly.
+@explicit_basic_7 = constant [2 x i16] [i16 1, i16 1], section ".explicit_basic"
+
+; CHECK: .section .explicit_initially_nonmergeable,"a",@progbits
+; CHECK: explicit_basic_8:
+; CHECK: .section .explicit_initially_nonmergeable,"aM",@progbits,4,unique,6
+; CHECK: explicit_basic_9:
+
+;; Assign a mergeble symbol to a section that initially had a non-mergeable symbol explicitly assigned to it.
+@explicit_basic_8 = constant [2 x i16] [i16 1, i16 1], section ".explicit_initially_nonmergeable"
+@explicit_basic_9 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_initially_nonmergeable"
+
+; CHECK: .section .explicit_initially_nonmergeable,"a",@progbits
+; CHECK: explicit_basic_10:
+; CHECK: .section .explicit_initially_nonmergeable,"aM",@progbits,4,unique,6
+; CHECK: explicit_basic_11:
+
+;; Assign compatible globals to the previously created sections.
+@explicit_basic_10 = constant [2 x i16] [i16 1, i16 1], section ".explicit_initially_nonmergeable"
+@explicit_basic_11 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_initially_nonmergeable"
+
+;; Check that mergeable symbols can be explicitly assigned to "default" sections.
+
+; CHECK: .section .rodata.cst16,"a",@progbits,unique,7
+; CHECK: explicit_default_1:
+
+;; Assign an incompatible (non-mergeable) symbol to a "default" mergeable section.
+@explicit_default_1 = constant [2 x i64] [i64 1, i64 1], section ".rodata.cst16"
+
+; CHECK: .section .rodata.cst16,"aM",@progbits,16
+; CHECK: explicit_default_2:
+
+;; Assign a compatible global to a "default" mergeable section.
+@explicit_default_2 = unnamed_addr constant [2 x i64] [i64 1, i64 1], section ".rodata.cst16"
+
+; CHECK: .section .debug_str,"MS",@progbits,1
+; CHECK: explicit_default_3:
+
+;; Non-allocatable "default" sections can have allocatable mergeable symbols with compatible entry sizes assigned to them.
+@explicit_default_3 = unnamed_addr constant [2 x i8] [i8 1, i8 0], section ".debug_str"
+
+; CHECK: .section .debug_str,"a",@progbits,unique,8
+; CHECK: explicit_default_4:
+
+;; Non-allocatable "default" sections cannot have allocatable mergeable symbols with incompatible (non-mergeable) entry sizes assigned to them.
+@explicit_default_4 = constant [2 x i16] [i16 1, i16 1], section ".debug_str"
+
+;; Test implicit section assignment for globals with associated globals.
+; CHECK: .section .rodata.cst4,"aMo",@progbits,4,implicit_rodata_cst4,unique,9
+; CHECK: implicit_rodata_cst4_assoc:
+; CHECK: .section .rodata.cst8,"aMo",@progbits,8,implicit_rodata_cst4,unique,10
+; CHECK: implicit_rodata_cst8_assoc:
+
+@implicit_rodata_cst4_assoc = unnamed_addr constant [2 x i16] [i16 1, i16 1], !associated !4
+@implicit_rodata_cst8_assoc = unnamed_addr constant [2 x i32] [i32 1, i32 1], !associated !4
+
+;; Check that globals with associated globals that are explicitly assigned
+;; to a section have been placed into distinct sections with the same name, but
+;; different entry sizes.
+; CHECK: .section .explicit,"aMo",@progbits,4,implicit_rodata_cst4,unique,11
+; CHECK: explicit_assoc_1:
+; CHECK: .section .explicit,"aMo",@progbits,4,implicit_rodata_cst4,unique,12
+; CHECK: explicit_assoc_2:
+; CHECK: .section .explicit,"aMo",@progbits,8,implicit_rodata_cst4,unique,13
+; CHECK: explicit_assoc_3:
+
+@explicit_assoc_1 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit", !associated !4
+@explicit_assoc_2 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit", !associated !4
+@explicit_assoc_3 = unnamed_addr constant [2 x i32] [i32 1, i32 1], section ".explicit", !associated !4
+
+!4 = !{[2 x i16]* @implicit_rodata_cst4}
+
+;; Test implicit section assignment for globals in distinct comdat groups.
+; CHECK: .section .rodata.cst4,"aGM",@progbits,4,f,comdat,unique,14
+; CHECK: implicit_rodata_cst4_comdat:
+; CHECK: .section .rodata.cst8,"aGM",@progbits,8,g,comdat,unique,15
+; CHECK: implicit_rodata_cst8_comdat:
+
+;; Check that globals in distinct comdat groups that are explicitly assigned
+;; to a section have been placed into distinct sections with the same name, but
+;; different entry sizes. Due to the way that MC currently works the unique ID
+;; does not have any effect here, although it appears in the assembly. The unique ID's
+;; appear incorrect as comdats are not taken into account when looking up the unique ID
+;; for a mergeable section. However, as they have no effect it doesn't matter that they
+;; are incorrect.
+; CHECK: .section .explicit_comdat_distinct,"aM",@progbits,4,unique,16
+; CHECK: explicit_comdat_distinct_supply_uid:
+; CHECK: .section .explicit_comdat_distinct,"aGM",@progbits,4,f,comdat,unique,16
+; CHECK: explicit_comdat_distinct1:
+; CHECK: .section .explicit_comdat_distinct,"aGM",@progbits,4,g,comdat,unique,16
+; CHECK: explicit_comdat_distinct2:
+; CHECK: .section .explicit_comdat_distinct,"aGM",@progbits,8,h,comdat,unique,17
+; CHECK: explicit_comdat_distinct3:
+
+$f = comdat any
+$g = comdat any
+$h = comdat any
+
+@implicit_rodata_cst4_comdat = unnamed_addr constant [2 x i16] [i16 1, i16 1], comdat($f)
+@implicit_rodata_cst8_comdat = unnamed_addr constant [2 x i32] [i32 1, i32 1], comdat($g)
+
+@explicit_comdat_distinct_supply_uid = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_comdat_distinct"
+@explicit_comdat_distinct1 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_comdat_distinct", comdat($f)
+@explicit_comdat_distinct2 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_comdat_distinct", comdat($g)
+@explicit_comdat_distinct3 = unnamed_addr constant [2 x i32] [i32 1, i32 1], section ".explicit_comdat_distinct", comdat($h)
+
+;; Test implicit section assignment for globals in the same comdat group.
+; CHECK: .section .rodata.cst4,"aGM",@progbits,4,i,comdat,unique,18
+; CHECK: implicit_rodata_cst4_same_comdat:
+; CHECK: .section .rodata.cst8,"aGM",@progbits,8,i,comdat,unique,19
+; CHECK: implicit_rodata_cst8_same_comdat:
+
+;; Check that globals in the same comdat group that are explicitly assigned
+;; to a section have been placed into distinct sections with the same name, but
+;; different entry sizes. Due to the way that MC currently works the unique ID
+;; does not have any effect here, although it appears in the assembly. The unique ID's
+;; appear incorrect as comdats are not taken into account when looking up the unique ID
+;; for a mergeable section. However, as they have no effect it doesn't matter that they
+;; are incorrect.
+; CHECK: .section .explicit_comdat_same,"aM",@progbits,4,unique,20
+; CHECK: explicit_comdat_same_supply_uid:
+; CHECK: .section .explicit_comdat_same,"aGM",@progbits,4,i,comdat,unique,20
+; CHECK: explicit_comdat_same1:
+; CHECK: explicit_comdat_same2:
+; CHECK: .section .explicit_comdat_same,"aGM",@progbits,8,i,comdat,unique,21
+; CHECK: explicit_comdat_same3:
+
+$i = comdat any
+
+@implicit_rodata_cst4_same_comdat = unnamed_addr constant [2 x i16] [i16 1, i16 1], comdat($i)
+@implicit_rodata_cst8_same_comdat = unnamed_addr constant [2 x i32] [i32 1, i32 1], comdat($i)
+
+@explicit_comdat_same_supply_uid = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_comdat_same"
+@explicit_comdat_same1 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_comdat_same", comdat($i)
+@explicit_comdat_same2 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit_comdat_same", comdat($i)
+@explicit_comdat_same3 = unnamed_addr constant [2 x i32] [i32 1, i32 1], section ".explicit_comdat_same", comdat($i)
+
+;; Check interaction between symbols that are explicitly assigned
+;; to a section and implicitly assigned symbols.
+
+; CHECK: .section .rodata.str1.1,"aMS",@progbits,1
+; CHECK: implicit_rodata_str1_1:
+; CHECK: explicit_implicit_1:
+
+;; Assign a compatible global to an existing mergeable section created implicitly.
+@implicit_rodata_str1_1 = unnamed_addr constant [2 x i8] [i8 1, i8 0]
+@explicit_implicit_1 = unnamed_addr constant [2 x i8] [i8 1, i8 0], section ".rodata.str1.1"
+
+; CHECK: .section .rodata.str1.1,"a",@progbits,unique,22
+; CHECK: explicit_implicit_2:
+
+;; Assign an incompatible symbol (non-mergeable) to an existing mergeable section created implicitly.
+@explicit_implicit_2 = constant [2 x i16] [i16 1, i16 1], section ".rodata.str1.1"
+
+; CHECK: .section .rodata.str1.1,"aMS",@progbits,1
+; CHECK: explicit_implicit_3:
+; CHECK: .section .rodata.str1.1,"a",@progbits,unique,22
+; CHECK: explicit_implicit_4:
+
+;; Assign compatible globals to the previously created sections.
+@explicit_implicit_3 = unnamed_addr constant [2 x i8] [i8 1, i8 0], section ".rodata.str1.1"
+@explicit_implicit_4 = constant [2 x i16] [i16 1, i16 1], section ".rodata.str1.1"
+
+; CHECK: .section .rodata.str2.2,"aMS",@progbits,2
+; CHECK: explicit_implicit_5:
+; CHECK: implicit_rodata_str2_2:
+
+;; Implicitly assign a compatible global to an existing mergeable section created explicitly.
+@explicit_implicit_5 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".rodata.str2.2"
+@implicit_rodata_str2_2 = unnamed_addr constant [2 x i16] [i16 1, i16 0]
+
+;; Check the interaction with inline asm.
+
+; CHECK: .section .asm_mergeable1,"aMS",@progbits,2
+; CHECK: explicit_asm_1:
+; CHECK: .section .asm_nonmergeable1,"a",@progbits
+; CHECK: explicit_asm_2:
+; CHECK: .section .asm_mergeable1,"aM",@progbits,4,unique,23
+; CHECK: explicit_asm_3:
+; CHECK: .section .asm_nonmergeable1,"aMS",@progbits,2,unique,24
+; CHECK: explicit_asm_4:
+; CHECK: .section .asm_mergeable2,"aM",@progbits,4,unique,25
+; CHECK: explicit_asm_5:
+; CHECK: .section .asm_nonmergeable2,"aMS",@progbits,2,unique,26
+; CHECK: explicit_asm_6:
+; CHECK: .section .asm_mergeable2,"aMS",@progbits,2
+; CHECK: explicit_asm_7:
+; CHECK: .section .asm_nonmergeable2,"a",@progbits
+; CHECK: explicit_asm_8:
+
+module asm ".section .asm_mergeable1,\22aMS\22,@progbits,2"
+module asm ".section .asm_nonmergeable1,\22a\22,@progbits"
+module asm ".section .asm_mergeable2,\22aMS\22,@progbits,2"
+module asm ".section .asm_nonmergeable2,\22a\22,@progbits"
+
+;; Assign compatible symbols to sections created using inline asm.
+@explicit_asm_1 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_mergeable1"
+@explicit_asm_2 = constant [2 x i16] [i16 1, i16 0], section ".asm_nonmergeable1"
+;; Assign incompatible globals to the same sections.
+@explicit_asm_3 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".asm_mergeable1"
+@explicit_asm_4 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_nonmergeable1"
+
+;; Assign incompatible globals to sections created using inline asm.
+@explicit_asm_5 = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".asm_mergeable2"
+@explicit_asm_6 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_nonmergeable2"
+;; Assign compatible globals to the same sections.
+@explicit_asm_7 = unnamed_addr constant [2 x i16] [i16 1, i16 0], section ".asm_mergeable2"
+@explicit_asm_8 = constant [2 x i16] [i16 1, i16 0], section ".asm_nonmergeable2"
+
+;; A .note.GNU-stack section is created implicitly. We add a check for this as we want to use
+;; --implicit-check-not to reduce the number of checks in this file.
+; CHECK: .section ".note.GNU-stack","",@progbits
+
+;; --no-integrated-as avoids the use of ",unique," for compatibility with older binutils.
+
+;; Error if an incompatible symbol is explicitly placed into a mergeable section.
+; RUN: not llc < %s -mtriple=x86_64 --no-integrated-as 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=NO-I-AS-ERR
+; NO-I-AS-ERR: error: Symbol 'explicit_default_1' from module '<stdin>' required a section with entry-size=0 but was placed in section '.rodata.cst16' with entry-size=16: Explicit assignment by pragma or attribute of an incompatible symbol to this section?
+; NO-I-AS-ERR: error: Symbol 'explicit_default_4' from module '<stdin>' required a section with entry-size=0 but was placed in section '.debug_str' with entry-size=1: Explicit assignment by pragma or attribute of an incompatible symbol to this section?
+; NO-I-AS-ERR: error: Symbol 'explicit_implicit_2' from module '<stdin>' required a section with entry-size=0 but was placed in section '.rodata.str1.1' with entry-size=1: Explicit assignment by pragma or attribute of an incompatible symbol to this section?
+; NO-I-AS-ERR: error: Symbol 'explicit_implicit_4' from module '<stdin>' required a section with entry-size=0 but was placed in section '.rodata.str1.1' with entry-size=1: Explicit assignment by pragma or attribute of an incompatible symbol to this section?
+
+;; Don't create mergeable sections for globals with an explicit section name.
+; RUN: echo '@explicit = unnamed_addr constant [2 x i16] [i16 1, i16 1], section ".explicit"' > %t.no_i_as.ll
+; RUN: llc < %t.no_i_as.ll -mtriple=x86_64 --no-integrated-as 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=NO-I-AS
+; NO-I-AS: .section .explicit,"a",@progbits

llvm/unittests/ExecutionEngine/Orc/LegacyRTDyldObjectLinkingLayerTest.cpp

LegacyRTDyldObjectLinkingLayer ObjLayer(
       AcknowledgeORCv1Deprecation, ES, [&MM](VModuleKey) {
         return LegacyRTDyldObjectLinkingLayer::Resources{
             MM, std::make_shared<NullResolver>()};
       });
 
   LLVMContext Context;
   auto M = std::make_unique<Module>("", Context);
   M->setTargetTriple("x86_64-unknown-linux-gnu");
-  Type *Int32Ty = IntegerType::get(Context, 32);
-  GlobalVariable *GV =
-    new GlobalVariable(*M, Int32Ty, false, GlobalValue::ExternalLinkage,
-                         ConstantInt::get(Int32Ty, 42), "foo");
+  Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
+  auto *GV =
+      new GlobalVariable(*M, StrConstant->getType(), true,
+                         GlobalValue::ExternalLinkage, StrConstant, "foo");
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(Align(1));
 
   GV->setSection(".debug_str");
 
 
   // Initialize the native target in case this is the first unit test
   // to try to build a TM.
   OrcNativeTarget::initialize();
   std::unique_ptr<TargetMachine> TM(

llvm/unittests/ExecutionEngine/Orc/RTDyldObjectLinkingLayerTest.cpp

static bool testSetProcessAllSections(std::unique_ptr<MemoryBuffer> Obj,
 
   return DebugSectionSeen;
 }
 
 TEST(RTDyldObjectLinkingLayerTest, TestSetProcessAllSections) {
   LLVMContext Context;
   auto M = std::make_unique<Module>("", Context);
   M->setTargetTriple("x86_64-unknown-linux-gnu");
-  Type *Int32Ty = IntegerType::get(Context, 32);
-  GlobalVariable *GV =
-      new GlobalVariable(*M, Int32Ty, false, GlobalValue::ExternalLinkage,
-                         ConstantInt::get(Int32Ty, 42), "foo");
+  Constant *StrConstant = ConstantDataArray::getString(Context, "forty-two");
+  auto *GV =
+      new GlobalVariable(*M, StrConstant->getType(), true,
+                         GlobalValue::ExternalLinkage, StrConstant, "foo");
+  GV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
+  GV->setAlignment(Align(1));
 
   GV->setSection(".debug_str");
 
   // Initialize the native target in case this is the first unit test
   // to try to build a TM.
   OrcNativeTarget::initialize();
   std::unique_ptr<TargetMachine> TM(EngineBuilder().selectTarget(
       Triple(M->getTargetTriple()), "", "", SmallVector<std::string, 1>()));