Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bugzilla Issue 24397 ImportC: support function-like macros #16199

Merged
merged 1 commit into from Feb 19, 2024

Conversation

WalterBright
Copy link
Member

Finally! Now macros like:

#define ADD(a, b) a + b

are rewritten as:

auto ADD(__MP1, __MP2)(__MP1 a, __MP2 b) { return a + b; }

It's a miracle!

@WalterBright WalterBright added Atila Neves ImportC Pertaining to ImportC support labels Feb 17, 2024
@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 17, 2024

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close Bugzilla Severity Description
24397 enhancement Support C preprocessor function-like macros

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

@WalterBright WalterBright force-pushed the functionLikeMacro branch 3 times, most recently from e5e8430 to cc8c006 Compare February 17, 2024 04:44
@WalterBright
Copy link
Member Author

It's very frustrating when all the test suite Ubuntus fail with a seg fault, yet everything works fine on my local Ubuntu box.

@WalterBright WalterBright force-pushed the functionLikeMacro branch 3 times, most recently from b88f83e to e639cdb Compare February 17, 2024 05:56
@WalterBright
Copy link
Member Author

I'm dead in the water on this. I cannot repro any failures on my Ubuntu machine. None of the test machines build the compiler with -debug, so the crash dumps are nonexistent or useless. If I add logging printfs, the resulting output is so large the ci machines hang.

@WalterBright WalterBright changed the title ImportC: support function-like macros fix bugzilla ImportC: support function-like macros Feb 17, 2024
@ibuclaw
Copy link
Member

ibuclaw commented Feb 17, 2024

I'm dead in the water on this. I cannot repro any failures on my Ubuntu machine. None of the test machines build the compiler with -debug, so the crash dumps are nonexistent or useless. If I add logging printfs, the resulting output is so large the ci machines hang.

Have you tried podman/docker to run it in a 22.04 system environment?

@tim-dlang
Copy link
Contributor

I can reproduce it locally. The failure happens in addVar, because symbols is null for this line:

defineTab[cast(void*)s.ident] = symbols.length;

@ibuclaw
Copy link
Member

ibuclaw commented Feb 17, 2024

@WalterBright this seems to be the reduction that confirms my observation.

#define pr16199_trigger(cond,func,args) _Generic (cond, default: func args)
#define pr16199_skipped1(a) (1)   /* This is incorrectly parsed by cparseExpression */
#define pr16199_skipped2(b) (2)   /* This is incorrectly skipped by skipToNextLine */
#define pr16199_ice         0x3   /* segfault */

@WalterBright
Copy link
Member Author

@tim-dlang @ibuclaw I sure appreciate your help with this! I will set about fixing this.

@WalterBright WalterBright force-pushed the functionLikeMacro branch 2 times, most recently from 4ce697a to 8bd9815 Compare February 17, 2024 19:41
@WalterBright WalterBright changed the title fix bugzilla ImportC: support function-like macros fix bugzilla Issue 24397 ImportC: support function-like macros Feb 17, 2024
@WalterBright
Copy link
Member Author

@ibuclaw I can confirm that your reduction fails on my machine. Thanks!

@WalterBright WalterBright force-pushed the functionLikeMacro branch 5 times, most recently from 3e8e812 to e6aa692 Compare February 18, 2024 08:06
@WalterBright
Copy link
Member Author

I'm mystified by this, as this PR is only about D code using #defines, it doesn't have anything to do with how the C preprocessor works. Maybe the FreeBSD preprocessor is different? Unfortunately, my FreeBSD machine is broken at the moment.

@WalterBright
Copy link
Member Author

The OMF DMD is also failing, and I can repro that, so I'll investigate it first.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 19, 2024

I'm mystified by this, as this PR is only about D code using #defines, it doesn't have anything to do with how the C preprocessor works. Maybe the FreeBSD preprocessor is different? Unfortunately, my FreeBSD machine is broken at the moment.

I have a VM, and I see that this PR causes the regression.

Command:

../dmd/generated/freebsd/release/64/dmd -c -conf= -I../dmd/druntime/import  -w -de -preview=dip1000 -preview=dtorfields -preview=fieldwise -m64 -fPIC -O -release -P=-Ietc/c/zlib -P=-DHAVE_UNISTD_H -ofgenerated/freebsd/release/64/zlib.o etc/c/zlib/adler32.c etc/c/zlib/compress.c etc/c/zlib/crc32.c etc/c/zlib/deflate.c etc/c/zlib/gzclose.c etc/c/zlib/gzlib.c etc/c/zlib/gzread.c etc/c/zlib/gzwrite.c etc/c/zlib/infback.c etc/c/zlib/inffast.c etc/c/zlib/inflate.c etc/c/zlib/inftrees.c etc/c/zlib/trees.c etc/c/zlib/uncompr.c etc/c/zlib/zutil.c

Before:

$ nm generated/freebsd/release/64/zlib.o | wc -l
     360

After:

$ nm generated/freebsd/release/64/zlib.o | wc -l
       3
$ nm generated/freebsd/release/64/zlib.o 
                 U __start_minfo
                 U __stop_minfo
                 U _d_dso_registry

No C symbols are being emitted. I have an idea of what this might be.

@ibuclaw
Copy link
Member

ibuclaw commented Feb 19, 2024

This is the reproducer:

#define M16199C(X,S,M) ({ int __x; })

int pr16199c()
{
    return 0;
}

All functions after the macro definition get erroneously swallowed up, so the result ends up being:

extern (C) auto M16199C(__MP1, __MP2, __MP3)(__MP1 X, __MP2 S, __MP3 M)
{
	return delegate ()
	{
		{
			extern (C) int pr16199c()
			{
				return 0;
			}
			extern (C) int __x = void;
			;
		}
	}
	();
}

@WalterBright
Copy link
Member Author

damn, you're good, @ibuclaw !

@ibuclaw
Copy link
Member

ibuclaw commented Feb 19, 2024

damn, you're good, @ibuclaw !

This is the behaviour seen on FreeBSD that causes nothing to be emitted.

#define M16199Da(TYPE,VAR) ((TYPE)(VAR))
#define M16199D(X,S,M) ({ int *__x = (X); M16199Da(S *, __x); })
int pr16199d() { return 0; }

Tweak it a bit more, and we get a segfault in cparse.

#define M16199Ea(TYPE) (TYPE __x;)
#define M16199E(X,S,M) ({ M16199Ea(S *); })

@WalterBright
Copy link
Member Author

Ouch! Anyhow, the OMF problem is fixed.

@WalterBright WalterBright force-pushed the functionLikeMacro branch 2 times, most recently from 3d0f5b2 to 421fd06 Compare February 19, 2024 07:26
@WalterBright
Copy link
Member Author

@ibuclaw the problems you identified seem to be fixed now. There were 3 bugs I found and fixed.

@thewilsonator
Copy link
Contributor

generated/windows/debug/64/linkD.exe 
LINK : generated\windows\debug\64\dynamiccast.exe not found or not built by the last incremental link; performing full link
LINK : warning LNK4217: symbol '_D11dynamiccast1C7__ClassZ' defined in 'dynamiccast.obj' is imported by 'dynamiccast.obj' in function '_D4core8lifetime__T12_d_newclassTTC11dynamiccast1CZQBgFNaNbNeZQBc'

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some observations for future iterations

  1. It would be good to support do {} while(0) function-like macros.

  2. I see that #define N (42) is converted into auto N()() => 42 where a simple lookahead might be able to detect the lone literal in parens and simplify it as enum N = 42

  3. Do these macros occupy storage space if instantiated? ie: By wrapping it in an eponymous template, it looks like it would be possible to take the address of a C macro from D. What about an enum function/delegate (think enum ADD(__MP1, __MP2) = (__MP1 a, __MP2 b) { return a + b; }) instead?

@dkorpel
Copy link
Contributor

dkorpel commented Feb 19, 2024

The translation of macro functions to D functions is not correct in general because of arguments with side-effects. Consider:

#define DOUBLE(x) (x) + (x)

void f(void)
{
    int i = 0;
    DOUBLE(i++);
    assert(i == 2); // fails after D translation
}

Which is a silly example, but I've encountered real C macros that rely on this behavior.

@ntrel
Copy link
Contributor

ntrel commented Feb 19, 2024

#define DOUBLE(x) (x) + (x)

Maybe it would be enough to ignore macros that reuse an argument?

@WalterBright
Copy link
Member Author

@dkorpel of course you're right. There are other problems like if the macro body deliberately did not enclose a parameter in ( ), or even the entire expansion in ( ). More or less, if the macros are intended for use as metaprogramming rather than as template-like, there's going to be trouble.

I don't think it is possible for ImportC to promise to replicate C preprocessor semantics. The best we can do is make our best shot. And document things that won't work, like your example.

@WalterBright
Copy link
Member Author

@ntrel it's a good idea, but it also then may annoyingly refuse to compile things that would otherwise work as intended.

@WalterBright WalterBright merged commit f834870 into dlang:master Feb 19, 2024
46 of 48 checks passed
@dkorpel
Copy link
Contributor

dkorpel commented Feb 19, 2024

The best we can do is make our best shot.

Not always. I've mistranslated macros to functions before, and it's annoying to debug. I'd rather have ImportC fail to translate a macro than produce an incorrect result.

@WalterBright
Copy link
Member Author

Doc PR: dlang/dlang.org#3771

@WalterBright
Copy link
Member Author

I understand your concern, and it bothers me, too. I pointed out some other cases in the doc PR. A couple people have pointed out that not supporting function-like macros makes some C packages more or less unusable.

@Herringway
Copy link
Contributor

shouldn't new features like this get a changelog entry? it seems like a lot of new features get introduced without them these days

@dkorpel
Copy link
Contributor

dkorpel commented Feb 19, 2024

A couple people have pointed out that not supporting function-like macros makes some C packages more or less unusable.

Not unusable, it just requires more manual work. I'm using ImportC to import ffmpeg headers, and manually translate the macros ImportC doesn't parse yet:

import ffmpegh; // .c file with #include <libavcodec/avcodec.h> etc.

auto MKTAG(int a, int b, int c, int d) => (a | (b << 8) | (c << 16) | (cast(uint) d << 24));
enum AVERROR_EOF = -MKTAG( 'E','O','F',' '); ///< End of file

I'm not objecting to this PR by the way. It's nice to reduce the amount of manual translations necessary. I'm only pointing out something to watch out for.

@WalterBright
Copy link
Member Author

@Herringway that's why I created the bugzilla issue for it, which should serve the purpose.

@WalterBright
Copy link
Member Author

It's probably technically possible to determine if the call to a converted macro will result in different semantics than a C preprocessor expansion, but it doesn't seem worth while, and may result in unwarranted confidence in the results :-/

@WalterBright WalterBright deleted the functionLikeMacro branch February 20, 2024 04:26
@bachmeil
Copy link
Contributor

I understand your concern, and it bothers me, too. I pointed out some other cases in the doc PR. A couple people have pointed out that not supporting function-like macros makes some C packages more or less unusable.

@WalterBright If you want to go this route, how about letting the user write C code inside unit tests, and running the C preprocessor on that code before compiling it. The output could be compared with the output of D code that uses the same macro.

That would at least provide a weak check for correctness, versus silently introducing bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Atila Neves Enhancement ImportC Pertaining to ImportC support
Projects
None yet
9 participants