Page MenuHomePhabricator

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

tpimh added a subscriber: tpimh.Mar 24 2020, 11:03 PM
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.

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

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 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
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 added inline comments.Mar 27 2020, 4:18 PM
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 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
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 added inline comments.Mar 27 2020, 5:24 PM
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 marked 30 inline comments as done.Mar 30 2020, 6:41 PM
bd1976llvm added inline comments.
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 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
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 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
755

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
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 added inline comments.Apr 2 2020, 8:12 AM
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 marked an inline comment as done.Apr 2 2020, 6:06 PM
bd1976llvm added inline comments.
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 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
755

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