This is an archive of the discontinued LLVM Phabricator instance.

[Bitcode] Mention that a multi-module bitcode is valid, each module has exactly one MODULE_BLOCK block
ClosedPublic

Authored by mingmingl on Jan 11 2022, 4:57 PM.

Diff Detail

Unit TestsFailed

Event Timeline

mingmingl created this revision.Jan 11 2022, 4:57 PM
mingmingl published this revision for review.Jan 11 2022, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 11 2022, 4:58 PM
mingmingl updated this revision to Diff 399709.Jan 13 2022, 9:58 AM

Update bitcode format to mention multi-module bitcode.

mingmingl retitled this revision from Call takeError if MaybeVBR doesn't have a value. to [Bitcode] Call takeError if MaybeVBR doesn't have a value, and update bitcode doc on module block.Jan 13 2022, 10:00 AM
mingmingl added a reviewer: tejohnson.

lgtm on the documentation change but that should be submitted separately as it is unrelated to the code fix.
The other change lgtm but if it is feasible to add a test that would be ideal.

llvm/docs/BitCodeFormat.rst
560

Can you fix the line length? The file seems to stick with 80 col.

llvm/lib/Bitstream/Reader/BitstreamReader.cpp
100

Is it feasible to add a test? What was the behavior without the fix?

mingmingl updated this revision to Diff 401057.Jan 18 2022, 5:27 PM
mingmingl retitled this revision from [Bitcode] Call takeError if MaybeVBR doesn't have a value, and update bitcode doc on module block to [Bitcode] Mention that a multi-module bitcode is valid, each module has exactly one MODULE_BLOCK block.
mingmingl removed a reviewer: jfb.
mingmingl added a subscriber: jfb.

Keep the documentation change and put bitcode reader change to https://reviews.llvm.org/D117630 (which is still a draft).

lgtm on the documentation change but that should be submitted separately as it is unrelated to the code fix.
The other change lgtm but if it is feasible to add a test that would be ideal.

thanks! I revise this patch to be doc-only, and reformat, and going to pursue bitcode reader change in https://reviews.llvm.org/D117630

The patch is still a draft and I'll see how to add a test (probably an invalid bitcode is needed to trigger the condition);

What's the behavior without the fix

I think upon hitting the branch, trying to call get will run into assertions (with the asserted condition being false); yet it happens very rarely (if at all) since most of time, bitcode file should be valid.

I'll see if there is a lightweight way to construct the bitcode to have a test (maybe by hacking bitcode writer to generate an invalid record).

This revision is now accepted and ready to land.Jan 18 2022, 5:39 PM
This revision was landed with ongoing or failed builds.Jan 18 2022, 5:50 PM
This revision was automatically updated to reflect the committed changes.