This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Implement build-id feature
ClosedPublic

Authored by dschuff on Aug 6 2021, 12:16 PM.

Details

Summary

Implement the --build-id flag similarly to ELF, and generate a build_id section according to the WebAssembly tool convention specified in https://github.com/WebAssembly/tool-conventions/pull/183

The default style ("fast" aka "tree") hashes the contents of the output and (unlike ELF) generates a v5 UUID based on the hash (using a random namespace).
It also supports generating a random v4 UUID, a sha1 hash, and a user-specified string (as ELF does).

Diff Detail

Event Timeline

dschuff created this revision.Aug 6 2021, 12:16 PM
dschuff updated this revision to Diff 364867.Aug 6 2021, 12:19 PM

fix, and rebase

dschuff updated this revision to Diff 433551.Jun 1 2022, 2:48 PM
  • Clean up and add test
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 2:48 PM
Herald added subscribers: pmatos, asb. · View Herald Transcript
dschuff updated this revision to Diff 433561.Jun 1 2022, 2:57 PM
  • remove stray line
dschuff published this revision for review.Jun 1 2022, 4:03 PM
dschuff retitled this revision from [WIP][WebAssembly] Implement build-id feature to [WebAssembly] Implement build-id feature.
dschuff edited the summary of this revision. (Show Details)
dschuff added a subscriber: aheejin.
Herald added a project: Restricted Project. · View Herald TranscriptJun 1 2022, 4:03 PM
abrown added a subscriber: abrown.Jun 1 2022, 6:03 PM
sbc100 added a comment.Jun 2 2022, 1:25 PM

Great to see this happening!

