Fix issues 1870 and 12790: Write out string mixin code to a file for debugging purpose #8677

thewilsonator · 2018-09-09T07:25:12Z

There is no test case for this yet, I'm not sure how to check the contents of generated files with dmd's test suite. I also don't know how to test debug info, any help appreciated.

cc @TurkeyMan

dlang-bot · 2018-09-09T07:25:16Z

Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	1870	enhancement	Reproduce offending lines in error messages for string mixins
✓	12790	enhancement	Compiler should keep mixin file around for debugging purposes

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8677"

ghost · 2018-09-09T08:08:37Z

doesn't the source file mars.d need to be updated to activate the new option ?

thewilsonator · 2018-09-09T09:44:28Z

True I forgot to sync before opening the PR

src/dmd/cli.d

RazvanN7 · 2018-09-09T12:48:50Z

Does it really need to be a compiler switch? Why not do this by default whenever mixins are used? @WalterBright is usually against adding new switches. How hard is it to simply print the offending line without any line number?

Also, if this gets merged it needs a changelog entry, updating the documentation and also the messages that dmd outputs if no parameter is provided.

rainers · 2018-09-09T14:18:13Z

There is also https://issues.dlang.org/show_bug.cgi?id=5051. My proposed patch writes mixins to a single file, otherwise your object directory might be filled with an ever increasing number of small files.

Visual D registers .mixin files with the D language service, so you get syntax highlighting and "auto" watches. I have not tested this since 2010, though.

I also don't know how to test debug info, any help appreciated.

For windows, there is testpdb.d with some examples, e.g. it should be possible to enumerate line number information for a function.

jacob-carlborg · 2018-09-09T15:23:09Z

I'm not sure how to check the contents of generated files with dmd's test suite

A test can consist of a shell script, there are existing examples. Perhaps look at a test for Ddoc.

jacob-carlborg · 2018-09-09T15:44:35Z

src/dmd/globals.d

@@ -151,6 +151,7 @@ struct Param
                            // https://issues.dlang.org/show_bug.cgi?id=16997
    bool vsafe;             // use enhanced @safe checking
    bool ehnogc;            // use @nogc exception handling
+    bool debugMixins;       // write out argument to mixin so that the debuuger has source


Ddoc comment please.

None of the others do, also there is cli.d

jacob-carlborg · 2018-09-09T15:47:59Z

src/dmd/parse.d

-            char* filename = cast(char*)mem.xmalloc(strlen(loc.filename) + 7 + (loc.linnum).sizeof * 3 + 1);
-            sprintf(filename, "%s-mixin-%d", loc.filename, cast(int)loc.linnum);
-            scanloc.filename = filename;
+            auto len = strlen(loc.filename) + 7 + (loc.linnum).sizeof * 3 + 1;


Looks like this can be const.

jacob-carlborg · 2018-09-09T15:49:24Z

src/dmd/parse.d

