Issue 1716: Improve CodeView debug info #1717

rainers · 2016-08-21T18:19:33Z

Changes:

byte,ubyte should be encoded as DW_ATE_signed_char/DW_ATE_unsigned_char, not DW_ATE_signed/DW_ATE_unsigned
encode wchar,dchar as UTF
do not declare struct/class definitions as FlagFwdDecl
pass deco as unique identifier to UDTs
dynamic arrays: use type name, don't leave it empty
use proper type name for delegates
add support for type "void" and "typeof(null)"

Not sure how much of this can negatively affect other platforms. Should I put it under some Windows/codeView specific conditional?

kinke · 2016-08-21T18:38:32Z

gen/dibuilder.cpp

  case Twchar:
  case Tdchar:
+#if LDC_LLVM_VER >= 305


LLVM 3.5 is the oldest supported version.

Yeah, I changed the condition from 309 to let the build servers tell me what version supports the changes.

kinke · 2016-08-21T18:41:03Z

Should I put it under some Windows/codeView specific conditional?

This seems to make perfect sense to me, so I guess you're fixing debuginfos generally for all platforms. ;)

JohanEngelen · 2016-08-22T13:10:48Z

Not sure how much of this can negatively affect other platforms.

Anything in particular you are worried about?

Also, could you add IR tests for the things you've fixed here?

kinke · 2016-08-22T15:23:37Z

gen/dibuilder.cpp

