Page MenuHomePhabricator

[DebugInfoMetadata] Move main subprogram DIFlag into DISPFlags
ClosedPublic

Authored by djtodoro on Mar 13 2019, 2:19 AM.

Details

Summary

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.

Diff Detail

Repository
rL LLVM

Event Timeline

djtodoro created this revision.Mar 13 2019, 2:19 AM
Herald added a project: Restricted Project. · View Herald Transcript
vsk added a subscriber: vsk.

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

probinson added inline comments.Mar 13 2019, 11:35 AM
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.

djtodoro marked an inline comment as done.Mar 14 2019, 12:37 AM

Thank you all for comments!

@aprantl

Yes. Can you please add upgrade functionality into MetadataLoader and add a bitcode upgrade test into test/Bitcode?

Sure.

include/llvm-c/DebugInfo.h
53 ↗(On Diff #190381)

It is better idea. We can do that way. Thanks!

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?

aprantl requested changes to this revision.Mar 14 2019, 9:34 AM

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.

This revision now requires changes to proceed.Mar 14 2019, 9:34 AM

Thanks for the comment! I understand.

djtodoro updated this revision to Diff 190834.Mar 15 2019, 8:59 AM
  • Add support in Metadataloader for older metadata
  • Add bitcode upgrade test

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);
}
ormris removed a subscriber: ormris.Mar 15 2019, 9:29 AM
djtodoro marked 2 inline comments as done.Mar 15 2019, 10:45 AM

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).
This will work only in the case of old DISubprogram, with no SPFlags at all.

djtodoro updated this revision to Diff 191062.Mar 18 2019, 2:53 AM

-Refactor upgrade metadata functionality

aprantl accepted this revision.Mar 18 2019, 9:38 AM
This revision is now accepted and ready to land.Mar 18 2019, 9:38 AM
This revision was automatically updated to reflect the committed changes.