+
+            if (global.params.debugMixins)
+            {
+                auto tmp = FileName.combine(global.params.objdir,filename.ptr);


Looks like this can be const.

dhasenan · 2018-09-09T18:51:34Z

src/dmd/parse.d

+            {
+                auto tmp = FileName.combine(global.params.objdir,filename.ptr);
+                filename = cast(char[])tmp[0 .. strlen(tmp)];
+                auto f = File.create(filename.ptr);


This is certainly better than the status quo.

However, consider the following code:

void foo(string s)() { mixin(s); } void main() { foo!(`writeln(a);`); foo!(`writeln("hello world");`); }

This will write writeln(a); to file scratch.d-mixin-5, then write writeln("hello world"); to the same file, overwriting what was there previously.

Is that likely to ever occur in real code?

Any mixin that occurs inside a template that's instantiated more than once. That might be a minority of cases, but it certainly does occur. For instance, std.typecons has at least a handful of these. I wrote some code to automatically extract a postgresql row into a struct, and it contains mixins inside templates that are instantiated multiple times.

Also the standard shortcut for implementing opBinary is with mixins. That's why we switched to opBinary!operator, instead of opAdd and friends.

Is that likely to ever occur in real code?

I think it happens more often than not.
In my experience, most string mixins are wrapped in a mixin template for sanitary reasons, or in a template function. If the enclosing scope is not a template, then there's little motivation for a string mixin... you could just put the code directly. I reckon it's the common case for a mixin to appear in a parametric environment.

WalterBright

When creating PRs for bugzilla issues, please include a link in the issue to the PR. This prevents multiple people from unwittingly working on the same issue. And even if it is rejected, it provides valuable information to anyone trying again.

I'm tired of repeatedly pointing this out to everyone making PRs.

thewilsonator · 2018-09-09T23:02:23Z

@RazvanN7 This will result in a lot of unnecessary I/O for projects with lots of mixins, not to mention disk space. This is also so that the debugger has source so it needs to be on disk for that.

Also, if this gets merged it needs a changelog entry,

~~Will do.~~ Done.

updating the documentation

isn't that what cli.d is for?

the messages that dmd outputs if no parameter is provided.

This is a parameterless switch.

Geod24 · 2018-09-10T01:25:55Z

src/dmd/parse.d

+
+            if (global.params.debugMixins)
+            {
+                auto tmp = FileName.combine(global.params.objdir,filename.ptr);


const tmp = FileName.combine(global.params.objdir.toDString(),filename);

Will give you a slice.

Geod24 · 2018-09-10T01:28:50Z

When submitting a PR, please provide a title which allow us to see what it's about from the notification.

Also, is there no better way to provide this to GDB than writing it to a file ? @ibuclaw ?

thewilsonator · 2018-09-10T01:35:51Z

As I understand it, the problem is that source locations are provided to the debugger (GDB/LLDB/whatever MS uses) but the source doesn't exist on disk, because its expanded at compile time, and so b somewhereinamixin will work, but show me the source where I am fails.

Geod24 · 2018-09-10T01:38:17Z

I understand the problem, and agree that it needs fixing, I'm just wondering if debuggers have another way to cope with this than writing a file to disk.
And if we do end up writing a file to disk, I think it should live alongside objects file.

thewilsonator · 2018-09-10T01:40:06Z

Can you embed source in an object file? I can't think of any other way to do it.

And if we do end up writing a file to disk, I think it should live alongside objects file.

Hence this is written toobjdir.

Geod24 · 2018-09-10T02:00:47Z

src/dmd/parse.d

-            sprintf(filename, "%s-mixin-%d", loc.filename, cast(int)loc.linnum);
-            scanloc.filename = filename;
+            auto len = strlen(loc.filename) + 7 + (loc.linnum).sizeof * 3 + 1;
+            char[] filename = cast(char[])mem.xmalloc(len)[0 .. len];


Cast to a pointer then slice. Casting arrays incur additional checks.

Geod24 · 2018-09-10T02:02:18Z

src/dmd/parse.d

+                filename = cast(char[])tmp[0 .. strlen(tmp)];
+                auto f = File.create(filename.ptr);
+                // We're going to write it out straight away so the cast below is fine
+                f.setbuffer(cast(void*)input.ptr,input.length);


Nit: space after comma (other places in the diff as well)

Geod24 · 2018-09-10T02:05:50Z

src/dmd/parse.d

+            {
+                auto tmp = FileName.combine(global.params.objdir,filename.ptr);
+                filename = cast(char[])tmp[0 .. strlen(tmp)];
+                auto f = File.create(filename.ptr);


Use new File(filename.ptr). create is a C++ wrapper.

Actually doesn't need to even be new'd

thewilsonator · 2018-09-10T10:45:10Z

Addressed all comments except doc comment for global.params.debugMixins and collision between mixing in a template parameter. Added changeling and tests for 1879/12790. Haven't added a test for debug info generation

wilzbach

Would be nice to avoid creating temporary files that aren't in test_results

wilzbach

Would be nice to avoid creating temporary files that aren't in test_results

wilzbach

Would be nice to avoid creating temporary files that aren't in test_results

PetarKirov · 2018-10-23T07:33:35Z

@ZombineDev It would effectively be under whatever licence LLVM uses

@thewilsonator I didn't mean to port the source code to D, but to implement the features you miss in the existing test suite code.

thewilsonator · 2018-10-31T08:06:49Z

Rebased. Fixed style.

No "mixin stack" yet. While nice, I'm not (yet) convinced that it will be necessary, and that can be done later if it is.

thewilsonator · 2018-10-31T22:38:07Z

All green except semaphore which is segfaulting. @rainers any idea why?

rainers · 2018-11-01T09:02:11Z

All green except semaphore which is segfaulting. @rainers any idea why?

Not sure, an earlier build reports "double free or corruption".

Maybe it has to with flushMixin doing something unexpected when executed by atexit.

rainers · 2018-11-01T09:06:48Z

No "mixin stack" yet. While nice, I'm not (yet) convinced that it will be necessary, and that can be done later if it is.

Ok, being opt-in should not block it for dmd to progress.

I'd love to enable this feature in Visual D by default, but not being able to easily jump to the actual source of an error is probably a deal breaker.

rainers · 2018-11-01T09:11:12Z

src/dmd/globals.d

+    OutBuffer* mixinOut;                // write expanded mixins for debugging
+    const(char)* mixinFile;             // .mixin file output name
+    int mixinLines;                     // Number of lines in writeMixins
+


mixinOut and mixinLines should better go into the Global struct rather than Param. I guess you followed the moduleDeps example, but I'd consider it a bad one.

rainers · 2018-11-01T09:13:01Z

src/dmd/mars.d

@@ -1508,6 +1525,21 @@ bool parseCommandLine(const ref Strings arguments, const size_t argc, ref Param
            params.map = true;
        else if (arg == "-multiobj")
            params.multiobj = true;
+        else if (startsWith(p + 1, "mixin="))
+        {
+            if (params.mixinFile)


This follows the -deps example, too, but no other option disallows overriding previous settings.

rainers · 2018-11-01T09:15:45Z

src/dmd/parse.d

@@ -242,7 +279,7 @@ final class Parser(AST) : Lexer
        //printf("Parser::Parser()\n");
        scanloc = loc;

-        if (loc.filename)
+        if (!writeMixin(input, scanloc) && loc.filename)


Not too happy about writeMixin being in the parse module. Maybe it would be better to do this (and the existing pseudo-filename creation) at the callsite with a small helper function.

I tried that originally, but that constructor is called from MixinStatement and MixinDeclaraion and I didn't really want to duplicate the code.

thewilsonator · 2018-11-01T10:55:33Z

Not sure, an earlier build reports "double free or corruption".

Aha. I fix the old problem with main's argv parameter no longer being valid in atexit, so I guess params.mixinOut being new'd means the GC might be getting to it?

thewilsonator · 2018-11-01T13:20:40Z

@rainers any idea why testing of --DRT-trapExceptions=0 has started failing?

#4  0x00000000004276f1 in D main ()
...
grep "in D main (args=...) at src/rt_trap_exceptions_drt.d:9" > /dev/null < ./generated/linux/release/64/rt_trap_exceptions_drt_gdb.output
make[2]: *** [generated/linux/release/64/rt_trap_exceptions_drt_gdb.done] Error 1

or why (args=...) has disappeared?

thewilsonator · 2018-11-04T02:17:40Z

Unrelated adding 72h -> merge

…debugging purpose

dlang-bot added the Enhancement label Sep 9, 2018

thewilsonator force-pushed the mixin-debug branch from a684d9e to 48dd2a0 Compare September 9, 2018 09:44

thewilsonator requested review from Geod24, MartinNowak, rainers and WalterBright as code owners September 9, 2018 09:44

drug007 reviewed Sep 9, 2018

View reviewed changes

src/dmd/cli.d Outdated Show resolved Hide resolved

jacob-carlborg reviewed Sep 9, 2018

View reviewed changes

dhasenan reviewed Sep 9, 2018

View reviewed changes

WalterBright requested changes Sep 9, 2018

View reviewed changes

Geod24 reviewed Sep 10, 2018

View reviewed changes

thewilsonator force-pushed the mixin-debug branch from 48dd2a0 to 03de9e7 Compare September 10, 2018 10:42

thewilsonator force-pushed the mixin-debug branch from 03de9e7 to 78ccc41 Compare September 10, 2018 10:48

thewilsonator requested a review from RazvanN7 as a code owner September 10, 2018 10:48

wilzbach reviewed Oct 22, 2018

View reviewed changes

thewilsonator force-pushed the mixin-debug branch from d40f01e to ae8a447 Compare October 31, 2018 07:48

thewilsonator force-pushed the mixin-debug branch from ae8a447 to 2990f60 Compare October 31, 2018 12:11

rainers reviewed Nov 1, 2018

View reviewed changes

thewilsonator force-pushed the mixin-debug branch 2 times, most recently from 4e34ddc to 5038008 Compare November 1, 2018 12:17

thewilsonator added 72h no objection -> merge The PR will be merged if there are no objections raised. auto-merge and removed 72h no objection -> merge The PR will be merged if there are no objections raised. labels Nov 4, 2018

thewilsonator self-assigned this Nov 7, 2018

Fix issues 1870 and 12790: Write out string mixin code to a file for …

3bbb228

…debugging purpose

thewilsonator force-pushed the mixin-debug branch from 5038008 to 3bbb228 Compare November 7, 2018 00:16

dlang-bot removed the auto-merge label Nov 7, 2018

thewilsonator added the auto-merge label Nov 7, 2018

thewilsonator merged commit 34db741 into dlang:master Nov 7, 2018

thewilsonator mentioned this pull request Nov 14, 2018

Musl fails due to std.regex import ldc-developers/ldc#2911

Closed

thewilsonator mentioned this pull request Jan 11, 2019

testsuite: Prepare for stricter TEST_OUTPUT tests #9230

Merged

thewilsonator deleted the mixin-debug branch April 30, 2022 23:38

Fix issues 1870 and 12790: Write out string mixin code to a file for debugging purpose #8677

Fix issues 1870 and 12790: Write out string mixin code to a file for debugging purpose #8677

Conversation

thewilsonator commented Sep 9, 2018

dlang-bot commented Sep 9, 2018

Bugzilla references

Testing this PR locally

ghost commented Sep 9, 2018

thewilsonator commented Sep 9, 2018

RazvanN7 commented Sep 9, 2018 • edited Loading

rainers commented Sep 9, 2018

jacob-carlborg commented Sep 9, 2018 • edited Loading

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

dhasenan Sep 9, 2018 • edited Loading

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

WalterBright left a comment

Choose a reason for hiding this comment

thewilsonator commented Sep 9, 2018 • edited Loading

Choose a reason for hiding this comment

Geod24 commented Sep 10, 2018

thewilsonator commented Sep 10, 2018

Geod24 commented Sep 10, 2018

thewilsonator commented Sep 10, 2018 • edited Loading

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

thewilsonator commented Sep 10, 2018

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

wilzbach left a comment

Choose a reason for hiding this comment

PetarKirov commented Oct 23, 2018 • edited Loading

thewilsonator commented Oct 31, 2018

thewilsonator commented Oct 31, 2018

rainers commented Nov 1, 2018

rainers commented Nov 1, 2018

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

Choose a reason for hiding this comment

thewilsonator commented Nov 1, 2018

thewilsonator commented Nov 1, 2018

thewilsonator commented Nov 4, 2018

RazvanN7 commented Sep 9, 2018 •

edited

Loading

jacob-carlborg commented Sep 9, 2018 •

edited

Loading

dhasenan Sep 9, 2018 •

edited

Loading

thewilsonator commented Sep 9, 2018 •

edited

Loading

thewilsonator commented Sep 10, 2018 •

edited

Loading

PetarKirov commented Oct 23, 2018 •

edited

Loading