ImportC: run cpp on Posix .c files #13955

WalterBright · 2022-04-06T09:07:34Z

It's way past time for dmd to start running the C preprocessor. This kicks that baby bird out of the nest so it can learn to fly.

I like incrementalism, so this PR does as little as possible:

it only works on .c files
if you don't want to run the preprocessor on Posix, name the file with a .i extension
it runs cpp on the default path
no parameters can be passed to cpp
doesn't automatically #include <importc.h>
error messages are minimal
most of the code is copypasta from runProgram() in link.d

Hence review should be easy.

Based on Max's prior work maxhaton@e132151 greatly simplified.

dlang-bot · 2022-04-06T09:07:36Z

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

WalterBright · 2022-04-06T09:23:01Z

Eh, looks like lexer.d is not liking the output of cpp. Will look at that tomorrow.

src/dmd/dmodule.d

ibuclaw · 2022-04-06T10:25:04Z

src/dmd/cpreprocess.d

+
+import dmd.astenums;
+import dmd.globals;
+import dmd.link;


Because, no dmd driver/backend code should be pulled into the front-end. (cc @kinke).

offer a suggestion

The Compiler interface might be one place for it. That is at least one module that is not meant to be common amongst implementing compilers.

It's already been replaced with a pointer.

What's imported from dmd.link?

WalterBright · 2022-04-06T21:09:16Z

@ibuclaw this fails on linux with:

... dshell/issue22816.d
==============================
Test 'dshell/issue22816.d' failed. The logged output:
[COMPILE_TEST] '/tmp/cirrus-ci-build/generated/linux/release/64/dmd' '-conf=' '-m64' '-fPIC' '-odtest_results/dshell/issue22816' '-oftest_results/dshell/issue22816/run' '-I=/tmp/cirrus-ci-build/test/dshell' '-I=test_results/dshell/issue22816' '-I=/tmp/cirrus-ci-build/test/tools/dshell_prebuilt' '-i' '-i=-dshell_prebuilt' 'test_results/dshell_prebuilt.o' '/tmp/cirrus-ci-build/test/dshell/issue22816.d'
[RUN_TEST] 'test_results/dshell/issue22816/run'
[RUN] '/tmp/cirrus-ci-build/generated/linux/release/64/dmd' '-m64' '-c' 'dshell/extra-files/issue22816.c'
core.exception.AssertError@/tmp/cirrus-ci-build/test/tools/dshell_prebuilt/dshell_prebuilt.d(287): Expected 'Error: cannot find input file'

The failing test case #13713 is written by you and I don't know what it is supposed to be doing. Please help.

ibuclaw · 2022-04-06T21:28:19Z

@ibuclaw this fails on linux with:

... dshell/issue22816.d
==============================
Test 'dshell/issue22816.d' failed. The logged output:
[COMPILE_TEST] '/tmp/cirrus-ci-build/generated/linux/release/64/dmd' '-conf=' '-m64' '-fPIC' '-odtest_results/dshell/issue22816' '-oftest_results/dshell/issue22816/run' '-I=/tmp/cirrus-ci-build/test/dshell' '-I=test_results/dshell/issue22816' '-I=/tmp/cirrus-ci-build/test/tools/dshell_prebuilt' '-i' '-i=-dshell_prebuilt' 'test_results/dshell_prebuilt.o' '/tmp/cirrus-ci-build/test/dshell/issue22816.d'
[RUN_TEST] 'test_results/dshell/issue22816/run'
[RUN] '/tmp/cirrus-ci-build/generated/linux/release/64/dmd' '-m64' '-c' 'dshell/extra-files/issue22816.c'
core.exception.AssertError@/tmp/cirrus-ci-build/test/tools/dshell_prebuilt/dshell_prebuilt.d(287): Expected 'Error: cannot find input file'

The failing test case #13713 is written by you and I don't know what it is supposed to be doing. Please help.

See #13707 for the related bug fix.

It is expecting an error due to filemanager.lookup failing, but now you are calling preprocess and possibly getting another error (I hope not success).

