[PATCH] D20004: Implement a safer bitcode upgrade for DISubprogram

[Adrian Prantl via llvm-commits]

> On May 6, 2016, at 1:23 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> Sorry for the delay answering your questions above, but it looks like we reached consensus anyway :).
> 
> LGTM with one fixup.
> 
>> On 2016-May-06, at 13:16, Adrian Prantl <aprantl at apple.com> wrote:
>> 
>> aprantl updated this revision to Diff 56450.
>> aprantl added a comment.
>> 
>> I get it. Independent flags don't really make sense in this context, and with the >2 check we can later add further bits *to the version number*.
>> 
>> 
>> http://reviews.llvm.org/D20004
>> 
>> Files:
>> lib/Bitcode/Reader/BitcodeReader.cpp
>> lib/Bitcode/Writer/BitcodeWriter.cpp
>> 
>> Index: lib/Bitcode/Writer/BitcodeWriter.cpp
>> ===================================================================
>> --- lib/Bitcode/Writer/BitcodeWriter.cpp
>> +++ lib/Bitcode/Writer/BitcodeWriter.cpp
>> @@ -1353,7 +1353,8 @@
>> void ModuleBitcodeWriter::writeDISubprogram(const DISubprogram *N,
>>                                            SmallVectorImpl<uint64_t> &Record,
>>                                            unsigned Abbrev) {
>> -  Record.push_back(N->isDistinct());
>> +  uint64_t HasUnitFlag = 1 << 1;
>> +  Record.push_back(N->isDistinct() | HasUnitFlag);
>>  Record.push_back(VE.getMetadataOrNullID(N->getScope()));
>>  Record.push_back(VE.getMetadataOrNullID(N->getRawName()));
>>  Record.push_back(VE.getMetadataOrNullID(N->getRawLinkageName()));
>> Index: lib/Bitcode/Reader/BitcodeReader.cpp
>> ===================================================================
>> --- lib/Bitcode/Reader/BitcodeReader.cpp
>> +++ lib/Bitcode/Reader/BitcodeReader.cpp
>> @@ -2472,28 +2472,30 @@
>>        return error("Invalid record");
>> 
>>      IsDistinct =
>> -          Record[0] || Record[8]; // All definitions should be distinct.
>> +          (Record[0] & 1) || Record[8]; // All definitions should be distinct.
>>      // Version 1 has a Function as Record[15].
>>      // Version 2 has removed Record[15].
>>      // Version 3 has the Unit as Record[15].
>> +      bool HasUnit = Record[0] > 2;
> 
> Should this be Record[0] >= 2?

I’ll change it for better readability, but it doesn’t really matter. A value of 2 means that the node is not distinct, which implies that it cannot have a unit field. Such a DISubprogram would not survive the Verifier.

-- adrian
> 
>> +      if (HasUnit && Record.size() != 19)
>> +        return error("Invalid record");
>>      Metadata *CUorFn = getMDOrNull(Record[15]);
>>      unsigned Offset = Record.size() == 19 ? 1 : 0;
>> -      bool HasFn = Offset && dyn_cast_or_null<ConstantAsMetadata>(CUorFn);
>> -      bool HasCU = Offset && !HasFn;
>> +      bool HasFn = Offset && !HasUnit;
>>      DISubprogram *SP = GET_OR_DISTINCT(
>>          DISubprogram,
>>          (Context, getDITypeRefOrNull(Record[1]), getMDString(Record[2]),
>>           getMDString(Record[3]), getMDOrNull(Record[4]), Record[5],
>>           getMDOrNull(Record[6]), Record[7], Record[8], Record[9],
>>           getDITypeRefOrNull(Record[10]), Record[11], Record[12], Record[13],
>> -           Record[14], HasCU ? CUorFn : nullptr,
>> +           Record[14], HasUnit ? CUorFn : nullptr,
>>           getMDOrNull(Record[15 + Offset]), getMDOrNull(Record[16 + Offset]),
>>           getMDOrNull(Record[17 + Offset])));
>>      MetadataList.assignValue(SP, NextMetadataNo++);
>> 
>>      // Upgrade sp->function mapping to function->sp mapping.
>>      if (HasFn) {
>> -        if (auto *CMD = dyn_cast<ConstantAsMetadata>(CUorFn))
>> +        if (auto *CMD = dyn_cast_or_null<ConstantAsMetadata>(CUorFn))
>>          if (auto *F = dyn_cast<Function>(CMD->getValue())) {
>>            if (F->isMaterializable())
>>              // Defer until materialized; unmaterialized functions may not have
>> 
>> 
>> <D20004.56450.patch>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160506/76f80766/attachment.html>