DI: Don't use llvm.dbg.declare with non-alloca storage #2294

kinke · 2017-08-27T14:18:22Z

An alternative to #2292.

…` parameters.

kinke · 2017-08-27T17:23:59Z

It becomes more and more evident that LLVM doesn't like non-allocas for debuginfo params and locals. That's especially proving to be problematic for Win64 with its ExplicitByvalRewrite, where the storage for a parameter is an alloca of the caller and that pointer is passed as LL pointer parameter (and used directly for the callee's DI local).
We may need to represent the params rewritten by ExplicitByvalRewrite as DI references to outer variables to fix this. That corresponds to the dirty ABI details, but shouldn't actually concern the debugger IMO.

dnadlinger · 2017-08-27T17:31:47Z

Presumably you've considered this already, but at some point, wouldn't it be easier to tackle the problem in upstream LLVM? We would presumably make life better for a whole host of other compilers as well.

JohanEngelen · 2017-08-27T18:12:12Z

Does this work for optimized compilation with debug info (-O3 -g)? I read that metadata does not count as use of the alloca,
so perhaps the alloca is stripped immediately and then the debuginfo is lost, see http://nondot.org/sabre/LLVMNotes/DebugInfoVariableInfo.txt . I think we can use @llvm.dbg.value instead to point directly to the non-alloca variable.

kinke · 2017-08-27T18:40:24Z

I think we can use @llvm.dbg.value instead to point directly to the non-alloca variable.

This definitely sounds interesting. We have the same issue with nested variables, those aren't allocas either.

JohanEngelen · 2017-08-27T18:42:06Z

Yeah, I am working on trying with llvm.dbg.value now.
(edit: i think it only works for us here because the reference (pointer) is constant)

kinke · 2017-08-27T18:51:48Z

I hoped all that needed to be constant is the storage/lvalue of the variable. The official DI docs are, hmm, crappy; it shouldn't be that hard to associate a debuginfo variable with its (constant) LL lvalue?!

Edit: I'm reading the docs like you, so it may work for reference params here, but not for nested and rewritten ones. 'Declaring' seems the way to connect the DI variable with its LL lvalue, but here LLVM seems to expect local allocas. Our attempts to work around this for nested variables, by using the frame alloca and indexing it via DWARF expressions, didn't fully work out IIRC. We may want to check what clang does in similar circumstances (captured vars in lambdas or so).

JohanEngelen · 2017-08-27T19:04:03Z

I hoped all that needed to be constant is the storage/lvalue of the variable.

When the value is constant, it's storage (LLVM value) is also constant and all is well. When a value is not constant, on every write you'd get a new LLVM value (because of SSA), and then you'd have to retag that new value with llvm.dbg.value (because the LLVM value associated with the D variable has changed). But for non-constant things, we always use an alloca (right?), and we can use llvm.dbg.declare.

…oreach statement, where it _is_ an alloca.

kinke · 2017-08-27T22:50:40Z

Now based on reworked #2292, and the first results are fantastic, no issues with struct params > 64 bit anymore on Win64 (with LLVM 5.0). I still have to check the nested vars, but I'm very optimistic.
I also checked with the VS (2017) debugger, which differs from the command-line cdb in some aspects.

dnadlinger · 2017-08-27T23:01:14Z

gen/dibuilder.h

@@ -141,6 +141,8 @@ class DIBuilder {
  DIScope GetCurrentScope();
  void Declare(const Loc &loc, llvm::Value *var, ldc::DILocalVariable divar,
               ldc::DIExpression diexpr);
+  void DbgValue(const Loc &loc, llvm::Value *var, ldc::DILocalVariable divar,


Shouldn't this be just Value in line with Declare (or rename both)?

dnadlinger · 2017-08-27T23:08:25Z

gen/dibuilder.cpp

+  const bool storedInAlloca = llvm::dyn_cast<llvm::AllocaInst>(ll);
+  bool useDbgValueIntrinsic = false;
+  if (!storedInAlloca || vd->isRef() || vd->isOut()) {
+    useDbgValueIntrinsic = !storedInAlloca;


I find this a bit confusing to read, especially given the comment – is there a situation where ref and out actually do use an alloca?

See Johan's original comment in #2292 (comment). Apparently used for special-ref loop variables. It's a bit smelly and should be definitely looked into sometime.
Edit: 'Declaring' these special-ref vars seems to make perfect sense, as their reference is advanced with each iteration and not const as in the other cases.

Oh yes, they are fun – I called them SpecialRefVars in our code precisely because they don't map to anything else in the D language. In terms of looking into things, I've been wondering for a while how much of the extra DValue metadata we could do away with given that we have LLVM IR type information and frontend type information already. The current design seems needlessly complicated, although I can't really pinpoint any specific issues yet.

Represent the local or parameter as DI reference instead, using llvm.dbg.value to set the constant storage/lvalue. Affects parameters rewritten by ExplicitByvalRewrite and nested variables, possibly some more.

kinke · 2017-08-29T21:47:22Z

DbgValue() renamed to SetValue() and comments slightly reworked. Nested variables definitely still need to be fixed though, working on it.

kinke · 2017-08-31T15:17:56Z

Unfortunately, this approach apparently doesn't work for nested vars at all, neither on Win64 nor on Linux x64. I checked the IR manually and it looks okay to me. The nested vars don't show up in the VS debugger anymore (previously they were shown, although each with some attached error message); in gdb the same (symbols still known, but not listed in info locals, optimized out when trying to print them), where they previously seemed to work pretty well. No LLVM assertions, no failing tests, but fully broken debuginfos for nested vars, argh.

kinke · 2017-08-31T16:20:59Z

An example from tests/debuginfo/nested.d: arg0 captured in nested() is represented as DI reference with a constant value (the arg0 storage, a GEP into the parent frame):

%arg0 = getelementptr inbounds %nest.encloser, %nest.encloser* %2, i32 0, i32 0 ; [#uses = 1, type = i32*]
call void @llvm.dbg.value(metadata i32* %arg0, i64 0, metadata !28, metadata !16), !dbg !34 ; [debug line = 1:6] [debug variable = arg0]

!11 = !DIDerivedType(tag: DW_TAG_reference_type, baseType: !12, size: 64, align: 32)
!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
!16 = !DIExpression()
!28 = !DILocalVariable(name: "arg0", scope: !24, file: !3, line: 1, type: !11)

This results in the following naked DWARF var without any value on Linux x64 (via dwarfdump):

< 2><0x000000a0>      DW_TAG_variable
                        DW_AT_name                  "arg0"
                        DW_AT_decl_file             0x00000001 /home/martin/current.d
                        DW_AT_decl_line             0x00000007
                        DW_AT_type                  <0x0000011c>

< 1><0x00000115>    DW_TAG_base_type
                      DW_AT_name                  "int"
                      DW_AT_encoding              DW_ATE_signed
                      DW_AT_byte_size             0x00000004
< 1><0x0000011c>    DW_TAG_reference_type
                      DW_AT_type                  <0x00000115>

kinke · 2017-08-31T17:04:13Z

I tried using DW_OP_stack_value as additional expression. Quoting official DWARF docs:

The DW_OP_stack_value operation specifies that the object does not exist in memory but its value is nonetheless known and is at the top of the DWARF expression stack. In this form of location description, the DWARF expression represents the actual value of the object, rather than its location.

Result on Linux x64: no change at all, the vars still only feature a name, type and source location.

rainers · 2017-08-31T17:46:43Z

Unfortunately, this approach apparently doesn't work for nested vars at all, neither on Win64 nor on Linux x64.

Not sure if it is relevant to your approach, but non-trivial DWARF expressions were unsupported on Windows for ages until a month ago llvm-mirror/llvm@2cd77a8 added "rudimentary DwarfExpression support to CodeView".

kinke · 2017-08-31T19:09:46Z

Thanks, but I guess an expression shouldn't be really necessary here; I mean it's working for immediate pointers from parameters (ref/out params, params rewritten by ExplicitByvalRewrite and params with the LLVM byval attribute), but llvm.dbg.value apparently (silently) hates a GEP value.

kinke · 2017-09-03T19:50:19Z

Now trying to avoid all GEPs and using an alloca + more complex DWARF expression instead. Seems to work fine on Linux x64; the VS debugger on Win64 displays closure parameters and locals correctly, but the captured ones (in nested functions) are missing, possibly due to a too complex DWARF expression for LLVM 5.

For both closure and captured vars. Both llvm.dbg.declare() and llvm.dbg.value() intrinsics apparently don't like GEP values.

JohanEngelen · 2017-09-05T20:16:22Z

Related discussion on LLVM-dev, pointing out some of the limitations of the current debuginfo LLVM code: https://lists.llvm.org/pipermail/llvm-dev/2017-September/117141.html

kinke · 2017-09-05T21:25:01Z

Yeah with the alloca+expression approach, it's #1933 all over again; LLVM < 3.9 fails for our testcase. We speculated about it being an LLVM bug back then, and apparently later other testcases came up which also failed with LLVM 3.9, so that we went ahead with the GEP + weird additional dereference approach.

Also considering that LLVM < 5 misused DW_OP_plus as DW_OP_plus_uconst (different 'signature', I checked the DWARF standard while trying to fix the CI builds) is not really making me feel any more comfortable about LLVM's debuginfos.

The original issue triggering this again, LLVM 5 complaining about the LL pointer param as DI storage for non-nested ref/out params, was definitely a bug on our side, as it was declared as reference, but we passed the reference rvalue instead of its lvalue.

I'll probably split off the fix as a separate PR, as this is still clearly WIP and I'm not sure the alloca+expression route is the proper way of handling this, or whether LLVM is supposed to swallow GEPs just fine and we're just having a hard time trying to work around it 'properly'.

JohanEngelen · 2017-09-06T18:54:53Z

I'll probably split off the fix as a separate PR

Sounds good.

kinke · 2017-09-07T22:34:59Z

Obsoleted by #2315.

An attempt to fix runnable/gdb4149.d test: correct debuginfo for `ref…

