Page MenuHomePhabricator

[DebugInfo] Make offsets of dwarf units 64-bit (19/19).
ClosedPublic

Authored by ikudrin on Wed, Sep 2, 6:23 AM.

Details

Summary

In the case of LTO, several DWARF units can be emitted in one section. For an extremely large application, they may exceed the limit of 4GiB for 32-bit offsets. As it is now possible to emit 64-bit debugging info, the patch enables storing the larger offsets.

Diff Detail

Event Timeline

ikudrin created this revision.Wed, Sep 2, 6:23 AM
ikudrin requested review of this revision.Wed, Sep 2, 6:23 AM
dblaikie accepted this revision.Wed, Sep 2, 1:53 PM

Looks OK - open to discussion on the assert/report_fatal_error issue.

llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
594–601

These might be report_fatal_error-worthy? (if there's ways to fail the build more nicely (without fatally stopping the program) that'd be nice - not sure how other attempts to try to use the AsmPrinter when the use is invalid fail/whether they can fail gracefully)

This revision is now accepted and ready to land.Wed, Sep 2, 1:53 PM
jhenderson added inline comments.Thu, Sep 3, 1:03 AM
llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
594–601

My personal thoughts:

  1. If this code can actually be hit, it should be converted to return Error/Expected up to whatever point does the error handling.
  2. report_fatal_error could be a temporary fallback if the above point is too big a piece of work for now, but shouldn't be the final state of the code (since report_fatal_error results essentially in a crash).
  3. assert is most appropriate if this situation can't actually be hit (i.e. something else somewhere is protecting the limit).
  4. It might make sense to add a check earlier in the code to allow the assert here if this assert could otherwise be triggered.
ikudrin updated this revision to Diff 289725.Thu, Sep 3, 8:00 AM
ikudrin marked 2 inline comments as done.
  • Added reporting a fatal error if the generated debug info exceeds the limits supported by the format.
llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
594–601

In the update, I am adding reporting a fatal error in DwarfFile::computeSizeAndOffsets(), where the offsets for DWARF units are calculated and set. As DWARF64 is not supported for Apple targets, these asserts are now not expected to be hit with the normal usage.

jhenderson accepted this revision.Fri, Sep 4, 12:54 AM

LGTM. Might want to give @dblaikie another chance to comment.

dblaikie added inline comments.Thu, Sep 10, 6:41 PM
llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
594–601

By "not supported" you mean "has no guarantees if you ask for it" or do you mean "will produce an error early telling the user this isn't valid/supported" & thus this assert should (assuming no other programmer bugs - but importantly /not/ assuming some well intentioned user - a user can ask for an invalid thing and should get an error, not an assert failure/crash) ?

ikudrin added inline comments.Fri, Sep 11, 5:40 AM
llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
594–601

I mean that DWARF64 is not enabled for anything except ELF (see D87011), while computeAccelTableKind() returns AccelTableKind::Apple only if TT.isOSBinFormatMachO(), or if an internal tool is called with -accel-tables=Apple switch. The Apple accelerator tables are emitted only if getAccelTableKind() returns AccelTableKind::Apple, thus I cannot see a way for an end-user to trigger emitting those tables when DWARF64 is on. And if DWARF64 is off, and the debug data is extremely big, an error will be reported in DwarfFile::computeSizeAndOffsets().

dblaikie accepted this revision.Fri, Sep 11, 4:22 PM
dblaikie added inline comments.
llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp
594–601

Ah, I see (just stating it here for the record: D87011 makes the DWARF64 flag a no-op/silently does nothing except on ELF targets). Yep, assert seems like the right thing here then. Thanks for walking me through it.

This revision was automatically updated to reflect the committed changes.