Print DFLAGS on -v to ease debugging by wilzbach · Pull Request #7865 · dlang/dmd

wilzbach · 2018-02-11T01:58:38Z

Motivation: it can be tricky to figure out what dflags DMD has read. This helps debugging at almost no cost (and -v is only used for debugging anyhow).

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach · 2018-02-11T02:01:33Z

test/compilable/testclidflags.sh

+# Force DMD to print the -v menu by passing an invalid object file
+# It will fail with "no object files to link", but print the log
+( ! "$DMD" -v foo.a 2> /dev/null ) | grep -q "dflags    (none)"
+( ! DFLAGS="-O -D" "$DMD" -v foo.a 2> /dev/null ) | grep -q "dflags    -O -D"


Imho it sucks that we have to workaround DMD so hard just to get it's config output. Shouldn't we make DMD's raw config settings easier to access? Most of this will be solved by the improved Json output, but there are still many cases where just looking what flags, args and precompiled versions DMD's as helps.
Ideas:

default for -v (instead of showing the help page)

--version=detailed

JinShil · 2018-02-11T02:02:58Z

src/dmd/mars.d

        message("binary    %s", global.params.argv0);
        message("version   %s", global._version);
        message("config    %s", global.inifilename ? global.inifilename : "(none)");
+        OutBuffer buf;


I recommend adding a header comment like "Print DFLAGS environment variable" for this block of code.

JinShil · 2018-02-11T02:05:47Z

src/dmd/mars.d

+        foreach (flag; dflags.asDArray)
+            buf.printf("%s ", flag);
+        auto res = buf.peekSlice() ? buf.peekSlice()[0 .. $ - 1] : "(none)";
+        message("dflags    %.*s", res.length, res.ptr);


Please use "DFLAGS" in all caps.

I used the existing convention of lower-casing everything - the entire output of -v starts with lower-cases tags.

Environment variables are case-sensitive (at least on Linux). Therefore, I think the case is important here.

What do other people think? (I don't mind changing it, but for now I focus on getting it to pass on auto-tester, so I won't change it immediately)

doesn't matter to me, both have small pros/cons. Lower case is consistent with all other verbose output, CAPITAL makes it consistent with the actual variable name.

If it's helpful, rdmd's regex for verbose output is here:

auto pattern = ctRegex!(r"^(import|file|binary|config|library)\s+([^$]+)\(?([^$]*)\)?\s*$");

marler8997 · 2018-02-11T04:59:22Z

src/dmd/mars.d

        message("version   %s", global._version);
        message("config    %s", global.inifilename ? global.inifilename : "(none)");
+        // Print DFLAGS environment variable
+        OutBuffer buf;


Should buf and res go in their own scope?

Actually I didn't realize this was inside such a small if block...nevermind.

marler8997 · 2018-02-11T05:24:29Z

src/dmd/mars.d

+        // Print DFLAGS environment variable
+        OutBuffer buf;
+        foreach (flag; dflags.asDArray)
+            buf.printf("%s ", flag);


I think individual dflags may contain spaces. If so, we may want to surround them with quotes when we see that they contain whitespace.

Maybe something similar to writeFilename in link.d.

So you want me to detect whether there's a space in the flag and then use quotes for it?
Most flags don't use spaces and it would look pretty ugly if this is done by default.

it would look pretty ugly if this is done by default

shouldn't be by default, just if there's space:

DFLAGS=-I"path to foo" -Ibar
=>
'-Ipath to foo' -Ibar
so only '' where needed;

also, using
-Ipath\ to\ foo -Ibar
is another option

Actually, a better solution might just be to print the original unprocessed value of DFLAGS because that one will already be quoted properly

wilzbach · 2018-02-11T20:07:14Z

Okay let's make a vote:

👍 - dflags
👎 - DFLAGS
❤️ - I don't care, just get this merged
😕 What are you talking about?

timotheecour · 2018-02-11T20:20:08Z

why not add it to json output as well?

wilzbach · 2018-02-11T20:22:28Z

Yes of course, but we are currently blocked on Walter for the JSON output: #7838

wilzbach · 2018-02-13T14:42:17Z

Okay let's make a vote:

Looks like DFLAGS is more popular. Changed accordingly.

shouldn't be by default, just if there's space:

Okay done.