lld/test/wasm/build-id.s
1 ↗(On Diff #433561)

lld/test/wasm/lit.local.cfg already takes care of this IIUC

43 ↗(On Diff #433561)

Why not just use lld/test/wasm/Inputs/start.s for this test .. or does it depend on there being some data?

When you could remove all the --no-entry flags above, and all the comment markers, and just call this file .test.

lld/wasm/Driver.cpp
353 ↗(On Diff #433561)

Do we want to support the tree alias? I assume this comes from the ELF backend?

lld/wasm/SyntheticSections.cpp
923 ↗(On Diff #433561)

Do you want to leave these TODOs in here?

dschuff updated this revision to Diff 434141.Jun 3 2022, 2:10 PM
  • use start.s as a test instead
lld/test/wasm/build-id.s
43 ↗(On Diff #433561)

no, it doesn't depend on there being data, I basically just started with the x86 version of this same test. But it has some additional assertions that don't apply to us.
So you're saying this would be a .test file because it would only have RUN lines, and the runs would just assemble start.s? That sounds good.

lld/wasm/Driver.cpp
353 ↗(On Diff #433561)

yes, this comes from the ELF backend. I think it probably makes sense to support the same options? I would think we'd at least want random UUID, some kind of hash, and user-defined. See also the discussion on the tool-conventions issue though, I'm open to making the implementation different if that makes sense.

lld/wasm/SyntheticSections.cpp
923 ↗(On Diff #433561)

no, the idea is to remove them once the discussion on tool-conventions is resolved and before committing this.

dschuff updated this revision to Diff 499322.Feb 21 2023, 4:41 PM

Rebase, create valid UUID for fast hash build ID, remove md5

dschuff updated this revision to Diff 499323.Feb 21 2023, 4:42 PM

Correctly rebase against main

dschuff edited the summary of this revision. (Show Details)Feb 21 2023, 4:48 PM
dschuff added a reviewer: aheejin.

I think this is ready for another review, PTAL.

I'd like to maybe also update objdump and/or other tools to print the build ID in UUID form when applicable, but that can be a followup change.

sbc100 accepted this revision.Mar 1 2023, 1:39 PM

lgtm % some minor nits

lld/wasm/SyntheticSections.h
444 ↗(On Diff #499323)

This seems a little hacky / fragile, but I can't think of a more elegant way to do it..

lld/wasm/Writer.cpp
1834 ↗(On Diff #499323)

I wonder if this should have a name that lets the reader know its going back an patching an existing location? Maybe patchBuildId ? But that doesn't convey that this function also calulates the buildid? "calculateAndPatchBuildId" seems like too much of a mouthful.

Maybe your existing names is best :)

This revision is now accepted and ready to land.Mar 1 2023, 1:39 PM
dschuff updated this revision to Diff 501697.Mar 1 2023, 4:36 PM
  • clean up baked-in buffer size
lld/wasm/SyntheticSections.h
444 ↗(On Diff #499323)

Ah yes I meant to try to fix that; WDYT of this fix?

lld/wasm/Writer.cpp
1834 ↗(On Diff #499323)

Yeah, those names are maybe a bit more descriptive but not obviously much better. This is the same name used by the other object formats (and has similar behavior) so consistency seems good in the end.

sbc100 accepted this revision.Mar 1 2023, 5:05 PM
sbc100 added inline comments.
lld/wasm/SyntheticSections.h
444 ↗(On Diff #499323)

Seems better, but maybe add a comment here. What is hashBuf for? Could a better name be used here? isHashLocation? hashPlaceholderPtr?

lld/wasm/Writer.cpp
1834 ↗(On Diff #499323)

Makes sense

dschuff updated this revision to Diff 501710.Mar 1 2023, 5:47 PM
  • add comment and rename hashBuf
dschuff marked an inline comment as done.Mar 1 2023, 5:47 PM
This revision was landed with ongoing or failed builds.Mar 2 2023, 2:30 PM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Mar 2 2023, 3:14 PM

@dschuff, I'm not sure if you are aware, but your change fails to build on Windows. You can see the failure on this buildbot:
https://lab.llvm.org/buildbot/#/builders/216/builds/17875

FAILED: tools/lld/wasm/CMakeFiles/lldWasm.dir/Writer.cpp.obj 
C:\bin\ccache.exe C:\PROGRA~2\MICROS~1\2019\BUILDT~1\VC\Tools\MSVC\1429~1.301\bin\HostX64\x64\cl.exe  /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_ASSERTIONS -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\lld\wasm -IC:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm -IC:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\include -Itools\lld\include -Iinclude -IC:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2   -wd4530 -wd4062  /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Fotools\lld\wasm\CMakeFiles\lldWasm.dir\Writer.cpp.obj /Fdtools\lld\wasm\CMakeFiles\lldWasm.dir\lldWasm.pdb /FS -c C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp
C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp(263): error C2672: 'std::copy': no matching overloaded function found
C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp(263): error C2780: '_FwdIt2 std::copy(_ExPo &&,_FwdIt1,_FwdIt1,_FwdIt2) noexcept': expects 4 arguments - 3 provided
C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\xutility(4463): note: see declaration of 'std::copy'
C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp(263): error C2782: '_OutIt std::copy(_InIt,_InIt,_OutIt)': template parameter '_InIt' is ambiguous
C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\xutility(4452): note: see declaration of 'std::copy'
C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp(263): note: could be '_Ty *'
        with
        [
            _Ty=uint8_t
        ]
C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp(263): note: or       'std::_Array_iterator<_Ty,20>'
        with
        [
            _Ty=uint8_t
        ]
C:\buildbot-root\llvm-clang-x86_64-sie-win\llvm-project\lld\wasm\Writer.cpp(263): error C2784: '_OutIt std::copy(_InIt,_InIt,_OutIt)': could not deduce template argument for '_InIt' from '_Ty *'
        with
        [
            _Ty=uint8_t
        ]
C:\Program Files (x86)\Microsoft Visual Studio\2019\BuildTools\VC\Tools\MSVC\14.29.30133\include\xutility(4452): note: see declaration of 'std::copy'

The failure was masked by an earlier failure which meant the buildbot never got to your code and so the failure didn't show up until the original problem was fixed.

Note this also seems to be the same failure the pre-merge checks hit. Can you take a look and revert if you need time to investigate?

Sorry about that, will fix.
And I guess I'll have to get into the habit of the pre-merge checks being useful!