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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/CodeGen/TargetLoweringObjectFileImpl.cpp:600
+    // sizes. Put the global into the correct existing section or create a
+    // new uniqued section.
+    if (Flags & ELF::SHF_MERGE) {
----------------
bd1976llvm wrote:
> nickdesaulniers wrote:
> > bd1976llvm wrote:
> > > MaskRay wrote:
> > > > bd1976llvm wrote:
> > > > > MaskRay wrote:
> > > > > > 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
> > > > > > 
> > > > > @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?
> > > > `,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 :)
> > > > 
> > > > 
> > > > 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?
> > 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.
> 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.
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72194/new/

https://reviews.llvm.org/D72194