Moving subprogram specific flags into DISPFlags makes IR code more readable.
In addition, we provide free space in DIFlags for other 'non-subprogram-specific' debug info flags.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
+ Steven, is debug info bitcode backwards compatibility important for the App Store? (I’m not 100% sure, but I think it is, in which case we’d need to 1) detect old DI metadata and 2) auto upgrade it?)
Yes. Can you please add upgrade functionality into MetadataLoader and add a bitcode upgrade test into test/Bitcode?
You can look at llvm/test/Bitcode/DISubprogram-v4.ll and llvm/test/Bitcode/DISubprogram-v4.ll.bc as an example.
include/llvm-c/DebugInfo.h | ||
---|---|---|
53 ↗ | (On Diff #190381) | Better to leave an unused bit in the middle? No need to reassign existing flags to new bits. The next time somebody adds a new flag, it can use the previously unused bit. This will simplify the auto-upgrade as well. |
Yes. Can you please add upgrade functionality into MetadataLoader and add a bitcode upgrade test into test/Bitcode?
You can look at llvm/test/Bitcode/DISubprogram-v4.ll and llvm/test/Bitcode/DISubprogram-v4.ll.bc as an example.
@aprantl In addition to this, I guess we want support for this in LLParser as well?
We don't typically require autoupgrade functionality in the IR parser, as there are no compatibility guarantees for textual LLVM IR. Implementing bitcode support is all that is required. Sometimes it can be convenient to support older formats in LLParser to avoid updating hundreds of tests, but it's definitely not required.
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.
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1421 ↗ | (On Diff #190834) | Better to use a local variable instead of a macro here. |
1423 ↗ | (On Diff #190834) | 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); } |
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;
lib/Bitcode/Reader/MetadataLoader.cpp | ||
---|---|---|
1421 ↗ | (On Diff #190834) | Yes, I agree. |
1423 ↗ | (On Diff #190834) | This won't work for the case you describe (when DISubprogram has SPFlags). |