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

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

> On 2016-May-06, at 15:37, Adrian Prantl <aprantl at apple.com> wrote:
> 
>> 
>> 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.
> 

Weird, but that sounds right.  Definitely easier to read if you change it :).

> -- 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>