@@ -131,18 +139,28 @@ ldc::DIType ldc::DIBuilder::CreateBasicType(Type *type) {
    Encoding = DW_ATE_boolean;
    break;
  case Tchar:
+    Encoding = type->isunsigned() ? DW_ATE_unsigned_char : DW_ATE_signed_char;


Ideally, this would be a DW_ATE_UTF too for UTF8-char, right?

Yes, but this is not supported in the CodeViewDebug::lowerTypeBasic.

kinke · 2016-08-22T15:36:48Z

What's the forward declaration issue? I've seen that compilable/test16080.d was the only failing one yesterday, and that one imports a struct A() { static immutable A a; }. So this (= referencing a currently declared parent DI type in a member, I think) works for LLVM 3.9 but needs the fwd-declared flag for older LLVMs?!

rainers · 2016-08-22T17:43:48Z

Anything in particular you are worried about?

Breaking debugging in gdb, which might only support a different combination of debug data. See the breakage of the CI builders...

Also, could you add IR tests for the things you've fixed here?

I could try that, but it wouldn't really test correctness, as this very much depends on the debug info emission and the used debugger.

JohanEngelen · 2016-08-22T19:20:32Z

but it wouldn't really test correctness, as this very much depends on the debug info emission and the used debugger.

It would test correct IR emission, right? If there are different paths for tuning the output for different debuggers (something we don't do but clang does), you can easily extend testing with passing commandline flags that trigger those paths.
(I wouldn't bother with old LLVM versions, just add // REQUIRES: atleast_llvm308 to the tests so you don't have to deal with all of the debuginfo changes that happened in LLVM)

The CI systems breaking is exactly why we need to have tests for this. Otherwise someone in the future is going to "fix" things for GDB again, but break the CodeView debuginfo, and your work is lost.

(btw, combining all these little changes into one PR is not helping!)

dnadlinger · 2016-08-22T20:27:45Z

The CI systems breaking is exactly why we need to have tests for this.

I'd argue that this is why we need similar CI tests for debugging on Windows as well.

The problem with IR-level tests is that they are exceedingly brittle – how do you know whether a change was benign or not? What about future changes to the (textual) IR format, such as possibly with the next LLVM release?

It is clear that we need rather comprehensive "integration tests" for debug info generation that actually use the target debugger, because there is just too much that can go wrong in the interplay of poorly specified components. The question then becomes whether IR-level tests on top of that add sufficient value.

kinke · 2016-08-22T21:16:23Z

I was just about to post the llvm::StringRef(nullptr) bugfix. ;)

kinke · 2016-08-22T21:31:12Z

gen/dibuilder.cpp

+#if LDC_LLVM_VER >= 309
+  return t->merge()->deco;
+#else
+  return nullptr;


Better return the llvm::StringRef directly here, and then the bugfix nullptr => {} needs to be applied here as well.

Thanks, done.

rainers · 2016-08-22T21:34:06Z

It is clear that we need rather comprehensive "integration tests" for debug info generation that actually use the target debugger

As an intermediate step, we could use object file dumpers to verify the debug info output. llvm-dwarfdump/llvm-readobj should do the job, there is also cvdump.exe released by Microsoft: https://github.com/Microsoft/microsoft-pdb/tree/master/cvdump.

I'm not yet familiar with FileCheck, but it seems it could help with this verification, too.

rainers · 2016-08-22T21:42:24Z

(btw, combining all these little changes into one PR is not helping!)

Sorry, I was hoping for easy passing of the builds. Seeing the breakage in all the previous LLVM versions I begin to regret that decision, too.

JohanEngelen · 2016-08-23T12:19:40Z

The problem with IR-level tests is that they are exceedingly brittle – how do you know whether a change was benign or not? What about future changes to the (textual) IR format, such as possibly with the next LLVM release?

I think "exceedingly brittle" is overstating things. When writing a test, you have to check for the things that matter, and not for the things that don't. FileCheck's regexp matching is pretty nice for that. When the IR text changes in a newer LLVM release, you can update the test for that using REQUIRES: atleast_llvm* or similar; the tests for earlier LLVM versions remain valid. If we want to check the alignment of a variable, we must check the IR and trust LLVM, instead of going into a binary and checking addresses (where passing the test may happen accidentally). Same for debuginfo.

There are many things here that are easily tested in a robust manner. If you don't want to add tests for workarounds, that's fine.

JohanEngelen · 2016-08-23T12:31:03Z

I'm not yet familiar with FileCheck, but it seems it could help with this verification, too.

FileCheck is just a string matcher. So whenever a program outputs text data, you can feed it into FileCheck and it will check whether certain strings are there (or check that they are not there). Usually we write the strings-to-be-checked-for (e.g. // CHECK:) interleaved with D source code, that's perhaps confusing at first. But essentially FileCheck doesn't care and only looks at the CHECK lines. You could just as well feed FileCheck a different file with string checks in it.

JohanEngelen · 2016-08-23T12:53:29Z

Anyway, all I'm saying is: testing this would be very good, whichever method you choose.

kinke · 2016-08-24T17:51:52Z

I'd say let's get these first steps merged.

I'd suggest using Type::deco as name only if the type is already merged (t->nextOf()->deco != nullptr), in order not to touch Type::next until we figure out the underlying issue there (just an idea - element type of void[N], where void is to be interpreted as a ubyte?).
I'll check whether gdb supports DW_ATE_UTF for char. If so, I'd say let's use DW_ATE_unsigned_char (signed UTF8 chars/code points don't make sense to me - edit: but I guess clang emits DW_ATE_signed_char for the C++ char) for MSVC targets only.
- Similarly, I'd suggest using DW_ATE_[un]signed_char for [u]byte only for MSVC targets, and keep on using DW_ATE_[un]signed for all other targets.
#if LDC_LLVM_VER >= 305 for {d,w}char isn't required.
Please re-add a space after the removed FwdDecl flag in line 398 (or just re-clang-format). ;)

kinke · 2016-08-24T20:45:59Z

Using DW_ATE_UTF or DW_ATE_signed_char for all D character types makes no difference for gdb v7.11.1 (e.g., char c1 = 101 'e'). Values > 8 bit are displayed as decimal numbers; no fancy Unicode characters unfortunately - also for wide strings, e.g., wstring w = {220, 98, 101, 114}.

