[PATCH] D92952: [WebAssembly] Support COMDAT sections in assembly syntax

[Derek Schuff via Phabricator via llvm-commits]

dschuff added a comment.

oops, forgot to publish my comments.



================
Comment at: llvm/lib/MC/WasmObjectWriter.cpp:1370
 
+    LLVM_DEBUG(dbgs() << "Processing Section " << SectionName << "  group "
+                      << Section.getGroup() << "\n";);
----------------
sbc100 wrote:
> Did you mean to leave this in?
Yeah I figured it was useful to go along with the other debug prints behind LLVM_DEBUG in this file.


================
Comment at: llvm/test/MC/WebAssembly/comdat-sections.s:5
+        .globl foo
+        .type foo, at function
+foo:
----------------
sbc100 wrote:
> I normally drop the `.type foo, at function` this its not needed.
Ah, you've actually hit on something interesting here. Currently the logic that sets the comdat flag on the function symbol is in `WasmAsmParser::parseDirectiveType()`, so it doesn't run if we leave out `.type`. That function also sets the symbol type. I don't see any logic anywhere else in WasmAsmParser or WebAssemblyAsmParser that sets the symbol type, but I guess it just defaults to function?

I tried it without the .type, and found that the tests all still worked! I found that that only the group flag on the section matters for creating the linking section. But other code still checks the comdat flag on the symbol, e.g. for creating imports. I updated the code for WebAssemblyAsmParser in this CL and I think it should work now, (i.e. it keeps the section group and symbol comdat proerties in sync). But I'm not sure how to test it. How would you specify an imported comdat function in assembly? Currently the only way to specify comdats is by using `.section` but that doesn't make sense for an import.

For now I put the .type there for foo, but left it off .bar, to exercise both codepaths. We can figure out the imported-comdat problem in a future CL. 


================
Comment at: llvm/test/MC/WebAssembly/comdat.ll:5
+; These RUN lines verify the ll direct-to-object path, the ll->asm path, and the
+; object output via asm.
 
----------------
sbc100 wrote:
> Do we still need `comdat-asm.ll` given that this test runs in both modes?
oh oops yes, I forgot I created comdat-asm.ll and then merged it in to here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92952