ImportC preprocess Win32 C programs with sppn.exe

[WalterBright]

To get sppn.exe: http://ftp.digitalmars.com/sppn.zip for the Windows test setups.

[dlang-bot]

Thanks for your pull request, @WalterBright!

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.

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#14090"

[WalterBright]

As expected, Win32 tests are failing because sppn.exe isn't on the test machine.

[thewilsonator]

Do we really care about this? OMF will soon be deprecated.

[WalterBright]

Even while it's deprecated, it still needs to work.

[thewilsonator]

Why? What does this buy us?

it still needs to work.

is as assertion. What it the reasoning/motivation behind it?

[WalterBright]

What it the reasoning/motivation behind it?

"ImportC doesn't work for Win32 OMF, though dmd does" is not a good look for us. Especially since it's just few lines of code to support it.

[thewilsonator]

"ImportC doesn't work for Win32 OMF, though dmd does"

Neither of those thing are completely true though.

Automatic preprocessing of includes is not the entirety of importC, and DMD does not work correctly for OMF is the entire reason it will be deprecated.

[WalterBright]

DMD does not work correctly for OMF

Yes, it does. The fact that vibe.d was using the wrong import libs is not persuasive. (The import libs needed to be run through coff2omf to work with optlink.)

It's less work to support OMF with sppn.exe than it is to constantly have to explain why not.

[rainers]

As expected, Win32 tests are failing because sppn.exe isn't on the test machine.

dm857.zip is installed to build druntime and phobos, but PATH doesn't include it (see lines 78+ in the log).

The MS linker is not required it to be found via PATH, so I think cl should be detected via vsoptions.d aswell, and the path to sppn could be set by an environment variable (in sc.ini) similar to LINKCMD.

[WalterBright]

The test suite finds cl.exe, so it must be on the PATH. The test suite can also find optlink. I suggest putting sppn.exe in the same place optlink is. After all, if you're generating OMF, you'll need optlink along with sppn.

[rainers]

The test suite finds cl.exe, so it must be on the PATH.

That's because vcvarsall.bat is run at the beginning of the build script. This is not necessary the case on a user system.
Please also note, that you cannot use the same cl.exe to compile for x86 and x64 as they provide different preprocessor definitions. Using PATH will not work when both -m32 and -m64 are supposed to work seamlessly.

[rainers]

I suggest putting sppn.exe in the same place optlink is.

optlink comes wih dmd, and is found without PATH due to sc.ini. Adding sppn to dmd is probably not a good idea as it needs dmc's include files, too.

[WalterBright]

Please also note, that you cannot use the same cl.exe to compile for x86 and x64 as they provide different preprocessor definitions. Using PATH will not work when both -m32 and -m64 are supposed to work seamlessly.

I need your expertise to make this work.

optlink comes wih dmd, and is found without PATH due to sc.ini.

My reading of dmd's call to optlink is it calls spawnlp which looks along the PATH. sc.ini can override that, of course, by setting LINKCMD.

Adding sppn to dmd is probably not a good idea as it needs dmc's include files, too.

For none of the test suite does it need dmc's include files. If the user has C code and uses dmc, he likely has dmc installed with its include files. If the user writes new C code, and imports core.stdc.stdio, he might not need the dmc header files :-) Besides, sppn.exe is 200K in size. Not very big, not a burden to the install. And it eliminates a point of irritation for the user.

Lastly, in order to run the dmd test suite, it needs sppn.exe on the test machine.

[WalterBright]

@rainers I'm not sure what druntime and phobos need from dmc to compile. It does need snn.lib, but that is available separately from dmc.

[rainers]

I need your expertise to make this work.

I was afraid I could be dragged in here ;-) I can have a look...

sc.ini can override that, of course, by setting LINKCMD.

And it always does with: LINKCMD=%@P%\optlink.exe

For none of the test suite does it need dmc's include files.

druntime compiles errno.c, and phobos compiles zlib.

[WalterBright]

I was afraid I could be dragged in here ;-) I can have a look...

I'm already using your's and Benjamin's code to intercept stdout. You know the VC ecosystem infinitely better than I do, so I sure appreciate your help!

And it always does with: LINKCMD=%@p%\optlink.exe

I hate adding another variable, but I suppose SPPCMD grumble grumble

druntime compiles errno.c, and phobos compiles zlib.

You're right, I forgot. We could figure out a way to avoid compiling errno.c, but zlib is another matter.

