Fix issue 20810: Freshly built DMD fails to read config file #11111

AndrejMitrovic · 2020-05-08T17:08:02Z

According to MSDN docs the return value of GetFullPathName does not count the terminating null character on a successful call.

See here: https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfullpathnamew

Note Although the return value in this case is a length that includes the terminating null character, the return value on success does not include the terminating null character in the count.

I'm not sure how this was never caught before though. But on my Win10 machine master fails.

Fixes https://issues.dlang.org/show_bug.cgi?id=20810

dlang-bot · 2020-05-08T17:08:04Z

Thanks for your pull request, @AndrejMitrovic!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	20810	normal	Freshly built DMD fails to read config file

Testing this PR locally

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

dub run digger -- build "master + dmd#11111"

src/dmd/root/filename.d

@@ -1005,10 +1005,13 @@ nothrow:
                auto fullPath = (cast(wchar*) mem.xmalloc_noscan((fullPathLength + 1) * wchar.sizeof))[0 .. fullPathLength + 1];
                scope(exit) mem.xfree(fullPath.ptr);

+                import core.stdc.stdio;


Geod24 · 2020-05-08T17:12:01Z

Needs a test tho

AndrejMitrovic · 2020-05-08T17:13:18Z

I'll see if I can add a test-case. It's coming from the call to usage(), so calling DMD with no params or probably --help. I'm guessing there's no such test in the test-suite.

AndrejMitrovic · 2020-05-08T17:30:27Z

I added a test, but I haven't been able to unittest on Windows locally yet. There's some issue with the build.d script:

rdmd build.d unittest

...

 dmd.obj : error LNK2001: unresolved external symbol _D3dmd7backend8bcomplex12__ModuleInfoZ
C:\dev\dmd\generated\windows\release\64\dmd.exe-unittest : fatal error LNK1120: 1 unresolved externals

I'll try and sort this out soon.

MoonlightSentinel · 2020-05-08T19:07:48Z

I added a test, but I haven't been able to unittest on Windows locally yet. There's some issue with the build.d script:

See #11112

AndrejMitrovic · 2020-05-09T06:23:23Z

It looks like the test-suite fails with the change. So it's not the appropriate fix.

I wonder if the bug is related to any recent Windows updates as I'm running v1909. But the only changes that are mentioned are for version 1607 in https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfullpathnamew.

MoonlightSentinel · 2020-05-09T10:03:10Z

test/dshell/test20810.d