Actually, a better solution might just be to print the original unprocessed value of DFLAGS because that one will already be quoted properly

Not sure that's a good idea. There might potentially be some bugs or other problems with the processing of DFLAGS, so in my experience it's better to dump the actual value the program uses.

rainers · 2018-02-28T10:30:43Z

test/compilable/testclidflags.sh

+# On OSX DMD terminates with a successful exit code, so `|| true` is used.
+( "$DMD" -v foo.d 2> /dev/null || true) | grep -q "DFLAGS    (none)"
+( DFLAGS="-O -D" "$DMD" -v foo.d 2> /dev/null || true) | grep -q "DFLAGS    -O -D"
+( DFLAGS="-O '-Ifoo bar' -c" "$DMD" -v foo.d 2> /dev/null || true) | grep -q "DFLAGS    -O '-Ifoo bar' -c"


I wonder why this works on the auto tester. For me, this test fails without any helpful output and if I run it explicitly it seems DFLAGS is picked up as a test parameter and d_do_test complains:

object.Exception@d_do_test.d(584): The DFLAGS test argument must be empty: It is ' ="-O -D" "$DMD" -v foo.d 2> /dev/null || true) | grep -q "DFLAGS -O -D" ="-O '-Ifoo bar' -c" "$DMD" -v foo.d 2> /dev/null || true) | grep -q "DFLAGS -O '-Ifoo bar' -c"'

You running Windows?

Yes, but picking up DFLAGS should be OS platfrom-agnostic.

Maybe something to do with the binutils or bash? What's the actual output of dmd for you?

It's not dmd complaining, but d_do_test because findTestParameter(envData, file, "DFLAGS", testArgs.dflags); results in the shown string.

Ouch, totally missed that shell scripts are (no longer?) executed through d_do_test.
There is zero output from the script itself, will have to investigate further...

We just merged a change which adds sh_do_test.sh that automatically redirects output from all BASH tests to the test output file, i.e. test_results/compilable/mytest.sh.out

Found the issue: dmd still used an sc.ini which overrides DFLAGS. Better to pass -conf= to dmd in these tests, too?

wilzbach requested review from MartinNowak, WalterBright, mathias-lang-sociomantic and rainers as code owners February 11, 2018 01:58

wilzbach commented Feb 11, 2018

View reviewed changes

wilzbach mentioned this pull request Feb 11, 2018

Add ability to override DFLAGS env variable in test suite #7845

Merged

JinShil suggested changes Feb 11, 2018

View reviewed changes

wilzbach force-pushed the v-debug branch 2 times, most recently from 9872cc7 to a158a35 Compare February 11, 2018 02:35

marler8997 reviewed Feb 11, 2018

View reviewed changes

wilzbach force-pushed the v-debug branch from a158a35 to 02b2d0d Compare February 13, 2018 14:36

Print DFLAGS on -v to ease debugging

acd990a

wilzbach force-pushed the v-debug branch from 02b2d0d to acd990a Compare February 13, 2018 14:40

JinShil approved these changes Feb 15, 2018

View reviewed changes

wilzbach mentioned this pull request Feb 15, 2018

Add more JSON #7894

Merged

JinShil added the Merge:auto-merge label Feb 16, 2018

dlang-bot merged commit 998841a into dlang:master Feb 16, 2018

JinShil mentioned this pull request Feb 21, 2018

Add [Environment{arch}] DFLAGS to -v output #7937

Merged

rainers reviewed Feb 28, 2018

View reviewed changes

wilzbach deleted the v-debug branch March 13, 2018 17:28

wilzbach mentioned this pull request Mar 29, 2018

Fix Issue 18433 - rdmd doesn't respect DFLAGS for its cache hash dlang/tools#343

Open

Uh oh!

Comments

Conversation

wilzbach commented Feb 11, 2018

Uh oh!

dlang-bot commented Feb 11, 2018

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timotheecour Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timotheecour commented Feb 11, 2018

Uh oh!

wilzbach commented Feb 11, 2018

Uh oh!

wilzbach commented Feb 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marler8997 Feb 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

marler8997 Feb 11, 2018 •

edited

Loading

marler8997 Feb 11, 2018 •

edited

Loading

timotheecour Feb 11, 2018 •

edited

Loading

wilzbach commented Feb 11, 2018 •

edited

Loading

marler8997 Feb 28, 2018 •

edited

Loading