[ibuclaw]

You're right, I forgot. We could figure out a way to avoid compiling errno.c, but zlib is another matter.

errno.c is almost redundant, but it's safer to keep it around for platforms where we haven't introduced D bindings/wrappers for.

[rainers]

I'm already using your's and Benjamin's code to intercept stdout. You know the VC ecosystem infinitely better than I do, so I sure appreciate your help!

See #14092

[maxhaton]

DMD does not work correctly for OMF

Yes, it does. The fact that vibe.d was using the wrong import libs is not persuasive. (The import libs needed to be run through coff2omf to work with optlink.)

It's less work to support OMF with sppn.exe than it is to constantly have to explain why not.

We should have this while dmd has any hint of OMF support but we should and can move away from OMF. In my experience helping people new to D it's solely been a source of confusion (and isn't actually in use in many 32 bit builds anyway since dub doesn't use it).

[rainers]

Azure is still not finding D:/a/1/s/tools/dm/bin/sppn.exe

I just noticed that runPreprocessor completely ignores the passed program. It needs a patch like:

 src/dmd/link.d | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/src/dmd/link.d b/src/dmd/link.d
index 20a7251f6..a5b1ae6d4 100644
--- a/src/dmd/link.d
+++ b/src/dmd/link.d
@@ -1063,7 +1063,8 @@ public int runPreprocessor(const(char)[] cpp, const(char)[] filename, const(char
                 /* Run command, intercept stdout, remove first line that CL insists on emitting
                  */
                 OutBuffer buf;
-                buf.printf("cl /P /nologo %.*s /Fi%.*s",
+                buf.writestring(cpp);
+                buf.printf(" /nologo %.*s /Fi%.*s",
                     cast(int)filename.length, filename.ptr, cast(int)output.length, output.ptr);
 
                 ubyte[2048] buffer = void;
@@ -1116,11 +1117,12 @@ public int runPreprocessor(const(char)[] cpp, const(char)[] filename, const(char
         }
         else if (target.objectFormat() == Target.ObjectFormat.omf)
         {
-            argv.push("sppn".xarraydup.ptr);     // Digital Mars C preprocessor
+            auto szCmd = cpp.xarraydup.ptr;
+            argv.push(szCmd);     // Digital Mars C preprocessor
             argv.push(filename.xarraydup.ptr);   // and the input file
             argv.push(null);                     // argv[] always ends with a null
             // spawnlp returns intptr_t in some systems, not int
-            return spawnvp(_P_WAIT, "sppn".ptr, argv.tdata());
+            return spawnvp(_P_WAIT, szCmd, argv.tdata());
         }
         else
         {

Adding "/P" to the command in cppCommand was probably the wrong idea in #14092.

[WalterBright]

Thanks, trying your changes now. I left the /P in because it causes cl to run the preprocessor only.

[WalterBright]

It's running sppn now! Yay! Thanks, @rainers
I'll look at the next error it's making tomorrow.

[WalterBright]

The OMF tests fail with:

-fail_compilation\test22935.c(18): Error: array index 5 is out of bounds `[0..4]`
+fail_compilation	est22935.c(18): Error: array index 5 is out of bounds `[0..4]`

The \t is being replaced by a tab. But who is doing it?

[WalterBright]

I found the problem. Microsoft's preprocessor outputs a preprocessed line directive as:

#line 1 "fail_compilation\\testTypeof.c"

whereas sppn.exe emits:

#line 1 "fail_compilation\testTypeof.c"

I'll see about modifying lexer.d to accommodate this.

[WalterBright]

Wound up modifying sppn.exe instead. Also, sppn.exe does not support U character literals, so moved that test into a .i file.

[WalterBright]

working now!

[rainers]

I left the /P in because it causes cl to run the preprocessor only.

Agreed, but it is now passed twice as cppCommand also appends it as I thought it could be useful to make it part of the environment variable. But more parameters are needed anyway, so I'd recommend removing it in cppCommand.

[rainers]

preprocess now has the same code for Posix and Windows. Folding that into unconditional code and keeping OS differences to runPreprocessor seems natural.

[WalterBright]

It's a good idea, but I'd prefer to do that as a refactoring.

[dkorpel]

working now!

Judging from the Codecov warnings, it's not being tested though. Edit: looks like coverage is 64-bit only

[WalterBright]

Code coverage testing is not done for Win32.

[WalterBright]

@RazvanN7 ?