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

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

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?

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