autodetect Visual Studio and linker options by rainers · Pull Request #7393 · dlang/dmd

rainers · 2017-12-03T18:45:29Z

This combines detection of latest VS and Windows SDK from Visual D and the DMD installer. All that is needed specific for -m64/-m32mscoff in sc.ini is

[Environment64]
LIB=%@P%\..\lib64;%LIB%
DFLAGS=%DFLAGS% -L/OPT:NOICF

[Environment32mscoff]
LIB=%@P%\..\lib32mscoff;%LIB%

Releated bugzilla issues:

https://issues.dlang.org/show_bug.cgi?id=16688
https://issues.dlang.org/show_bug.cgi?id=17999
https://issues.dlang.org/show_bug.cgi?id=14849

dlang-bot · 2017-12-03T18:45:29Z

Thanks for your pull request, @rainers!

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.

WalterBright · 2017-12-04T02:17:35Z

src/ddmd/link.d

+            if(!key)
+                return def;
+
+            char[260] buf;


append = void

WalterBright · 2017-12-04T02:18:51Z

src/ddmd/link.d

+            }
+        }
+
+        void Create(HKEY root, wstring keyname, bool x64hive = false)


I generally dislike using default arguments unless doing it for backwards compatibility. Since this is new code, please use explicit arguments.

WalterBright · 2017-12-04T02:19:08Z

src/ddmd/link.d

+            HRESULT hr;
+        }
+
+        const(char)* GetString(const(char)* szName, const(char)* def = null)


no default args

WalterBright · 2017-12-04T02:19:57Z

src/ddmd/link.d

+                return def;
+
+            char[260] buf;
+            DWORD cnt = 260 * char.sizeof;


replace 260 with buf.length

WalterBright · 2017-12-04T02:27:12Z

src/ddmd/link.d

+                if (fileinfo.cFileName[0] >= '1' && fileinfo.cFileName[0] <= '9')
+                    if (res is null || strcmp(res, fileinfo.cFileName.ptr) < 0)
+                        if (FileName.exists(FileName.combine(FileName.combine(baseDir, fileinfo.cFileName.ptr), testfile)))
+                            res = strdup(fileinfo.cFileName.ptr);


being in a loop, the res seems to be leaking memory upon reassignment

WalterBright · 2017-12-04T02:27:37Z

src/ddmd/link.d

+            while(FindNextFileA(h, &fileinfo));
+            FindClose(h);
+
+            return res;


returning malloced memory should be documented

WalterBright · 2017-12-04T02:27:53Z

src/ddmd/link.d

+                            res = strdup(fileinfo.cFileName.ptr);
+            }
+            while(FindNextFileA(h, &fileinfo));
+            FindClose(h);


Return value is ignored - should check it for error

Pretty unlikely to fail, and even if it does, should it abort a build?

WalterBright · 2017-12-04T02:28:48Z

src/ddmd/link.d

