This is an archive of the discontinued LLVM Phabricator instance.

[Assignment Tracking] Fix fragments for assignments to variables smaller than the alloca
ClosedPublic

Authored by Orlando on Apr 7 2023, 2:59 AM.

Details

Summary

Prior to this patch the trackAssignments function would attribute all stores to an alloca to all variables linked to the alloca. This is wrong in the case where the alloca contains variables which are smaller than the alloca, and caused erroneous fragment information to be generated.

Now stores outside the variable bounds are discarded, and we check whether a fragment is needed based on whether the store covers the entire variable as opposed to whether it covers the entire alloca (except for variables of unknown size).

Note that trackAssignments doesn't yet understand whole variables sitting at anything other than offset 0 in an alloca - those variables are still tracked using dbg.declares.

Fixes https://lab.llvm.org/buildbot/#/builders/70/builds/36007

Diff Detail

Event Timeline

Orlando created this revision.Apr 7 2023, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2023, 2:59 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Orlando requested review of this revision.Apr 7 2023, 2:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 7 2023, 2:59 AM
jmorse accepted this revision.Apr 7 2023, 3:30 AM

What would be the implications of a store that partially overlaps an AT tracked variable -- as a result of this patch, we would lose some of that information (to avoid a crash)? It feels like an extremely unlikely edge case, so doesn't worry me too much, but is the ultimate fix to truncate the assignment here, or just not produce that kind of debug-info in the first place? And if the latter, should we assert for that, or just conclude "SROA really hates what you're doing in this situation".

Otherwise LGTM

llvm/lib/IR/DebugInfo.cpp
1849

all variables that _reach_ here? Otherwise it's unclear what context "here" is in.

1850
This revision is now accepted and ready to land.Apr 7 2023, 3:30 AM
This revision was automatically updated to reflect the committed changes.
Orlando marked 2 inline comments as done.

What would be the implications of a store that partially overlaps an AT tracked variable -- as a result of this patch, we would lose some of that information (to avoid a crash)?

Yeah that's right.

It feels like an extremely unlikely edge case, so doesn't worry me too much, but is the ultimate fix to truncate the assignment here, or just not produce that kind of debug-info in the first place? And if the latter, should we assert for that, or just conclude "SROA really hates what you're doing in this situation".

I think truncating would make sense too - I've added a note in the comments but I think you're right that the situation should be extremely rare.

Thanks for the review.