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

[ben via Phabricator via llvm-commits]

bd1976llvm marked 8 inline comments as done.
bd1976llvm added a comment.

In D72194#1861171 <https://reviews.llvm.org/D72194#1861171>, @MaskRay wrote:

> 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 <https://sourceware.org/ml/binutils/2020-02/msg00020.html> (I'm glad that my __patchable_function_entries complaints <https://gcc.gnu.org/ml/gcc/2020-01/msg00067.html> 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?



================
Comment at: llvm/lib/MC/MCContext.cpp:477
+std::pair<bool, unsigned>
+MCContext::getELFUniqueIDForEntsize(StringRef SectionName, unsigned Flags,
+                                    unsigned EntrySize) {
----------------
MaskRay wrote:
> 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.
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.


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

https://reviews.llvm.org/D72194