To reproduce/get the new error message:

touch issue22816.d
dmd issue22816.c

Yes, those file extensions are correct.

WalterBright · 2022-04-06T21:40:54Z

@ibuclaw thanks for the quick answer

src/dmd/README.md

Made it a function pointer.

src/dmd/globals.d

ibuclaw · 2022-04-07T06:12:35Z

src/dmd/dmodule.d

+            if (FileName.equalsExt(srcfile.toString(), c_ext) &&
+                FileName.exists(srcfile.toString()))
+            {
+                filename = global.preprocess(srcfile);  // run C preprocessor


Check for !is null.

It returns a struct, not a pointer.

I presume he means if (global.preprocess !is null) filename = global.preprocess(srcfile);?

I prefer to ask @ibuclaw to explain, as I tire of guessing :-)

Yes, the function pointer might be null.

I expect a "no-op preprocess" function would be provided that simply returned its argument. It's more attractive than adding the extra test.

I expect a "no-op preprocess" function would be provided that simply returned its argument. It's more attractive than adding the extra test.

If you want that, global.preprocess should have a "contract" check for non-null function pointers. I agree with @ibuclaw anyway, because that way, there is a way to disable preprocess for those files. EDIT: I mean a "no-op" can do the job but it's counter-intuitive and a function call has higher latency for that purpose, instead of a simple test check.

Don't need a contract check for null pointers, as the hardware does that for you.

it's counter-intuitive

It's a different style, called branchless programming. The nice thing about it is it removes clutter from the code.

and a function call has higher latency for that purpose, instead of a simple test check.

I'd agree if it was an inner loop, but it's once per file. Not significant.

Initially this would be unimplemented in gdc and ldc though, resulting in a segfault. If this field was default initialized with a noop lambda then that'd be different.

ibuclaw · 2022-04-07T06:22:16Z

src/dmd/cpreprocess.d

+
+import dmd.astenums;
+import dmd.globals;
+import dmd.link;


The Compiler interface might be one place for it. That is at least one module that is not meant to be common amongst implementing compilers.

WalterBright · 2022-04-07T07:34:49Z

We've had ImportC without preprocessor support for 6 months or thereabouts. It desperately needs to support the preprocessor. This is just a first step. Please approve this so I can move forward to getting it completed and useful. We won't know what a better design would be without making more progress with this.

Nothing will move forward without this PR.

thewilsonator · 2022-04-07T07:40:25Z

src/dmd/dmodule.d

