Page MenuHomePhabricator

[AddressSanitizer] Ensure only AllocaInst is passed to dbg.declare
ClosedPublic

Authored by vsk on Feb 10 2020, 4:50 PM.

Details

Summary

Various parts of the LLVM code generator assume that the address
argument of a dbg.declare is not a ptrtoint-of-alloca. ASan breaks
this assumption, and this results in local variables sometimes being
unavailable at -O0.

GlobalISel, SelectionDAG, and FastISel all do not appear to expect
dbg.declares to have a ptrtoint as an operand. This means that they do
not place entry block allocas in the usual side table reserved for local
variables available in the whole function scope. This isn't always a
problem, as LLVM can try to lower the dbg.declare to a DBG_VALUE, but
those DBG_VALUEs can get dropped for all the usual reasons DBG_VALUEs
get dropped. In the ObjC test case I'm looking at, the cause happens to
be that replaceDbgDeclare has hoisted dbg.declares into the entry
block, causing LiveDebugValues to "kill" the DBG_VALUEs because the
lexical dominance check fails.

To address this, I propose:

  1. Have ASan (always) pass an alloca to dbg.declares (this patch). This is a narrow bugfix for -O0 debugging.
  1. Make replaceDbgDeclare not move dbg.declares around. This should be a generic improvement for optimized debug info, as it would prevent the lexical dominance check in LiveDebugValues from killing as many variables.

    This means reverting llvm/r227544, which fixed an assertion failure (llvm.org/PR22386) but no longer seems to be necessary. I was able to complete a stage2 build with the revert in place.

rdar://54688991

Diff Detail

Event Timeline

vsk created this revision.Feb 10 2020, 4:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2020, 4:50 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

This all sounds good to me (other than "hey can we start getting rid of these intrinsics?" :)

rnk added a subscriber: rnk.Feb 10 2020, 5:04 PM

I can think of several cases where non-alloca values are passed to dbg.declare, so if you want to make the verifier change, there's much more work to do. It might be better just to make the backends handle these cases.

Here is the most common case, passing non-trivially copyable objects by value:

struct Foo {
  Foo();
  Foo(const Foo &o);
  ~Foo();
  int x;
};
int receiveFoo(Foo o) { return o.x; }

In this case, the C++ ABI passes the argument by address. Arguably, in this case, clang should do what it does for references: store the pointer into a stack slot and adjust the DIExpression to indicate that the stack slot contains an address, not a value.

The next most common would be byval:

struct Foo {
  int x;
  int ys[6];
};
int receiveFoo(Foo o) { return o.x; }

In this case, o is already in memory, and it would be unreasonable for clang to copy it into a local alloca, since it could be large. The verifier should probably allow dbg.declare to point to a byval argument, since they are just stack objects for most targets.

The next case would be inalloca, which is like byval, but there is a GEP in the way. Compile example 1 with --target=i686-windows-msvc, and it looks like so:

define dso_local i32 @"?receiveFoo@@YAHUFoo@@@Z"(<{ %struct.Foo }>* inalloca %0) #0 !dbg !8 {
entry:
  %o = getelementptr inbounds <{ %struct.Foo }>, <{ %struct.Foo }>* %0, i32 0, i32 0
  call void @llvm.dbg.declare(metadata %struct.Foo* %o, metadata !25, metadata !DIExpression()), !dbg !26

This needs to keep working, although I have tried to come up with alternative designs: https://github.com/rnk/llvm-project/blob/call-setup-docs/llvm/docs/CallSetup.md

The next case I can think of would be this, ASan.

Next, safestack, which I believe also replaces static allocas.

I'd rather createAllocaForLayout returned an alloca pointer and intptr was applied to it later.
Also, would not this new assert fire in the case of fake stack?

vsk edited the summary of this revision. (Show Details)Feb 10 2020, 5:13 PM
vsk planned changes to this revision.Feb 10 2020, 5:17 PM

Apologies, it looks like the various ISel implementations do have some way to handle dbg.declares which point to arguments. In light of that it doesn't seem desirable to ban non-alloca dbg.declares outright. There might be a narrower condition that's worth enforcing, but that'll take more thought and analysis.

I'll work on incorporating feedback from @eugenis.

This part of the patch LGTM.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
3133

// The ptrtoint operation is transparent to the debug info and only confuses later passes.

llvm/test/DebugInfo/X86/asan_debug_info.ll
4

While having an end-to-end test is great, I wonder if we should also have an IR->IR test that just specifies the ASAN transform's expected behavior.

vsk updated this revision to Diff 244032.Feb 11 2020, 4:55 PM
vsk marked 4 inline comments as done.
vsk edited the summary of this revision. (Show Details)

Address review feedback.

llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp
3118

@eugenis I tried to change createAllocaForLayout to return an alloca, but I couldn't manage it. This required changes all over this pass. I tried tackling this twice, but the resulting IR from the pass was broken in ways I didn't quite understand.

3133

I've reworded this a bit, lmk if it's all right.

3137

@eugenis Re:

Also, would not this new assert fire in the case of fake stack?

I think LocalStackBaseAlloca is either an alloca, or is the result of createAllocaForLayout, so the assert should not fire. Testing with an ASan-enabled stage2 build and with check-llvm was clean.

llvm/test/DebugInfo/X86/asan_debug_info.ll
4

The existing IR->IR tests do cover what the ASan transform produces. I'll leave a note about this in one of the tests: they used to match the ptrtoint, I've changed one of them to match an alloca.

eugenis accepted this revision.Feb 11 2020, 5:08 PM

LGTM

This revision is now accepted and ready to land.Feb 11 2020, 5:08 PM
aprantl accepted this revision.Feb 12 2020, 8:53 AM
aprantl added inline comments.
llvm/test/Instrumentation/AddressSanitizer/debug_info.ll
26

Ah here!

This revision was automatically updated to reflect the committed changes.
vsk added a comment.Feb 12 2020, 4:56 PM

Looking into this some more, I no longer think it's all that important to stop replaceDbgDeclarefrom moving dbg.declares around. But I still think we should make the change as a cleanup, more context in: https://reviews.llvm.org/D74517