This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo] Avoid triggering global location assert for 2-byte pointer sizes.
ClosedPublic

Authored by jackoalan on Jan 1 2022, 1:19 PM.

Details

Summary

D111404 moved a 4/8 byte check assert into a block taken by 2-byte platforms.
Since these platforms do not take the branches where the pointer size is used,
sink the assert accordingly.

Diff Detail

Unit TestsFailed

Event Timeline

jackoalan created this revision.Jan 1 2022, 1:19 PM
jackoalan requested review of this revision.Jan 1 2022, 1:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 1 2022, 1:19 PM
jackoalan updated this revision to Diff 396882.Jan 1 2022, 2:22 PM

Fix test for windows

Could you add some details about where the assert came from/went to, link to the patch that made the change?

If this codepath that's being changed that relies on the pointer size variations isn't reachable by a 2 byte pointer platform, it might be better to move the assert & then it can be changed if/when the codoepath is used on a 2 byte pointer platform and the code changes can be tested.

jackoalan edited the summary of this revision. (Show Details)Jan 4 2022, 9:51 AM
jackoalan added a comment.EditedJan 4 2022, 9:53 AM

Could you add some details about where the assert came from/went to, link to the patch that made the change?

The assert was moved by D111404 in order to cover an additional branch that uses the pointer size. Neither the MSP430 nor AVR targets make use of TLS or RWPI globals.

The solutions are one of the following:

  • Duplicate the assert to each nested block instead
  • Add (!TLS && !RWPI) || ... logic to the assert
  • Add support for 2-byte data forms like this patch (which strikes me as the most general solution in a target-independent place like this)

Perhaps the right thing to do then would be to sink the assertion back down in some way - either just duplicating it in the two places it's used or using a lambda, perhaps?

Perhaps the right thing to do then would be to sink the assertion back down in some way - either just duplicating it in the two places it's used or using a lambda, perhaps?

Oh yeah, lambda would be a concise way to check that.

The lamdbda could assert and compute the pointer size - so the assertion stays close to the code that then uses the asserted property - and remove some of the duplication of computing the pointer size form in several places.

jackoalan updated this revision to Diff 397332.Jan 4 2022, 10:28 AM
jackoalan retitled this revision from [DebugInfo] Support 2-byte addresses for global location attributes. to [DebugInfo] Avoid triggering global location assert for 2-byte pointer sizes..
jackoalan edited the summary of this revision. (Show Details)

Sink assert using lambda

dblaikie added inline comments.Jan 4 2022, 10:41 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
263–308

Maybe like this? (open to better naming)

265–270

Could this instead be "GetPointerSizedForm" and encapsulate the PointerSize == 4 ? dwarf::DW_OP_const4u : dwarf::DW_OP_const8u logic too?

Oh, I see there's both OP and FORM needed, not just FORM... maybe still nice to share?

jackoalan updated this revision to Diff 397346.Jan 4 2022, 11:08 AM

Return resolved DWARF enums from lambda.

jackoalan marked 2 inline comments as done.Jan 4 2022, 11:09 AM
dblaikie accepted this revision.Jan 4 2022, 11:14 AM

Awesome, thanks!

This revision is now accepted and ready to land.Jan 4 2022, 11:14 AM
This revision was landed with ongoing or failed builds.Jan 4 2022, 12:18 PM
This revision was automatically updated to reflect the committed changes.