~~There's a difference for 8-bit DW_ATE_[un]signed, for which gdb shows both the decimal and, rather strangely, an octal representation (byte a = -2 '\376', ubyte b = 248 '\370').~~ Edit: That octal one is just shown for non-printable characters apparently. Still a better option than DW_ATE_[un]signed_char for non-MSVC targets IMO though.

rainers · 2016-08-24T22:05:01Z

I've removed changing Type.next, maybe calling "t->merge()" is also not necessary without it.
The changes to character types are now MSVC specific. UTF16 and UTF32 display nicely in VS (for single characters and strings), but char[] is not UTF8, but some other encoding.

rainers · 2016-08-24T22:20:07Z

maybe calling "t->merge()" is also not necessary without it.

Removed that, too.

BTW: the debug info for local variables emitted by LDC doesn't work at all with the "old" debug engine (the one that you get when you enable "native" edit and continue in VS2013 or earlier). The UTF16/UTF32 types also don't show in VS2013.
I guess mago will also need special support for LLVM debug output...

dnadlinger · 2016-08-24T22:29:49Z

@JohanEngelen: Definitely agreed on the alignment use case. My claim is just that for debug info, the IR layer is not the correct place for tests, as it doesn't really have much meaning by itself – as you pointed out from Clang, it is a rather leaky abstraction. What we really want to test is that debug info is lowered to the object/PDB files in a way we know works with the target debuggers. We can probably dump the debug info into a textual form, though, and use FileCheck on that, like Rainer suggested (this is way less complex than requiring debugger scripting, of course).

kinke · 2016-08-25T08:29:41Z

gen/dibuilder.cpp