bea0a37

…` parameters.

kinke force-pushed the di branch from 5a7f111 to b5f5beb Compare August 27, 2017 18:23

Attempt to use llvm.dbg.value instead for ref params.

152416b

ref variables are not always function parameters: could be from a f…

7563930

…oreach statement, where it _is_ an alloca.

kinke force-pushed the di branch from b5f5beb to e68afc9 Compare August 27, 2017 22:48

kinke changed the title ~~LLVM 5.0+: Fix debuginfos for ref/out parameters~~ DI: Don't use llvm.dbg.declare with non-alloca storage Aug 27, 2017

dnadlinger reviewed Aug 27, 2017

View reviewed changes

kinke mentioned this pull request Aug 27, 2017

Incomplete CodeView debug information #1716

Open

23 tasks

kinke force-pushed the di branch from e68afc9 to b367f26 Compare August 27, 2017 23:42

DI: Don't use llvm.dbg.declare with non-alloca storage

ff0af8c

Represent the local or parameter as DI reference instead, using llvm.dbg.value to set the constant storage/lvalue. Affects parameters rewritten by ExplicitByvalRewrite and nested variables, possibly some more.

kinke force-pushed the di branch 2 times, most recently from bc5b1e8 to ff0af8c Compare August 28, 2017 20:37

Cosmetics & improved comments wrt. llvm.dbg.value

d65a19b

kinke force-pushed the di branch from 0cb8c89 to d65a19b Compare August 29, 2017 21:30

Try to fix debuginfos for nested variables

276126b

JohanEngelen mentioned this pull request Aug 30, 2017

An attempt to fix runnable/gdb4149.d test: correct debuginfo for ref parameters. #2292

Closed

Fix DI storage for captured function parameters

ce60644

kinke force-pushed the di branch 2 times, most recently from a0aebeb to e1538bc Compare September 3, 2017 22:52

kinke added 2 commits September 4, 2017 01:28

Try to use alloca + DWARF expression for nested vars

a89782c

For both closure and captured vars. Both llvm.dbg.declare() and llvm.dbg.value() intrinsics apparently don't like GEP values.

Don't declare params passed via LLVM byval attribute as DI refs

aa1562e

kinke force-pushed the di branch from e1538bc to aa1562e Compare September 3, 2017 23:28

And some more tentative DI fixes

39f8286

kinke mentioned this pull request Sep 6, 2017

Adapt to LLVM 5.0 debuginfo changes & misc. fixes, mainly for Win64 #2315

Merged

kinke closed this Sep 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DI: Don't use llvm.dbg.declare with non-alloca storage #2294

DI: Don't use llvm.dbg.declare with non-alloca storage #2294

kinke commented Aug 27, 2017

kinke commented Aug 27, 2017 •

edited

Loading

dnadlinger commented Aug 27, 2017

JohanEngelen commented Aug 27, 2017 •

edited

Loading

kinke commented Aug 27, 2017

JohanEngelen commented Aug 27, 2017 •

edited

Loading

kinke commented Aug 27, 2017 •

edited

Loading

JohanEngelen commented Aug 27, 2017

kinke commented Aug 27, 2017 •

edited

Loading

dnadlinger Aug 27, 2017

dnadlinger Aug 27, 2017

kinke Aug 27, 2017 •

edited

Loading

dnadlinger Aug 28, 2017 •

edited

Loading

kinke commented Aug 29, 2017

kinke commented Aug 31, 2017 •

edited

Loading

kinke commented Aug 31, 2017 •

edited

Loading

kinke commented Aug 31, 2017

rainers commented Aug 31, 2017

kinke commented Aug 31, 2017

kinke commented Sep 3, 2017 •

edited

Loading

JohanEngelen commented Sep 5, 2017

kinke commented Sep 5, 2017

JohanEngelen commented Sep 6, 2017

kinke commented Sep 7, 2017

DI: Don't use llvm.dbg.declare with non-alloca storage #2294

DI: Don't use llvm.dbg.declare with non-alloca storage #2294

Conversation

kinke commented Aug 27, 2017

kinke commented Aug 27, 2017 • edited Loading

dnadlinger commented Aug 27, 2017

JohanEngelen commented Aug 27, 2017 • edited Loading

kinke commented Aug 27, 2017

JohanEngelen commented Aug 27, 2017 • edited Loading

kinke commented Aug 27, 2017 • edited Loading

JohanEngelen commented Aug 27, 2017

kinke commented Aug 27, 2017 • edited Loading

dnadlinger Aug 27, 2017

Choose a reason for hiding this comment

dnadlinger Aug 27, 2017

Choose a reason for hiding this comment

kinke Aug 27, 2017 • edited Loading

Choose a reason for hiding this comment

dnadlinger Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

kinke commented Aug 29, 2017

kinke commented Aug 31, 2017 • edited Loading

kinke commented Aug 31, 2017 • edited Loading

kinke commented Aug 31, 2017

rainers commented Aug 31, 2017

kinke commented Aug 31, 2017

kinke commented Sep 3, 2017 • edited Loading

JohanEngelen commented Sep 5, 2017

kinke commented Sep 5, 2017

JohanEngelen commented Sep 6, 2017

kinke commented Sep 7, 2017

kinke commented Aug 27, 2017 •

edited

Loading

JohanEngelen commented Aug 27, 2017 •

edited

Loading

JohanEngelen commented Aug 27, 2017 •

edited

Loading

kinke commented Aug 27, 2017 •

edited

Loading

kinke commented Aug 27, 2017 •

edited

Loading

kinke Aug 27, 2017 •

edited

Loading

dnadlinger Aug 28, 2017 •

edited

Loading

kinke commented Aug 31, 2017 •

edited

Loading

kinke commented Aug 31, 2017 •

edited

Loading

kinke commented Sep 3, 2017 •

edited

Loading