[PATCH] D20004: Implement a safer bitcode upgrade for DISubprogram
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Mon May 9 08:54:44 PDT 2016
> 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>
More information about the llvm-commits
mailing list