[PATCH] D102973: [ELF] Suppress GRP_COMDAT deduplication if the signature symbol is STB_LOCAL

[James Henderson via Phabricator via llvm-commits]

jhenderson added a subscriber: thopre.
jhenderson added a comment.

Whilst I see where you're coming from, I think you've missed out an important part fo the spec's wording:

> The name of a symbol from one of the containing object's symbol tables provides a signature for the section group.

The key thing here is that it's the name that forms the signature, not the symbol, which I think means that your comment "The wording "having the same group signature" is not clear" is not correct. It seems pretty clear to me that the name is the thing that's important and nothing else, read as written, so strict reading of the spec says two same-named STB_LOCAL symbols acting as group signatures should be considered the same for deduplication purposes.

We also need to be confident that something isn't going to break whilst making this change. I think it's possible to craft something where this would potentially be the case, although I acknowledge it's a little contrived, so the behaviour change may be acceptable:

  # test.s
  .section .a,"axG", at progbits,a,comdat
  a:
  .local a
  .hidden a
      ret
  
  # test2.s
  .section .a,"ax", at progbits
  a:
  .weak a
  .protected a
      nop
      ret

In this example, the current behaviour when linking as a shared object always produce a protected dynamic symbol for `a`, but it's marked as undefined if test.o is linked first, whereas it's defined if test2.o was first in link order. If you delete the comdat aspects of the example, `a` will always be defined in the dynamic symbol table. This change in behaviour could theoretically impact dynamic symbol resolution, if I'm not mistaken, which could be a problem.

That all being said, I do see your point, and there is a good chance STB_LOCAL symbols were not considered when this part of the spec was written. I'd therefore not oppose a discussion on the gABI list suggesting the change. I don't think acting without wider discussion is a good idea though (but could be persuaded otherwise).



================
Comment at: lld/test/ELF/comdat-local.s:14
+# CHECK:      Hex dump of section '.zero':
+# CHECK-NEXT: [[#%x,_:]] 0202
+# CHECK:      Hex dump of section '.comdat':
----------------
Aside: @thopre, it would be nice if FileCheck would allow this pattern without the need to name the variable something ("_" in this case). The ideal format would be `[[#%x]]` indicating match any hex format value, much like `[[#]]` matches any default-format (i.e. decimal) number.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102973