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

[Fangrui Song via Phabricator via llvm-commits]

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




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

https://reviews.llvm.org/D72194