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


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

https://reviews.llvm.org/D72194