+            if (FileName.equalsExt(srcfile.toString(), c_ext) &&
+                FileName.exists(srcfile.toString()))
+            {
+                filename = global.preprocess(srcfile);  // run C preprocessor


I presume he means if (global.preprocess !is null) filename = global.preprocess(srcfile);?

src/dmd/root/env.d

src/dmd/utils.d

src/dmd/README.md

implemented suggestion

WalterBright · 2022-04-15T00:13:30Z

ping @ibuclaw

RazvanN7 · 2022-04-25T13:06:55Z

@WalterBright there is this comment from @ibuclaw that has not been addressed.

RazvanN7 · 2022-04-25T13:16:14Z

cc @maxhaton. What are your thoughts on this?

WalterBright · 2022-04-26T22:59:20Z

@RazvanN7 it had already been addressed.

thewilsonator

it had already been addressed.

No it hasn't, dmd/cpreprocess.d still imports dmd/link.d

WalterBright · 2022-04-28T07:17:44Z

No it hasn't, dmd/cpreprocess.d still imports dmd/link.d

Right. But cpreprocess.d is only called via a pointer. It does not need to be imported. It is only imported by mars.d, which is not used by gdc and ldc.

requirements are already met

RazvanN7 · 2022-04-28T12:46:50Z

Added the "72h no objection -> merge" label.

ljmf00 · 2022-04-28T14:10:34Z

This should have a changelog entry.

maxhaton · 2022-04-28T17:04:25Z

Added the "72h no objection -> merge" label.

The approach is fairly crude but I don't mind merging this for now, as I can still work on mine. This PR cannot last because it can't pick the right preprocessor, but it's good enough for the immediate future.

ibuclaw · 2022-04-29T07:40:07Z

@kinke - any comments?

kinke · 2022-04-29T12:14:52Z

Somewhat off-topic, regarding hardcoded cpp for Posix targets - I assume there are according switches for gcc/clang too? LDC has a -gcc option to explicitly select the C[++] compiler for linking (other option: CC env var); it'd be nice to be able to reuse that for C preprocessing (especially for cross-compilation, where -gcc is often set to something like aarch64-linux-gnu-gcc).

ibuclaw · 2022-04-29T15:57:10Z

Somewhat off-topic, regarding hardcoded cpp for Posix targets - I assume there are according switches for gcc/clang too? LDC has a -gcc option to explicitly select the C[++] compiler for linking (other option: CC env var); it'd be nice to be able to reuse that for C preprocessing (especially for cross-compilation, where -gcc is often set to something like aarch64-linux-gnu-gcc).

Agreed on the hard coding of cpp in this PR. For me it would pretty much mean that I can no longer run a part of the DMD testsuite locally anymore (I have a weird set-up with different sysroots for various sorts of compilers, however I'm usually running on the fringe anyway, so do regularly encounter periods when tests are broken for long stretches of time :-).

That the preprocessor handler is an extern(C++) callback though, that would mean you have the opportunity to implement it as you see fit.

I have reservations on this callback expecting a new file to be created though. I might chance it to instead do this all in memory without needing to fork().

maxhaton · 2022-04-29T16:05:27Z

Somewhat off-topic, regarding hardcoded cpp for Posix targets - I assume there are according switches for gcc/clang too? LDC has a -gcc option to explicitly select the C[++] compiler for linking (other option: CC env var); it'd be nice to be able to reuse that for C preprocessing (especially for cross-compilation, where -gcc is often set to something like aarch64-linux-gnu-gcc).

Agreed on the hard coding of cpp in this PR. For me it would pretty much mean that I can no longer run a part of the DMD testsuite locally anymore (I have a weird set-up with different sysroots for various sorts of compilers, however I'm usually running on the fringe anyway, so do regularly encounter periods when tests are broken for long stretches of time :-).

That the preprocessor handler is an extern(C++) callback though, that would mean you have the opportunity to implement it as you see fit.

I have reservations on this callback expecting a new file to be created though. I might chance it to instead do this all in memory without needing to fork().

The design in my PR is intended at least somewhat to automate finding the right preprocessor, or make it illegal to use the wrong one. It's beerconf so why not discuss it there I think

WalterBright · 2022-04-29T16:19:21Z

This PR is coming up on being a month old. I don't have any angst about replacing it with something better, but right now we don't have anything better. We need something that works, and this works.

WalterBright · 2022-05-01T06:14:30Z

@ibuclaw what's holding this up?

maxhaton · 2022-05-01T18:58:16Z

@ibuclaw what's holding this up?

I can't see it so needs to implicitly ImportC.h for one otherwise it's useless in practice because no system headers will work.

WalterBright · 2022-05-01T20:23:06Z

I'm planning on implicitly #include-ing importc.h after this one is merged.

WalterBright · 2022-05-02T07:43:10Z

72 hours are up!

kinke · 2022-05-02T11:05:05Z

I assume there are according switches for gcc/clang too?

Answering myself: yes, both gcc and clang support -E for preprocessing only.

I'm planning on implicitly #include-ing importc.h after this one is merged.

For gcc and clang, -include importc.h does exactly that.

WalterBright · 2022-05-03T03:58:59Z

@kinke yes. There's also a switch for both MSVC and DMC's preprocessors to do a -include.

ibuclaw · 2023-11-20T10:05:34Z

It looks like this PR introduced a CI regression.

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

WalterBright added Review:Easy Review Feature:ImportC Pertaining to ImportC support labels Apr 6, 2022

WalterBright force-pushed the preprocess branch 2 times, most recently from ccdb18b to 06a362e Compare April 6, 2022 09:11

ibuclaw previously requested changes Apr 6, 2022

View reviewed changes

WalterBright force-pushed the preprocess branch 5 times, most recently from 8202e5c to 1c51280 Compare April 6, 2022 20:48

WalterBright force-pushed the preprocess branch from 1c51280 to f150c26 Compare April 6, 2022 21:40

WalterBright force-pushed the preprocess branch 2 times, most recently from a73e5ca to 59fd43a Compare April 6, 2022 23:24

ljmf00 reviewed Apr 6, 2022

View reviewed changes

src/dmd/README.md Outdated Show resolved Hide resolved

src/dmd/README.md Show resolved Hide resolved

WalterBright force-pushed the preprocess branch from 59fd43a to 6b1d9f0 Compare April 7, 2022 03:23

WalterBright requested review from MartinNowak, Geod24, rainers and UplinkCoder as code owners April 7, 2022 03:23

WalterBright force-pushed the preprocess branch 2 times, most recently from 1926386 to d769360 Compare April 7, 2022 04:12

ibuclaw requested changes Apr 7, 2022

View reviewed changes

thewilsonator previously requested changes Apr 7, 2022

View reviewed changes

WalterBright force-pushed the preprocess branch from 04e0a4c to 8be14b0 Compare April 14, 2022 18:46

thewilsonator previously requested changes Apr 27, 2022

View reviewed changes

RazvanN7 added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label Apr 28, 2022

WalterBright merged commit 67c6479 into dlang:master May 2, 2022

WalterBright deleted the preprocess branch May 2, 2022 07:43

WalterBright added a commit to WalterBright/dmd that referenced this pull request May 9, 2022

ImportC: run cpp on Posix .c files (dlang#13955)

2cdfe9a

thewilsonator pushed a commit to thewilsonator/dmd that referenced this pull request May 26, 2022

ImportC: run cpp on Posix .c files (dlang#13955)

12605da

thewilsonator pushed a commit to thewilsonator/dmd that referenced this pull request May 26, 2022

ImportC: run cpp on Posix .c files (dlang#13955)

794fbbf

ImportC: run cpp on Posix .c files #13955

ImportC: run cpp on Posix .c files #13955

Conversation

WalterBright commented Apr 6, 2022

dlang-bot commented Apr 6, 2022

Bugzilla references

Testing this PR locally

WalterBright commented Apr 6, 2022

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

WalterBright commented Apr 6, 2022

ibuclaw commented Apr 6, 2022 • edited Loading

WalterBright commented Apr 6, 2022

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

ljmf00 Apr 8, 2022 • 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

Choose a reason for hiding this comment

WalterBright commented Apr 7, 2022 • edited Loading

Choose a reason for hiding this comment

WalterBright commented Apr 15, 2022

RazvanN7 commented Apr 25, 2022

RazvanN7 commented Apr 25, 2022

WalterBright commented Apr 26, 2022

thewilsonator left a comment

Choose a reason for hiding this comment

WalterBright commented Apr 28, 2022

RazvanN7 commented Apr 28, 2022

ljmf00 commented Apr 28, 2022

maxhaton commented Apr 28, 2022

ibuclaw commented Apr 29, 2022

kinke commented Apr 29, 2022

ibuclaw commented Apr 29, 2022

maxhaton commented Apr 29, 2022

WalterBright commented Apr 29, 2022

WalterBright commented May 1, 2022

maxhaton commented May 1, 2022

WalterBright commented May 1, 2022

WalterBright commented May 2, 2022

kinke commented May 2, 2022

WalterBright commented May 3, 2022

ibuclaw commented Nov 20, 2023 • edited Loading

ibuclaw commented Apr 6, 2022 •

edited

Loading

ljmf00 Apr 8, 2022 •

edited

Loading

WalterBright commented Apr 7, 2022 •

edited

Loading

ibuclaw commented Nov 20, 2023 •

edited

Loading