[PATCH] D27839: Strip invalid TBAA when reading bitcode
Mehdi AMINI via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Dec 16 10:12:13 PST 2016
mehdi_amini added inline comments.
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:51
> I'd have mildly preferred doing the NFC header sorting in a separate change.
That was not intentional, clang-format did it for me.
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:157
+ if (F.isMaterializable())
+ for (auto &I : instructions(F))
> Why are you skipping these? Would be nice to add a one-liner, unless it is patently obvious.
A function that is Materializable hasn't been materialized, so it does not have a body. It is not a declaration either as its body may be parsed later. Trying to iterate it results in an assertion.
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:476
bool StripDebugInfo = false;
+ bool StripTBAA = false;
+ TBAAVerifier TBAAVerifyHelper;
> I'd have named this as `TBAAStripped` since it really indicates if we've already stripped out TBAA metadata (and hence there is no need to check this in for newer functions). Right now this seems imperative, and it sounds like setting this to `true` will cause TBAA to be stripped.
Actually it will also cause TBAA to be stripped :)
The first time you encounter an invalid TBAA, you set this boolean to request that from now on when parsing the body of a function, it has to ignore TBAA MD.
It is also used line 4469 to see if we're in a mode where we strip TBAA during reading, so we don't need to check every instruction in the function.
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1347
+ if (StripTBAA)
assert(!MD->isTemporary() && "should load MDs before attachments");
> Should this not `continue` (otherwise you'll drop all metadata once you've seen `!tbaa`)?
> Actually, I'd rather write this out separately as:
> if (I->second == LLVMContext::MD_tbaa && StripTBAA)
> right after the `if (!MD)` check.
It is breaking out of the switch. I'll move the check above.
More information about the llvm-commits