@@ -131,18 +139,33 @@ ldc::DIType ldc::DIBuilder::CreateBasicType(Type *type) {
    Encoding = DW_ATE_boolean;
    break;
  case Tchar:
+    if (global.params.targetTriple->isWindowsMSVCEnvironment()) {
+      // VS debugger does not support DW_ATE_UTF for char
+      Encoding = type->isunsigned() ? DW_ATE_unsigned_char : DW_ATE_signed_char;


A D char is always unsigned, right? [I can't test it myself at work right now]

A D char is always unsigned, right? [I can't test it myself at work right now]

Yes. This is just the original code, but I guess it's clearer to just emit DW_ATE_unsigned_char.

JohanEngelen · 2016-08-25T10:18:20Z

My claim is just that for debug info, the IR layer is not the correct place for tests, as it doesn't really have much meaning by itself – as you pointed out from Clang, it is a rather leaky abstraction. What we really want to test is that debug info is lowered to the object/PDB files in a way we know works with the target debuggers. We can probably dump the debug info into a textual form, though, and use FileCheck on that, like Rainer suggested (this is way less complex than requiring debugger scripting, of course).

@klickverbot Yeah I got that. I was thinking about LLVM modifying the IR debuginfo when optimizing (this is what causes some ICEs, I still have to file bugs about it, argh). So the IR has to be "correct" in a certain way too. Regardless, let's get things tested, whichever way :)

rainers · 2016-08-27T08:39:20Z

Anyway, all I'm saying is: testing this would be very good, whichever method you choose.

I investigated a couple of options:

the output of llvm-readobj -codeview is very verbose and difficult to check because it spreads information across a lot of lines. More severely, it contains a lot of backward references that don't seem to be supported by the [[capture]] feature of FileCheck. Information is dumped less verbose by Microsofts cvdump but it still needs backward reference support.
I tried cdb, the console version of windbg, which could be used similar to the gdb scripts in the dmd testsuite. Unfortunately, it doesn't support the UTF char types changed in this PR (e.g. it prints <CLR type> <unknown base type 80000013>).
The D debug engine mago very recently got a console version, but it seems to be too basic at the moment. We could tweak it to make it a test tool, but we'd probably have to implement support for some CodeView features used by LLVM.
The IR output is likely to be better testable by FileCheck, but as said, it doesn't cover the system/debugger specific parts.

I'm not 100% which option to choose, but my current preference would be cdb while ignoring UTF16/UTF32 types. The latter seem to be supported by the Concord debug engine (https://github.com/Microsoft/ConcordExtensibilitySamples/wiki/Overview) only. This raises the question what engine to target: the latest default engine in VS2015 or a more common subet of features that also work in windbg?

kinke · 2016-08-27T09:05:11Z

I'm not 100% which option to choose, but my current preference would be cdb while ignoring UTF16/UTF32 types. The latter seem to be supported by the Cordova debug engine (https://github.com/Microsoft/ConcordExtensibilitySamples/wiki/Overview) only. This raises the question what engine to target: the latest default engine in VS2015 or a more common subet of features that also work in windbg?

Cool, I didn't know there was a console version of windbg. I'd tend towards it too then. So it'd be cool if most stuff worked for both windbg and VS 2015+ (the LLVM guys are thinking about dropping pre-2015 support too). Afaik, windbg still offers functionality you don't get in the VS debugger, for hardcore debugging sessions; and it allows subsequent automated testing via cdb for AppVeyor then. UTF16 and UTF32 not showing up properly in windbg is something I could live with, as it's not supported by gdb shipped with Ubuntu 16.04 either (and as I said, using DW_ATE_[un]signed_char doesn't help gdb in that regard).

rainers · 2016-08-28T09:13:56Z

Added a cdb based test.

kinke · 2016-08-28T14:58:26Z

tests/lit.site.cfg.in

+if (platform.system() == 'Windows') and (config.default_target_bits == 32):
+    cdb = config.environment['WindowsSDKDir'] + 'Debuggers\\x86\\cdb.exe'
+if (platform.system() == 'Windows') and (config.default_target_bits == 64):
+    cdb = config.environment['WindowsSDKDir'] + 'Debuggers\\x64\\cdb.exe'


Apparently lit's TestingConfig.environment is only a small subset of the actual environment variables. So accessing those directly via os.environ['WindowsSdkDir'] should do the job.

So accessing those directly via os.environ['WindowsSdkDir'] should do the job.

Thanks, fixed. There was also an import missing. [My last tries were from a different computer that cannot realistically compile LDC in finite time.]

[My last tries were from a different computer that cannot realistically compile LDC in finite time.]

Off topic, but out of curiosity: What sort of system was that? I've found LDC to be relatively quick to build even on a 32 bit ARM board with only 1 GB of RAM and slow storage – it's LLVM that's the annoying step.

Off topic, but out of curiosity: What sort of system was that?

A Core2 Duo @ 2 GHz, with 2 GB of RAM. You might be right, with a precompiled LLVM, it could be managable. I've seen failures compiling dmd due to other applications eating too much memory...

dnadlinger · 2016-08-29T02:47:57Z

What happened to the AppVeyor build? It looks like we should get this merged as soon as possible (possibly after cleaning up the history a bit).

rainers · 2016-08-29T06:35:36Z

What happened to the AppVeyor build?

The cdb integration test fails ;-( As it works on my system, I'll have to grab some more output from the build server to figure out why.

kinke · 2016-08-29T23:53:06Z

tests/debuginfo/codeview.d

@@ -13,7 +13,8 @@ int main(string[] args)
    wstring ws = "b";
    dstring ds = "c";

-// CDB: bp `codeview.d:18`
+// CDB: ld codeview
+// CDB: bp `codeview.d:19`


Congratz, finally working. 👍 Some stashing (- AppVeyor hack) and it's good to be merged.
There's an offset by +1 line, right? I seem to recall that from VS too...

The problem was not a line offset (I had to change it here due to the added line), but -s above caused loading of other modules' symbols aswell, so it displayed combase!STRING instead of codeview!string.

- byte,ubyte should be encoded as DW_ATE_signed_char/DW_ATE_unsigned_char, not DW_ATE_signed/DW_ATE_unsigned - encode wchar,dchar as UTF - do not declare struct/class definitions as FlagFwdDecl - pass deco as unique identifier to UDTs - dynamic arrays: use type name, don't leave it empty - use proper type name for delegates - add support for type "void" and "typeof(null)"

rainers · 2016-08-30T07:06:43Z

Squashed commits and added test for x86 codeview output.

dnadlinger · 2016-08-30T07:55:38Z

Nice!

rainers · 2016-08-31T21:12:02Z

Thanks, I updated #1716 accordingly.

kinke reviewed Aug 21, 2016
View reviewed changes

rainers force-pushed the cv1 branch from f110a5f to 1ea43a4 Compare August 21, 2016 18:40

kinke reviewed Aug 22, 2016
View reviewed changes

rainers force-pushed the cv1 branch from 7063bfd to 608642b Compare August 22, 2016 21:14

kinke reviewed Aug 22, 2016
View reviewed changes

rainers force-pushed the cv1 branch from d894c2d to b63cc01 Compare August 24, 2016 22:14

kinke reviewed Aug 25, 2016
View reviewed changes

rainers force-pushed the cv1 branch from f21dfe8 to 479de3c Compare August 28, 2016 09:12

rainers force-pushed the cv1 branch 4 times, most recently from 16601d6 to 59ec9ae Compare August 28, 2016 12:30

kinke reviewed Aug 28, 2016
View reviewed changes

rainers force-pushed the cv1 branch 2 times, most recently from 8a4e73a to c33f0b3 Compare August 29, 2016 17:12

kinke reviewed Aug 29, 2016
View reviewed changes

rainers added 4 commits August 30, 2016 08:17

add CodeView test running the built executable through cdb

3142798

circleCI: uninstall existing llvm package before installing llvm 3.9

5f643d6

test codeview output on x86, too

3c12fe9

rainers force-pushed the cv1 branch from 4760d6f to 3c12fe9 Compare August 30, 2016 07:05

fix x86 test for struct value argument

6d55645

dnadlinger merged commit 719717f into ldc-developers:master Aug 31, 2016

Issue 1716: Improve CodeView debug info #1717

Issue 1716: Improve CodeView debug info #1717

Conversation

rainers commented Aug 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Aug 21, 2016

JohanEngelen commented Aug 22, 2016 • edited

kinke Aug 22, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kinke commented Aug 22, 2016 • edited

rainers commented Aug 22, 2016

JohanEngelen commented Aug 22, 2016

dnadlinger commented Aug 22, 2016

kinke commented Aug 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Aug 22, 2016

rainers commented Aug 22, 2016

JohanEngelen commented Aug 23, 2016

JohanEngelen commented Aug 23, 2016

JohanEngelen commented Aug 23, 2016

kinke commented Aug 24, 2016 • edited

kinke commented Aug 24, 2016 • edited

rainers commented Aug 24, 2016

rainers commented Aug 24, 2016

dnadlinger commented Aug 24, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohanEngelen commented Aug 25, 2016

rainers commented Aug 27, 2016 • edited

kinke commented Aug 27, 2016

rainers commented Aug 28, 2016

kinke Aug 28, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnadlinger commented Aug 29, 2016

rainers commented Aug 29, 2016

kinke Aug 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rainers commented Aug 30, 2016

dnadlinger commented Aug 30, 2016

rainers commented Aug 31, 2016

JohanEngelen commented Aug 22, 2016 •

edited

kinke Aug 22, 2016 •

edited

kinke commented Aug 22, 2016 •

edited

kinke commented Aug 24, 2016 •

edited

kinke commented Aug 24, 2016 •

edited

dnadlinger commented Aug 24, 2016 •

edited

rainers commented Aug 27, 2016 •

edited

kinke Aug 28, 2016 •

edited

kinke Aug 29, 2016 •

edited