+
+        const(char)* GetString(const(char)* szName, const(char)* def = null)
+        {
+            if(!key)


Sometimes this PR uses if( and sometimes if (. Please use the latter style.

WalterBright · 2017-12-04T02:29:39Z

src/ddmd/link.d

+            {
+                if (!x64hive)
+                {
+                    size_t len = strlen(szRegPath);


declare as const

WalterBright · 2017-12-04T02:31:20Z

src/ddmd/link.d

+                    assert(len > 9 && szRegPath[0..9] == r"SOFTWARE\");
+                    char* npath = cast(char*)mem.xmalloc(len + 13);
+                    strcpy(npath, r"SOFTWARE\WOW6432Node\");
+                    strcpy(npath + 21, szRegPath + 9);


Since in both calls to strcpy the lengths are known, use memcpy instead.

WalterBright · 2017-12-04T02:31:58Z

src/ddmd/link.d

+    {
+        pragma(lib, "advapi32.lib");
+
+        this(HKEY root, const(char)* szRegPath, bool x64hive = false)


no default arguments

WalterBright · 2017-12-04T02:33:16Z

src/ddmd/link.d

+        {
+            if(key)
+            {
+                RegCloseKey(key);


Check for error return

WalterBright · 2017-12-04T02:34:12Z

src/ddmd/link.d

+
+        void Create(HKEY root, wstring keyname, bool x64hive = false)
+        {
+            HRESULT hr;


Declaration that is unused?

WalterBright · 2017-12-04T02:36:25Z

src/ddmd/link.d

+        const(char)* p;
+        alias p this;
+
+        PathBuilder opDiv(const(char)* name)


Please don't do non-arithmetic operator overloading.

WalterBright · 2017-12-04T02:39:15Z

src/ddmd/link.d

+
+        PathBuilder opDiv(const(char)* name)
+        {
+            if (!name)


The uses I see of PathBuilder suggest that name is never null, so this struct is kinda pointless. In fact, Filename.combine should do the right thing with a null second argument anyway. (I checked and it does not, so that should be fixed there, not adding a wrapper here.)

WalterBright · 2017-12-04T02:44:09Z

src/ddmd/link.d

+                auto sdk = PathBuilder(WindowsSdkDir) / "lib";
+                if(FileName.exists(sdk / WindowsSdkVersion / "um" / arch / "kernel32.lib")) // SDK 10.0
+                    return sdk / WindowsSdkVersion / "um" / arch;
+                else if(FileName.exists(sdk / r"win8\um" / arch / "kernel32.lib")) // SDK 8.0


I'd rather see a / b / c added as a function to root.filename, so it can be used elsewhere. It should use variadic parameters to list them, or an array literal, or even 2,3,4 parameter overloads.

I introduced the PathBuilder struct because FileName.combine cannot be used with UFCS, but creates unreadable code if nesting multiple calls.
Using variadic parameters instead is almost as good, though, so I replaced PathBuilder with FileName.buildPath now.

WalterBright · 2017-12-04T02:47:46Z

src/ddmd/link.d

+                {
+                    // debug info needs DLLs Common7\IDE for most linker versions
+                    const char* path = getenv("PATH");
+                    const char* npath = FileName.combine(VSInstallDir, r"Common7\IDE");


path and npath appear to never be used?

Ooops, missing PATH modification.

WalterBright · 2017-12-04T02:49:17Z

src/ddmd/link.d

+        const(char)* VCInstallDir;
+        const(char)* VCToolsInstallDir; // used by VS 2017
+
+        void initialize()


In general, all functions need Ddoc comments.

rainers · 2017-12-04T23:56:55Z

@WalterBright Thanks for review, I've addressed most comments.

WalterBright · 2017-12-05T00:57:33Z

src/ddmd/link.d

+                        }
+            }
+            while(FindNextFileA(h, &fileinfo));
+            FindClose(h);


FindClose() returns 0 on failure. Please check it. https://msdn.microsoft.com/en-us/library/windows/desktop/aa364413(v=vs.85).aspx

I know, but it feels a bit harsh to fail while a sensible result is already obtained. I've changed it nevertheless.

WalterBright · 2017-12-05T00:59:13Z

src/ddmd/link.d

+        {
+            enum x64hive = false; // VS registry entries always in 32-bit hive
+
+            version(Win64)


Wait a minute! Why would the 64 bit build of DMD behave differently here? Shouldn't this be if the memory model of the target is set to 64?

No, the target architecture is not relevant here. The VS registry/Windows SDK keys are only found in the 32-bit registry hive. KEY_WOW64_32KEY doesn't seem good enough, I had to use WOW6432Node explicitly, while a 32-bit process is automatically mapped to that hive.

WalterBright · 2017-12-05T01:01:37Z

src/ddmd/link.d

+        }
+
+        /**
+         * retreive options to be passed to the Micrsoft linker


"retrieve", and "Microsoft"

WalterBright · 2017-12-05T01:02:00Z

src/ddmd/link.d

+        }
+
+        /**
+         * retreive path to the Micrsoft linker executable


"retrieve", and "Microsoft"

WalterBright · 2017-12-05T01:06:29Z

Much better. A couple more issues I commented on. I'm still not happy with the avoidable memory leaks, but it's not a deal breaker here. I do like how you fixed one leak by using realloc, I hadn't thought of that!

rainers · 2017-12-05T08:32:24Z

I'm still not happy with the avoidable memory leaks, but it's not a deal breaker here.

FileName.combine sometimes returns newly allocated memory, sometimes one of the arguments. This makes it necessary to predict what happens if you want to free the memory later. As this is done nowhere else, I opted not to do it here for a few more hundred bytes of leakage.

MartinNowak · 2017-12-16T17:44:33Z

Could you please add a changelog entry mentioning that VC is now detected at link-time instead of at install-time. Should we remove the detection from the installer?

MartinNowak · 2017-12-16T17:47:19Z

Factoring this out into a separate dmd/windows_detect_vs module would make sense IMO.

rainers · 2017-12-17T07:47:25Z

Could you please add a changelog entry mentioning that VC is now detected at link-time instead of at install-time.

Will do.

Should we remove the detection from the installer?

Part of it. We might still want to allow the option of downloading VS if not found. Please note that the "light" runtime still needs the VC 2010 distributables.

rainers · 2017-12-22T09:30:36Z

Could you please add a changelog entry mentioning that VC is now detected at link-time instead of at install-time.

Done: #7493

autodetect Visual Studio and linker options

6cb6aa8