[PATCH] D24533: Fix auto-upgrade of TBAA tags in Bitcode Reader

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Sep-13, at 16:52, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
>> On 2016-Sep-13, at 16:37, Mehdi AMINI <mehdi.amini at apple.com> wrote:
>> 
>> mehdi_amini added a comment.
>> 
>> Sorry forgot the test case, will update soon!
>> 
>> 
>> https://reviews.llvm.org/D24533
> 
>> Index: lib/Bitcode/Reader/BitcodeReader.cpp
>> ===================================================================
>> --- lib/Bitcode/Reader/BitcodeReader.cpp
>> +++ lib/Bitcode/Reader/BitcodeReader.cpp
>> @@ -4404,11 +4403,11 @@
>>         if (HasSeenOldLoopTags && I->second == LLVMContext::MD_loop)
>>           MD = upgradeInstructionLoopAttachment(*MD);
>> 
>> -        Inst->setMetadata(I->second, MD);
>>         if (I->second == LLVMContext::MD_tbaa) {
>> -          InstsWithTBAATag.push_back(Inst);
>> -          continue;
>> +          assert(!isa<TempMDTuple>() && "should load metadata before attachment");
> 
> I don't think this compiles (perhaps you didn't try an asserts build?).
> Two reasons: isa<>() takes an argument, and TempMDTuple is defined
> something like this:
> --
> typedef std::unique_ptr<MDTuple, SomeDeleter> TempMDTuple;
> --
> I think you should call 'MD->isTemporary()' instead.

Also, I think this should `return error(...)` instead of asserting,
so that the corrupt bitcode doesn't cause a crash.

> 
>> +          MD = UpgradeTBAANode(Inst->getContext(), MD);
>>         }
>> +        Inst->setMetadata(I->second, MD);
>>       }
>>       break;
>>     }
>> Index: include/llvm/IR/AutoUpgrade.h
>> ===================================================================
>> --- include/llvm/IR/AutoUpgrade.h
>> +++ include/llvm/IR/AutoUpgrade.h
>> @@ -51,6 +52,11 @@
>>   /// module is modified.
>>   bool UpgradeModuleFlags(Module &M);
>> 
>> +  /// If the given TBAA tag uses the scalar TBAA format, create a new node
>> +  /// corresponding to the upgrade to the struct-path aware TBAA format.
>> +  /// Otherwise return the \p TBAANode itself.
>> +  MDNode *UpgradeTBAANode(LLVMContext &Context, MDNode *TBAANode);
>> +
>>   /// If the TBAA tag for the given instruction uses the scalar TBAA format,
>>   /// we upgrade it to the struct-path aware TBAA format.
>>   void UpgradeInstWithTBAATag(Instruction *I);
> 
> Do we need this still?  I know there's a caller in LLParser, but that
> could also be updated to use this API.  Even better, we could delete the
> upgrade logic from the LLParser entirely, given that we don't support
> reading old textual IR (that would be a separate commit though).
> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits