issue 4014: output debug info for all referenced classes/structs and enumerators

[rainers]

This patch outputs full debug info for anything that is referenced, not only for declaration in modules on the command line. Leave it to the linker to filter out duplicates.

In toobj.c, it might also be possible to just leave out the call to toDebug()/toCtype(), because the debug info will always end up where the declaration is used.

The patch in cgcv.c was necessary, because it crashed on some debug info for me. Is the hack still necessary?

bugzilla: http://d.puremagic.com/issues/show_bug.cgi?id=4014

[WalterBright]

I'm not sure this is a good idea. People rely on debug builds being fast, and duplicationg the debug info in ever .obj file can make for pretty slow builds.

[bhelyer]

Furthermore passing unneeded symbols to OPTLINK could be a Bad Idea (™).

[rainers]

I don't think debug info has a big impact on build times. I tried it
with Visual D:

combined file compilation (build all modules to one object file)

• with debug info (including the patch): 24 sec
• without debug info: 24 sec

single file compilation (build all modules to object file first, then link)

• with debug info (including the patch): 3min 45 sec
• without debug info: 3min 31 sec

so generating the debug info needs about 5% more compilation time, and
the additional cost of this patch is probably only a fraction of that time.

[rainers]

Furthermore passing unneeded symbols to OPTLINK could be a Bad Idea (™)

That could be a valid concern ;-) But I have not seen any difficulties
yet, even with large debug info (Visual D generates more than 10MB).

[braddr]

This pull request has been broken on win32 for essentially forever. It causes dmd to crash which on win32 pops up a dialog box, stalling the auto-tester. For now, I've disabled it from being tested (on every platform, sorry, quick hack). If/when it's fixed, let me know and I can remove the block.

[WalterBright]

I just don't like the idea of duplicating the symbolic debug info. In reading the bug report, it seems the primary issue is that symdeb info is only generated for the module with the .init data.

A better solution may be to have a module generated with only symdeb info for a class, and a single global defined. Then, other modules can reference that single global, thereby pulling in the symdeb data.

[rainers]

I think that it could work, it was also my first idea about fixing the problem. But it has the drawback that you need to have the library compiled with debug information to be able to inspect data types declared in the library. For example, the phobos library in a standard installation comes without debug info, so you won't have any luck investigating non-templated structs or classes that are declared in phobos or druntime.

Another (maybe constructed) argument: A struct with versioned members might not generate the same debug info when compiling the library and when actually using it in user code with the versioned declarations changed. This is a real problem in C/C++, the debugger can even switch between different representations when jumping from one source to another. I think the safer solution would be if the linker complains.

The solution in the pull request is pretty simple and I haven't seen issues with it yet (I get linker crashes when compiling std.algorithm unittests with debug symbols for the library, but IIRC it happens without the patch aswell).

If there is interest I can rebase the pull request and fix it.

[WalterBright]

Another option may be to enable it with a compiler switch, say, -g2.

[rainers]

I rebased the branch and fixed the problems that were also happening with the pull tester, but something caused the diff to gather other commits aswell. Can someone with more git-fu tell me what went wrong and how can I reduce the commits to my own ones?

[alexrp]

Given upstream as the upstream remote, do

git rebase upstream/master

while on your branch.

[rainers]

Some comments to the changes: I ran the test suite with command line option -g, but had to add a few tweaks:

• I added the missing patch from Issue 5985 - string enum fails with debugging enabled  #257 to compile xtestenum.d
• I had to disable fail4.d due to http://d.puremagic.com/issues/show_bug.cgi?id=8664
• fixed crashes that were caused by missing type information to generate debug info from, I am not sure if these are ok or mere workarounds to missing type inference elsewhere.

I measured building the test suite with and without the patch, with and without debug info (not used multi-processor execution):

clean quick: 1:11min, with -g: 1:18min
clean full : 6:58min, with -g: 7:12min

4014a quick: 1:11min, with -g: 1:13min
4014a full : 6:29min, with -g: 7:19min

Unfortunately the results for the full build don't seem very consistent, my processor is a mobile i7 with Turbo Boost. But I think they suggest that there is no noticeable build time penalty with the patch.

[rainers]

|>||git rebase upstream/master|

Thanks. But when I do this and push, I get

To https://rainers@github.com/rainers/dmd.git
! [rejected] issue4014a -> issue4014a (non-fast-forward)
error: failed to push some refs to
'https://rainers@github.com/rainers/dmd.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

I had this error before and pulled, so I guess it will bring me back to
square 1.

Last time I had the same issue I needed to git-reset something.

[alexrp]

When you rebase, you rewrite history, so you have to do a forced push, i.e.:

git push origin somebranch -f

[rainers]

Looks much better now. Thanks

[andralex]

(background: I'm doing a review of pull requests < 1000 as they tend to be controversial)

How about the option of doing it all on a flag, as Walter suggested? That may as well please everyone. I'm not sure I understand the best motivating use cases, but I presume those would go in the documentation.

[rainers]

An option to have this would be an improvement, but I don't see a reason why incomplete debug information can be desirable. Maybe, use the "complete" debug info with the standard "-g", but allow "-g2" to get back to the current behaveour in case you run into trouble.

[CyberShadow]

This pull request has been broken on win32 for essentially forever. It causes dmd to crash which on win32 pops up a dialog box, stalling the auto-tester. For now, I've disabled it from being tested (on every platform, sorry, quick hack). If/when it's fixed, let me know and I can remove the block.

Brad, not sure if this is still useful, but you can disable the crash dialog boxes on Windows. I've encountered the need for this when using DustMite:
https://github.com/CyberShadow/DustMite/wiki/Suppressing-Windows-crashes

Hope this helps.

[MartinNowak]

@braddr You should disable error reporting on test machines.
http://msdn.microsoft.com/en-us/library/bb513638.aspx

[MartinNowak]

I think that it could work, it was also my first idea about fixing the problem. But it has the drawback that you need to have the library compiled with debug information to be able to inspect data types declared in the library. For example, the phobos library in a standard installation comes without debug info, so you won't have any luck investigating non-templated structs or classes that are declared in phobos or druntime.

That's the expected behavior.

A better solution may be to have a module generated with only symdeb info for a class, and a single global defined. Then, other modules can reference that single global, thereby pulling in the symdeb data.

Sounds like a good idea. How would that work if something references undefined debug info?
AFAIK we couldn't use weak linkage because it does not drag in symbols from a lib.

[rainers]

That's the expected behavior.

I expect the behaviour of C/C++ where you can inspect a variable with struct type declared in some include file. Technically this is the same compilation unit as the actual source file, but this doesn't translate to D. What about di files that never get compiled to a library (C wrapper files in deimos)?

A better solution may be to have a module generated with only symdeb info for a class, and a single global defined. Then, other modules can reference that single global, thereby pulling in the symdeb data.

Sounds like a good idea. How would that work if something references undefined debug info?
AFAIK we couldn't use weak linkage because it does not drag in symbols from a lib.

Isn't the init data just that global symbol containing the debug info? It might contain data actually never used, though.

I'll try to rebase and enable the full debug info for referenced structs/classes with a flag -gf...

[MartinNowak]

Mmh, you don't even have initializers in Deimos, otherwise it would require linking.
Let's go with the -glevel then. The current behavior is -g1, the new one -g2 and the default for -g remains 1 for now.

[rainers]

Rebased and option added.
My first idea was to set the "debug verbosity level" in params.symdebug, but unfortunately this interferes with option -gc. So I went for a feature based flag in the global parameters (which is better design anyway and could be used for -gc aswell).

[andralex]

What's the status of this? I'll close for now, please reopen when ready. Thanks!

[MartinNowak]

It's a hack to write the debug information as a side effect of toCtype.
Also using toCtype as a mean to detect that something was referenced is somewhat flawed.
Furthermore writing the debug information only once will create incomplete object files with -c.

[rainers]

It's a hack to write the debug information as a side effect of toCtype.
Also using toCtype as a mean to detect that something was referenced is somewhat flawed.

I agree to some extend, it should be better done in the backend. I'll have a look if it fits in there somewhere.

Furthermore writing the debug information only once will create incomplete object files with -c.

You mean when writing multiple object files? Yes, that will be incomplete aswell with this patch, but not as incomplete as without the patch. Moving it into the backend might also solve this.

[alexrp]

(@andralex for future reference, avoid closing a pull request as a person with push rights unless you mean to permanently close it. It will prevent the contributor from reopening it.)

[MartinNowak]

You mean when writing multiple object files?

Yes, because toCtype is called only once per type per compile run. Thus when creating multiple objects
the one that actually defines a symbol might miss the corresponding debug info because it was emitted to an object file that references the symbol.

I agree to some extend, it should be better done in the backend.

Either there or in the glue layer. AFAIK there is no data in the compiler that easily tells you which types are used in which object file. So I don't know a good solution but toCtype won't work as is due to the reason mentioned above.

[andralex]

@alexrp ouch thanks for letting me know! I thought reopening is as simple as a button click regardless of who closed it.

[MartinNowak]

I thought reopening is as simple as a button click regardless of who closed it.

It still makes sense for abandoned pull request, it is as simple as writing a message to ask for reopening.

[andralex]

Fine, so should I continue to proactively close pull requests that need attention from their proponents?

[WalterBright]

In the closed pull request list, how can one tell which ones were not pulled?

[MartinNowak]

In the closed pull request list, how can one tell which ones were not pulled?

This should be a simple query, but the github API is awful.
https://gist.github.com/dawgfoto/5580693

3, 4, 5, 7, 8, 12, 14, 15, 17, 22, 26, 38, 57, 58, 62, 66, 70, 71, 74, 75, 84, 85, 88, 89, 91, 94, 97, 105, 109, 114, 115, 119, 120, 121, 123, 126, 135, 140, 141, 151, 155, 157, 159, 162, 173, 176, 177, 180, 181, 184, 188, 194, 198, 204, 207, 233, 234, 237, 240, 241, 242, 244, 248, 261, 263, 269, 277, 290, 296, 309, 318, 323, 339, 341, 342, 345, 347, 349, 351, 354, 357, 372, 377, 423, 426, 427, 448, 456, 457, 459, 461, 486, 489, 490, 495, 497, 501, 513, 520, 521, 523, 527, 545, 551, 559, 614, 632, 634, 638, 644, 646, 650, 653, 657, 660, 669, 674, 678, 682, 685, 687, 688, 696, 699, 701, 705, 714, 721, 726, 734, 738, 745, 760, 769, 770, 777, 786, 791, 793, 794, 843, 847, 861, 863, 868, 894, 902, 911, 928, 937, 941, 945, 950, 951, 952, 953, 955, 988, 1005, 1017, 1018, 1019, 1044, 1061, 1073, 1076, 1085, 1091, 1094, 1115, 1117, 1124, 1131, 1133, 1140, 1144, 1146, 1148, 1152, 1160, 1162, 1165, 1168, 1176, 1179, 1189, 1202, 1203, 1204, 1207, 1210, 1214, 1215, 1217, 1237, 1242, 1246, 1248, 1257, 1274, 1292, 1295, 1308, 1310, 1311, 1313, 1328, 1337, 1387, 1390, 1392, 1398, 1402, 1403, 1424, 1427, 1428, 1435, 1437, 1442, 1443, 1445, 1454, 1459, 1467, 1478, 1484, 1514, 1516, 1520, 1525, 1551, 1579, 1580, 1581, 1607, 1614, 1615, 1622, 1637, 1646, 1647, 1654, 1663, 1671, 1677, 1691, 1693, 1708, 1731, 1735, 1751, 1764, 1771, 1779, 1783, 1794, 1795, 1798, 1802, 1808, 1811, 1825, 1832, 1843, 1880, 1882, 1886, 1889, 1895, 1923, 1926, 1935, 1955, 1957, 1961, 1971, 1992, 2003, 2017, 2020

[ghost]

In the closed pull request list, how can one tell which ones were not pulled?

I've asked for this feature once, they said it would be a great idea if we could tell if a pull is closed or merged. But this was a few months ago, I don't know if they'll actually work on it..

[WalterBright]

Auto-merge toggled on

[yebblies]

Auto-merge toggled off

[rainers]

Updated and rebased this PR.

This is still an improvement, but I'm not sure this is the best possible solution. It still doesn't work well if the debug info is built into a library. The info is then linked to some random symbol (the first which referenced the type), but the code that is actually linked into the executable might not include this symbol.

Thus when creating multiple objects the one that actually defines a symbol might miss the corresponding debug info because it was emitted to an object file that references the symbol.

I don't think this PR can solve all the problems of incremental building to separate object files.

[MartinNowak]

A better solution may be to have a module generated with only symdeb info for a class, and a single global defined. Then, other modules can reference that single global, thereby pulling in the symdeb data.

Sounds reasonable.

I think that it could work, it was also my first idea about fixing the problem. But it has the drawback that you need to have the library compiled with debug information to be able to inspect data types declared in the library. For example, the phobos library in a standard installation comes without debug info, so you won't have any luck investigating non-templated structs or classes that are declared in phobos or druntime.

Can we handle this similarly to the new template instantiation logic.

[rainers]

A better solution may be to have a module generated with only symdeb info for a class, and a single global defined. Then, other modules can reference that single global, thereby pulling in the symdeb data.

Sounds reasonable.

I changed my mind over this, it will cause a lot of link errors when building against a library witthout debug symbols. The init symbol still exists in the release library, so no harm done apart from being unable to debug that part of the application.

Can we handle this similarly to the new template instantiation logic.

That is similar to assuming debug info is built with the init-symbol, that is what #4051 does.

[andralex]

This is badly bitrotten. Closing for now.

[rainers]

This is badly bitrotten.

Not so sure, I've updated it 5 month ago and the merge conflict is probably only minor. I think it still represents the best solution to the issue. But it doesn't seem to have much chance to get merged, so I don't insist on keeping it open.

[andralex]

Didn't mean to dismiss the work, just clean up the pr queue a bit. Please let me know what would be the best course of action.

[rainers]

No problem. #4051 seems to be better in line with what @WalterBright and @MartinNowak expect, but it also has drawbacks. Maybe a combination of these would be best.