[PATCH] D59288: [DebugInfoMetadata] Move main subprogram DIFlag into DISPFlags

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 2 inline comments as done.
djtodoro added a comment.

> IIUC this upgrade is sweeping the variant of DISubprograms that have SPflags and do not yet have this patch under the rug? The way the Swift compiler branches work out I can live with that but other downstream users may have a problem with that as it could break LTO builds that include object files versions of clang between 347239 and this patch.

I think this should support both DISubprogram versions, with/out `SPFlags`.
That's  why before this upgrade functionality I put reading of  `SPFlags` (if any) and just update it, in the case of  `HasOldMainSubprogramFlag`, with
`if (HasSPFlags) SPFlags |= DISubprogram::SPFlagMainSubprogram;`



================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1421
+    // subprogram specific flags are placed in DIFlags.
+#define DIFlagMainSubprogram (1 << 21)
+    bool HasOldMainSubprogramFlag = Flags & DIFlagMainSubprogram;
----------------
aprantl wrote:
> Better to use a local variable instead of a macro here.
Yes, I agree.


================
Comment at: lib/Bitcode/Reader/MetadataLoader.cpp:1423
+    bool HasOldMainSubprogramFlag = Flags & DIFlagMainSubprogram;
+    if (HasOldMainSubprogramFlag) {
+      // Remove old DIFlagMainSubprogram from DIFlags.
----------------
aprantl wrote:
> I think it would be more clear to write this as:
> ```
> if (!HasSPFlags) {
>   const unsigned DIFlagMainSubprogram = 1 << 21;
>   bool HasOldMainSubprogramFlag = Flags & DIFlagMainSubprogram;
>   // Remove old DIFlagMainSubprogram from DIFlags.
>   // Note: This assumes that any future use of bit 21 defaults to it being 0.
>   Flags &= ~static_cast<DINode::DIFlags>(DIFlagMainSubprogram);
> 
>   DISubprogram::toSPFlags(
>                     /*IsLocalToUnit=*/Record[7], /*IsDefinition=*/Record[8],
>                     /*IsOptimized=*/Record[14], /*Virtuality=*/Record[11],
>                     /*DIFlagMainSubprogram*/HasOldMainSubprogramFlag);
> }
> ```
This won't work for the case you describe (when `DISubprogram` has `SPFlags`).
This will work only in the case of old `DISubprogram`, with no `SPFlags` at all.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59288/new/

https://reviews.llvm.org/D59288