This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Parse llvm.ident into producers section
ClosedPublic

Authored by tlively on Jan 15 2019, 1:43 PM.

Details

Summary

Everything before the word "version" is the tool, and everything after
the word "version" is the version.

Diff Detail

Repository
rL LLVM

Event Timeline

tlively created this revision.Jan 15 2019, 1:43 PM

This patch seems to break llvm-dwarfdump, used by emscripten with -g, so I will need to make further changes and probably add new tests.

This patch seems to break llvm-dwarfdump, used by emscripten with -g, so I will need to make further changes and probably add new tests.

Actually, it turns out that this is a linker bug. The linker does not know about the producers section yet, so the section ends up being placed before the name section, which makes the resulting binary invalid as far as LLVM is concerned. The fix is to implement proper producers section support in lld. Coming soon.

aheejin added a comment.EditedJan 15 2019, 2:16 PM
  • Where do you actually put the contents into the binary producer section? YAML result does not show anything.
  • Does object file reader support come later as a separate patch?
aheejin added a comment.EditedJan 15 2019, 2:16 PM

This patch seems to break llvm-dwarfdump, used by emscripten with -g, so I will need to make further changes and probably add new tests.

Actually, it turns out that this is a linker bug. The linker does not know about the producers section yet, so the section ends up being placed before the name section, which makes the resulting binary invalid as far as LLVM is concerned. The fix is to implement proper producers section support in lld. Coming soon.

Why would linker have to know about custom section to make this patch work? We didn't run any linker tests in this patch, did we?

sbc100 accepted this revision.Jan 15 2019, 2:19 PM

Nice!

We could teach ObjectYAML to understand the contents but I don't think that is necessary for this change.

lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
154 ↗(On Diff #181878)

What if version is not part of the ident? Is that legal?

This revision is now accepted and ready to land.Jan 15 2019, 2:19 PM

Where do you actually put the contents into the binary producer section? YAML result does not show anything.

See WebAssemblyAsmPrinter.cpp

This patch seems to break llvm-dwarfdump, used by emscripten with -g, so I will need to make further changes and probably add new tests.

Actually, it turns out that this is a linker bug. The linker does not know about the producers section yet, so the section ends up being placed before the name section, which makes the resulting binary invalid as far as LLVM is concerned. The fix is to implement proper producers section support in lld. Coming soon.

Why would linker have to know to make this patch work? We didn't run any linker tests in this patch, did we?

It's more like the linker is broken in the presence of producers sections, but we didn't know until now because we never had producers sections.

tlively marked an inline comment as done.Jan 15 2019, 2:21 PM
tlively added inline comments.
lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp
154 ↗(On Diff #181878)

In that case the entire llvm.ident string will be taken as the tool and the version string will be empty.

As a follow-on, I guess we could get ambitious and add the "language" field as well. I think the only way to do that would be to sniff the language field of the DICompileUnit metadata node (it would of course only exist if there's debug info, but doing anything else seems a bit crazytown).

tlively updated this revision to Diff 182145.Jan 16 2019, 2:01 PM
  • Add ObjectYAML support and tests
  • Restrict producers to be unique by name
dschuff added inline comments.Jan 16 2019, 2:21 PM
lib/ObjectYAML/WasmYAML.cpp
79 ↗(On Diff #182145)

Shouldn't these all be optional?

sbc100 accepted this revision.Jan 16 2019, 2:28 PM
sbc100 added inline comments.
include/llvm/Object/Wasm.h
160 ↗(On Diff #182145)

Could we keep this interface simpler by putting these in a struct and have "getProducerInfo()" (see DylinkInfo).

Otherwise lgtm

tlively updated this revision to Diff 182165.Jan 16 2019, 3:18 PM
  • Make YAML fields optional, change to getProducerInfo interface
tlively marked 2 inline comments as done.Jan 16 2019, 3:20 PM
This revision was automatically updated to reflect the committed changes.