This is an archive of the discontinued LLVM Phabricator instance.

LowerDbgDeclare: look through bitcasts.
ClosedPublic

Authored by eugenis on Nov 26 2019, 4:38 PM.

Details

Summary

Emit a value debug intrinsic (with OP_deref) when an alloca address is
passed to a function call after going through a bitcast.

This generates an FP or SP-relative location for the local variable in
the following case:

int x;
use((void *)&x;

Event Timeline

eugenis created this revision.Nov 26 2019, 4:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2019, 4:38 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
aprantl added inline comments.Dec 2 2019, 11:13 AM
llvm/test/Transforms/Util/dbg-call-bitcast.ll
8

What is %1 going to be after this transformation? Can you add a check for that, too?

aprantl added inline comments.Dec 2 2019, 11:15 AM
llvm/lib/Transforms/Utils/Local.cpp
1425

I think the worklist is overkill here, since it will contain at most one element. How about turning the loop body into a helper function/lambda and calling it recursively?

vsk added inline comments.Dec 3 2019, 11:04 AM
llvm/lib/Transforms/Utils/Local.cpp
1425

I think the worklist would contain more than one element in this case:

%a = alloca
%c1 = bitcast %a to ...
call void @use(%c1)
%c2 = bitcast %a to ...
call void @use(%c2)

That is probably worth a test, though.

aprantl added inline comments.Dec 3 2019, 1:16 PM
llvm/lib/Transforms/Utils/Local.cpp
1425

But that would also work fine with the recursive implementation, right?

1425

Unless we are expecting very deep recursion I would prefer the recursive solution over the worklist since it is much easier to understand.

vsk added inline comments.Dec 3 2019, 2:48 PM
llvm/lib/Transforms/Utils/Local.cpp
1425

Yes, a recursive approach would work fine. Fwiw, the patch as-written seems idiomatic to me (the alloca use visitation logic in both CaptureTracking & PtrUseVisitor look like this).

I also find worklist a more common and recognizable pattern, but I don't mind recursion either.

Btw, this function terribly incomplete. This is not the only case when it destroys perfectly good debug info. For example, storing an address of a local variable to memory does not generate a dbg.value. In the following example "y" has no location at -O3:

int *p;
void publish_and_wait();
void f() {
  int y;
  p = &y;
  publish_and_wait();
}

I'm looking at this because HWASan tries to use debug info to recover stack frame layout at run time. This works as long as we get at least one frame pointer based location for a non-promoted variable. I'm considering disabling this optimization altogether in functions with sanitize_hwaddress attribute; or maybe adding a dbg.value with DW_OP_deref to all allocas in such functions.

vsk added inline comments.
llvm/lib/Transforms/Utils/Local.cpp
1448

Yeah, this is also giving up on chasing GEPs. PtrUseVisitor has a facility to follow GEPs and figure out the accumulated offset, so we could try to emit:

%a = alloca
%g = gep %a, 0, 4
store i32 %val, %g
; emit: dbg.value(%val, <var>, DIExpression(OP_LLVM_fragment 32 32))

It's certainly a larger change, so I wouldn't be opposed to landing a narrow fix for now, but perhaps that's an interesting future direction.

I also find worklist a more common and recognizable pattern, but I don't mind recursion either.

It's not that the worklist is terribly complicated to understand either, but when I see a worklist I'm expecting a tricky algorithm that couldn't be implemented with recursion and then need to stop and spend some time that it's really doing something simple. Not a big deal though. In the grand scheme of things both approaches are fine. The recursion has the advantage that we can give the helper function a descriptive name.

eugenis marked 3 inline comments as done.Dec 4 2019, 4:41 PM

I don't have a strong opinion either way, but since we seem to have 2-1 in favor of the worklist I'll leave it as is.
Please let me know if you disagree.

I've updated the test case to include two bitcasts of the same alloca.

llvm/lib/Transforms/Utils/Local.cpp
1448

Agreed.

aprantl accepted this revision.Dec 4 2019, 5:10 PM
This revision is now accepted and ready to land.Dec 4 2019, 5:10 PM
This revision was automatically updated to reflect the committed changes.