[PATCH] D27839: Strip invalid TBAA when reading bitcode

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:51
 #include "llvm/IR/InlineAsm.h"
+#include "llvm/IR/InstIterator.h"
 #include "llvm/IR/InstrTypes.h"
----------------
sanjoy wrote:
> 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())
+      continue;
+    for (auto &I : instructions(F))
----------------
sanjoy wrote:
> 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;
----------------
sanjoy wrote:
> 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.
Make sense?


================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1347
+          if (StripTBAA)
+            break;
           assert(!MD->isTemporary() && "should load MDs before attachments");
----------------
sanjoy wrote:
> 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)
>   continue;
> ```
> 
> right after the `if (!MD)` check.
It is breaking out of the switch. I'll move the check above.


https://reviews.llvm.org/D27839