+
+int main()
+{
+    run("$DMD");  // should print usage and exit with 0


dmd without arguments returns exit code 1...

Suggested change

run("$DMD"); // should print usage and exit with 0

run("$DMD --help"); // should print usage and exit with 0

MoonlightSentinel · 2020-05-09T10:04:00Z

You could also try a compilable test as defined below.

EDIT: I would rather add the compilable test because it's also a proper test for dmd --help

/*
REQUIRED_ARGS: --help
PERMUTE_ARGS:
TEST_OUTPUT:
----
DMD64 D Compiler $r:.*$
Copyright (C) 1999-$n$ by The D Language Foundation, All Rights Reserved written by Walter Bright

Documentation: https://dlang.org/
Config file: 
Usage:
  dmd [<option>...] <file>...
  dmd [<option>...] -run <file> [<arg>...]

Where:
  <file>           D source file
  <arg>            Argument to pass when running the resulting program

<option>:
  @<cmdfile>       read arguments from cmdfile
  -allinst          generate code for all template instantiations
  -betterC          omit generating some runtime information and helper functions
  -boundscheck=[on|safeonly|off]
                    bounds checks on, in @safe only, or off
  -c                compile only, do not link
  -check=[assert|bounds|in|invariant|out|switch][=[on|off]]
                    enable or disable specific checks
  -check=[h|help|?] list information on all available checks
  -checkaction=[D|C|halt|context]
                    behavior on assert/boundscheck/finalswitch failure
  -checkaction=[h|help|?]
                    list information on all available check actions
  -color            turn colored console output on
  -color=[on|off|auto]
                    force colored console output on or off, or only when not redirected (default)
  -conf=<filename>  use config file at filename
  -cov              do code coverage analysis
  -cov=<nnn>        require at least nnn% code coverage
  -D                generate documentation
  -Dd<directory>    write documentation file to directory
  -Df<filename>     write documentation file to filename
  -d                silently allow deprecated features and symbols
  -de               issue an error when deprecated features or symbols are used (halt compilation)
  -dw               issue a message when deprecated features or symbols are used (default)
  -debug            compile in debug code
  -debug=<level>    compile in debug code <= level
  -debug=<ident>    compile in debug code identified by ident
  -debuglib=<name>  set symbolic debug library to name
  -defaultlib=<name>
                    set default library to name
  -deps             print module dependencies (imports/file/version/debug/lib)
  -deps=<filename>  write module dependencies to filename (only imports)
  -extern-std=<standard>
                    set C++ name mangling compatibility with <standard>
  -extern-std=[h|help|?]
                    list all supported standards
  -fPIC             generate position independent code
  -g                add symbolic debug info
  -gf               emit debug info for all referenced types
  -gs               always emit stack frame
  -gx               add stack stomp code
  -H                generate 'header' file
  -Hd=<directory>   write 'header' file to directory
  -Hf=<filename>    write 'header' file to filename
  -HC               generate C++ 'header' file
  -HCd=<directory>  write C++ 'header' file to directory
  -HCf=<filename>   write C++ 'header' file to filename
  --help            print help and exit
  -I=<directory>    look for imports also in directory
  -i[=<pattern>]    include imported modules in the compilation
  -ignore           ignore unsupported pragmas
  -inline           do function inlining
  -J=<directory>    look for string imports also in directory
  -L=<linkerflag>   pass linkerflag to link
  -lib              generate library rather than object files
  -lowmem           enable garbage collection for the compiler
  -m32              generate 32 bit code
  -m64              generate 64 bit code
  -main             add default main() (e.g. for unittesting)
  -man              open web browser on manual page
  -map              generate linker .map file
  -mcpu=<id>        generate instructions for architecture identified by 'id'
  -mcpu=[h|help|?]  list all architecture options
  -mixin=<filename> expand and save mixins to file specified by <filename>
  -mv=<package.module>=<filespec>
                    use <filespec> as source file for <package.module>
  -noboundscheck    no array bounds checking (deprecated, use -boundscheck=off)
  -O                optimize
  -o-               do not write object file
  -od=<directory>   write object & library files to directory
  -of=<filename>    name output file to filename
  -op               preserve source path for output files
  -preview=<id>     enable an upcoming language change identified by 'id'
  -preview=[h|help|?]
                    list all upcoming language changes
  -profile          profile runtime performance of generated code
  -profile=gc       profile runtime allocations
  -release          compile release version
  -revert=<id>      revert language change identified by 'id'
  -revert=[h|help|?]
                    list all revertable language changes
  -run <srcfile>    compile, link, and run the program srcfile
  -shared           generate shared library (DLL)
  -transition=<id>  help with language change identified by 'id'
  -transition=[h|help|?]
                    list all language changes
  -unittest         compile in unit tests
  -v                verbose
  -vcolumns         print character (column) numbers in diagnostics
  -verror-style=[digitalmars|gnu]
                    set the style for file/line number annotations on compiler messages
  -verrors=<num>    limit the number of error messages (0 means unlimited)
  -verrors=context  show error messages with the context of the erroring source line
  -verrors=spec     show errors from speculative compiles such as __traits(compiles,...)
  --version         print compiler version and exit
  -version=<level>  compile in version code >= level
  -version=<ident>  compile in version code identified by ident
  -vgc              list all gc allocations including hidden ones
  -vtls             list all variables going into thread local storage
  -w                warnings as errors (compilation will halt)
  -wi               warnings as messages (compilation will continue)
  -X                generate JSON file
  -Xf=<filename>    write JSON file to filename
  -Xcc=<driverflag> pass driverflag to linker driver (cc)
----
*/

kinke · 2020-05-10T16:50:50Z

I'm not sure how this was never caught before though. But on my Win10 machine master fails.

I've just refactored this and misread the docs, that's why. And canonicalName() is either unused during the whole testsuite, or no tested DMD built with enabled assertions.

Anyway, I think a little unittest would be the best way to test this.

According to MSDN docs, the return value of GetFullPathName does *not* count the terminating null character. In the first call, it *does* include this count because a buffer was not specified in that initial call. Fix Issue 20810

AndrejMitrovic · 2020-05-12T07:27:34Z

I thought $r:.*$ would do substitution, but it doesn't.

And actually, I don't think this compilable test-case is correct. Because it's not testing the call properly. The sc.ini config path is empty here..

MoonlightSentinel · 2020-05-12T07:30:59Z

I thought $r:.*$ would do substitution, but it doesn't.

It does, the problem lies in the platform specific options (and a whitespace error due to markdown).

And actually, I don't think this compilable test-case is correct. Because it's not testing the call properly. The sc.ini config path is empty here..

Actually, you're right because the test runner invokes dmd with -conf. Adding a unittest as @kinke
suggested should do the job.

Sorry for the distraction.

AndrejMitrovic · 2020-05-12T07:37:35Z

Alright, I'll fix it up later.

Sorry for the distraction.

Not at all. Thanks for help from both of you!

ghost · 2020-05-31T08:27:04Z

test/compilable/test20810.d

@@ -0,0 +1,126 @@
+/*
+REQUIRED_ARGS: --help


don't use --help but pass a simple module containing an empty main func. The use of --help is, in addition of not testing the right thing, something that would have to be maintained over years.

don't use --help but pass a simple module containing an empty main func. The use of --help is, in addition of not testing the right thing.

Not an ideal test but running dmd --help triggers that bug.

something that would have to be maintained over years.

Only the current version, I'll re-enable a maintenance free version after this PR is merged.

A brittle integration test for this is overkill; a trivial little unittest would do the job perfectly, just like

dmd/src/dmd/root/filename.d

Lines 363 to 370 in 77ae162

unittest

{

version (Windows)

assert(combine("foo"[], "bar"[]) == "foo\\bar");

else

assert(combine("foo"[], "bar"[]) == "foo/bar");

assert(combine("foo/"[], "bar"[]) == "foo/bar");

}

Agreed, a unittest is the best solution here.

MoonlightSentinel · 2020-05-31T15:29:31Z

Any updates @AndrejMitrovic ?

AndrejMitrovic · 2020-05-31T23:04:41Z

Well it seems that master works with your test, so it looks like the issue might be somewhere else?

MoonlightSentinel · 2020-05-31T23:06:37Z

it fails with current master, the PR was accidentally merged by dlang-bot and got reverted.

AndrejMitrovic · 2020-06-17T12:32:30Z

I'll close this since I can't reproduce it anymore, sorry.

The assertion was wrong and guaranteed to fail when enabled (my bad). This is a reboot of dlang#11111. Also make sure to return a new root.rmem-managed memory allocation in all cases, i.e., not the malloc'd memory directly for the canonicalize_file_name version. E.g., the memory is released via mem.xfree() in FileName.safeSearchPath().

AndrejMitrovic commented May 8, 2020

View reviewed changes

src/dmd/root/filename.d Outdated

@@ -1005,10 +1005,13 @@ nothrow:

auto fullPath = (cast(wchar*) mem.xmalloc_noscan((fullPathLength + 1) * wchar.sizeof))[0 .. fullPathLength + 1];

scope(exit) mem.xfree(fullPath.ptr);

import core.stdc.stdio;

This comment was marked as resolved.

Sign in to view

AndrejMitrovic force-pushed the fix-sc-ini branch 2 times, most recently from d4cbadd to f5c37cc Compare May 8, 2020 17:11

dlang-bot added the Severity:Bug Fix label May 8, 2020

AndrejMitrovic force-pushed the fix-sc-ini branch from f5c37cc to a9857d7 Compare May 8, 2020 17:29

AndrejMitrovic added the Review:Needs Work label May 9, 2020

MoonlightSentinel reviewed May 9, 2020

View reviewed changes

AndrejMitrovic force-pushed the fix-sc-ini branch from a9857d7 to 5c1b8c9 Compare May 12, 2020 01:43

dlang-bot removed the Review:Needs Work label May 12, 2020

Fix getting canonical path in GetFullPathNameW

77ae162

According to MSDN docs, the return value of GetFullPathName does *not* count the terminating null character. In the first call, it *does* include this count because a buffer was not specified in that initial call. Fix Issue 20810

AndrejMitrovic force-pushed the fix-sc-ini branch from 5c1b8c9 to 77ae162 Compare May 12, 2020 01:44

ghost suggested changes May 31, 2020

View reviewed changes

AndrejMitrovic closed this Jun 17, 2020

kinke mentioned this pull request Jun 18, 2020

dmd.root.filename: Fix FileName.canonicalPath() regression on Windows #11293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue 20810: Freshly built DMD fails to read config file #11111

Fix issue 20810: Freshly built DMD fails to read config file #11111

AndrejMitrovic commented May 8, 2020

dlang-bot commented May 8, 2020 •

edited

Loading

This comment was marked as resolved.

Geod24 commented May 8, 2020

AndrejMitrovic commented May 8, 2020

AndrejMitrovic commented May 8, 2020

MoonlightSentinel commented May 8, 2020

AndrejMitrovic commented May 9, 2020 •

edited

Loading

MoonlightSentinel May 9, 2020

MoonlightSentinel commented May 9, 2020 •

edited

Loading

kinke commented May 10, 2020

AndrejMitrovic commented May 12, 2020

MoonlightSentinel commented May 12, 2020

AndrejMitrovic commented May 12, 2020

ghost May 31, 2020

MoonlightSentinel May 31, 2020

kinke May 31, 2020

MoonlightSentinel May 31, 2020

MoonlightSentinel commented May 31, 2020

AndrejMitrovic commented May 31, 2020

MoonlightSentinel commented May 31, 2020

AndrejMitrovic commented Jun 17, 2020

	run("$DMD"); // should print usage and exit with 0
	run("$DMD --help"); // should print usage and exit with 0

	unittest
	{
	version (Windows)
	assert(combine("foo"[], "bar"[]) == "foo\\bar");
	else
	assert(combine("foo"[], "bar"[]) == "foo/bar");
	assert(combine("foo/"[], "bar"[]) == "foo/bar");
	}

Fix issue 20810: Freshly built DMD fails to read config file #11111

Fix issue 20810: Freshly built DMD fails to read config file #11111

Conversation

AndrejMitrovic commented May 8, 2020

dlang-bot commented May 8, 2020 • edited Loading

Bugzilla references

Testing this PR locally

This comment was marked as resolved.

Geod24 commented May 8, 2020

AndrejMitrovic commented May 8, 2020

AndrejMitrovic commented May 8, 2020

MoonlightSentinel commented May 8, 2020

AndrejMitrovic commented May 9, 2020 • edited Loading

MoonlightSentinel May 9, 2020

Choose a reason for hiding this comment

MoonlightSentinel commented May 9, 2020 • edited Loading

kinke commented May 10, 2020

AndrejMitrovic commented May 12, 2020

MoonlightSentinel commented May 12, 2020

AndrejMitrovic commented May 12, 2020

ghost May 31, 2020

Choose a reason for hiding this comment

MoonlightSentinel May 31, 2020

Choose a reason for hiding this comment

kinke May 31, 2020

Choose a reason for hiding this comment

MoonlightSentinel May 31, 2020

Choose a reason for hiding this comment

MoonlightSentinel commented May 31, 2020

AndrejMitrovic commented May 31, 2020

MoonlightSentinel commented May 31, 2020

AndrejMitrovic commented Jun 17, 2020

dlang-bot commented May 8, 2020 •

edited

Loading

AndrejMitrovic commented May 9, 2020 •

edited

Loading

MoonlightSentinel commented May 9, 2020 •

edited

Loading