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

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AccelTable.cpp:594-601
+  assert(Die.getDebugSectionOffset() <= UINT32_MAX &&
+         "The section offset exceeds the limit.");
   Asm->emitInt32(Die.getDebugSectionOffset());
 }
 
 void AppleAccelTableTypeData::emit(AsmPrinter *Asm) const {
+  assert(Die.getDebugSectionOffset() <= UINT32_MAX &&
----------------
ikudrin wrote:
> dblaikie wrote:
> > ikudrin wrote:
> > > jhenderson wrote:
> > > > dblaikie wrote:
> > > > > 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)
> > > > My personal thoughts:
> > > > 
> > > > # If this code can actually be hit, it should be converted to return `Error`/`Expected` up to whatever point does the error handling.
> > > > # `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).
> > > > # `assert` is most appropriate if this situation can't actually be hit (i.e. something else somewhere is protecting the limit).
> > > > # It might make sense to add a check earlier in the code to allow the `assert` here if this `assert` could otherwise be triggered.
> > > 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 `assert`s are now not expected to be hit with the normal usage.
> > 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) ?
> 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()`.
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.


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

https://reviews.llvm.org/D87026