This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by bd1976llvm on Jan 3 2020, 6:48 PM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bd1976llvm marked 4 inline comments as done.Jan 13 2020, 5:28 PM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
692

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

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

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.

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

rjmccall resigned from this revision.Jan 13 2020, 7:43 PM

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

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
736

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?

This is a partial fix for

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

bd1976llvm marked an inline comment as done.Jan 14 2020, 12:07 PM
bd1976llvm added inline comments.
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.

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.

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
736

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

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 added inline comments.Jan 14 2020, 6:08 PM
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.

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

MaskRay added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
684

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

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

Yes please.

bd1976llvm marked an inline comment as done.Jan 16 2020, 3:03 AM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
684

@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?

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.

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.

672

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

MaskRay added inline comments.Jan 18 2020, 2:35 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
684

,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 marked 5 inline comments as done.

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 marked 5 inline comments as done.Jan 23 2020, 6:48 PM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
684

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 this revision to Diff 242492.Feb 4 2020, 6:32 PM
bd1976llvm edited the summary of this revision. (Show Details)

updated the patch and description

bd1976llvm edited the summary of this revision. (Show Details)Feb 5 2020, 5:00 AM
bd1976llvm added reviewers: MaskRay, maskray0.
MaskRay added a comment.EditedFeb 5 2020, 10:55 PM

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
477

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.

482
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
7

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 edited the summary of this revision. (Show Details)Mar 10 2020, 9:07 AM
bd1976llvm marked 8 inline comments as done.Mar 10 2020, 11:34 AM

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
477

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.

bd1976llvm marked an inline comment as done.
MaskRay added a comment.EditedMar 11 2020, 8:30 PM

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
452

The variable IsGenericSection can be omitted.

MaskRay added inline comments.Mar 11 2020, 8:32 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
613

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

620

/*MayAlwaysUsePrivate=*/true

bd1976llvm marked 6 inline comments as done.
bd1976llvm edited the summary of this revision. (Show Details)
bd1976llvm marked 2 inline comments as done.Mar 16 2020, 9:09 AM

Addressed review comments and rebased onto master.

The summary is very long. Can you simplify it?

I have rewritten the summary.

MaskRay added inline comments.Mar 16 2020, 9:21 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
620

Not done yet.

MaskRay added inline comments.Mar 16 2020, 2:30 PM
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
773

Accidentally indented.

bd1976llvm edited the summary of this revision. (Show Details)Mar 17 2020, 8:23 AM
bd1976llvm edited the summary of this revision. (Show Details)Mar 17 2020, 8:29 AM
bd1976llvm edited the summary of this revision. (Show Details)
bd1976llvm edited the summary of this revision. (Show Details)
bd1976llvm marked 5 inline comments as done.Mar 19 2020, 12:45 PM
bd1976llvm added inline comments.
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
620

Sorry - lost the change rebasing somehow,

773

Thanks!

MaskRay added inline comments.Mar 19 2020, 2:58 PM
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 marked 2 inline comments as done.

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

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
589

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 marked 4 inline comments as done.

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
589

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.

bd1976llvm marked an inline comment as done.Mar 23 2020, 11:20 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
589

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

tpimh added a subscriber: tpimh.Mar 24 2020, 11:03 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
684

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.

Corrected silly mistake.

bd1976llvm marked 3 inline comments as done.Mar 25 2020, 5:58 PM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
589

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

684

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.

llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
684

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 marked an inline comment as done.

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

bd1976llvm marked 2 inline comments as done.Mar 26 2020, 7:12 PM

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.

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
754

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
292

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 added inline comments.Mar 27 2020, 4:18 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
720

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 added inline comments.Mar 27 2020, 5:09 PM
llvm/include/llvm/MC/MCContext.h
20

ELF.h should be after Dwarf.h

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

Nit: -linux-gnu can be deleted.

2

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

17

Delete trailing whitespace

47

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

63

explicit_basic_5 does not test string vs non-string.

65

Change to 1 x i64 to enhance the test.

71

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.

109

This test does not seem correct.

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

115

This non-unnamed_addr test does not add coverage.

MaskRay added inline comments.Mar 27 2020, 5:24 PM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
734

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

754

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
267

Changing to 1 x i32 can enhance the test.

274

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]

281

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

285

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

bd1976llvm marked 30 inline comments as done.Mar 30 2020, 6:41 PM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
734

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

754

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.

754

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
2

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

47

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

63

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.

65

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

71

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?

109

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.

115

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.

267

These need to be strings. See other comments.

274

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

281

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

285

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.

292

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 marked 12 inline comments as done.

Addressed review comments.
Cosmetic test improvements.

bd1976llvm marked an inline comment as done.

Addressed header file order review comment

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

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

bd1976llvm marked 2 inline comments as done.Mar 31 2020, 12:30 PM

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

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
291

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 accepted this revision.Mar 31 2020, 1:12 PM

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.

This revision is now accepted and ready to land.Mar 31 2020, 1:12 PM
efriedma added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
754

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

bd1976llvm marked an inline comment as done.Apr 1 2020, 6:13 PM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
754

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 added inline comments.Apr 2 2020, 8:12 AM
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
754

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 marked an inline comment as done.Apr 2 2020, 6:06 PM
bd1976llvm added inline comments.
llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp
754

@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 updated this revision to Diff 255520.Apr 6 2020, 3:42 PM
bd1976llvm edited the summary of this revision. (Show Details)

Report error using LLVM diagnostics framework.

nickdesaulniers accepted this revision.Apr 6 2020, 3:59 PM

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

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

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

bd1976llvm updated this revision to Diff 255848.Apr 7 2020, 4:08 PM
bd1976llvm marked an inline comment as done.

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

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
754

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

MaskRay added a comment.EditedApr 7 2020, 6:12 PM

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 updated this revision to Diff 256011.Apr 8 2020, 7:01 AM
  • 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 marked 2 inline comments as done.Apr 8 2020, 7:03 AM

Thanks for the interdiff rationale. Please take another look!

MaskRay accepted this revision.Apr 8 2020, 9:27 AM

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

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

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?

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

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.

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.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2020, 12:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript