Skip to content

Set DTrace preprocessor via the DTRACE_CPP variable #11853

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

Closed
wants to merge 1 commit into from

Conversation

petk
Copy link
Member

@petk petk commented Aug 1, 2023

The way DTrace generates files depend on the given preprocessor. Without passing the CPP environment variable dtrace Python script uses the cpp by default.

For systems where the CPP is set to something else, the generated files won't be created the same way, so instead the DTrace preprocessor can be overriden manually with the new DTRACE_CPP variable. For example:

./configure DTRACE_CPP="gcc -E -xc" --enable-dtrace

Fixes GH-11847

The way DTrace generates files depend on the given preprocessor. Without
passing the CPP environment variable dtrace Python script uses the cpp
by default.

For systems where the CPP is set to something else, the generated files
won't be created the same way, so instead the DTrace preprocessor can be
overriden manually with the new DTRACE_CPP variable. For example:

./configure DTRACE_CPP="gcc -E -xc" --enable-dtrace

Fixes phpGH-11847
@remicollet
Copy link
Member

For consistency, shouldn't we also have a DTRACE_CC ?

I will try to test it later (not much available, you know... Holidays..)

@f4z4on
Copy link
Contributor

f4z4on commented Aug 2, 2023

Wow 🤯 That is definitely the issue. Spaces are alright… I’ll describe my thinking here (you PHP implementation folks all probably know this already 😅).

I was not aware of the difference between cpp and gcc -E. It’s probably the .d file extension which confuses gcc executable while cpp probably defaults to C. Proper D language does not have a preprocessor. However, DTrace uses #pragma directives (that is C preprocessing macro) for statically-defined traces (like PHP does) and scripts. Therefore -xc is correct, I guess. SystemTap’s dtrace defaults to cpp, so that issue wasn't visible until I started meddling with PHP cross compilation with DTrace enabled which requires different compiler executables altogether.

Now to possible solution…

I don’t think DTrace needs its own preprocessor. If so, people should be able to override it when invoking both ./configure and make. Much like CC, CFLAGS and so on. Good build systems following ./configure && make && make check paradigm do that. All in all, I still think there should not be a separate macro for users to set.

There may be a macro derived from CPP macro, similar to what CFLAGS and CFLAGS_CLEAN are doing. If someone really really wanted to override that, they still could, but I’d say that is a very obscure use case.

Or, and I prefer this one, we may simply force -xc after CPP when invoking dtrace -h -C on non-C files (that is files with .d extension). There is only one such invocation (two if you count oci8 extension), and it is that make rule I cherry-picked in my previous comment. The other invocations do not use preprocessing. So, perhaps even ditching CPP from their invocations to be more deliberate about it. Similarly ditching CC and CFLAGS from other dtrace -h -C invocations. This is SystemTap-specific behavior. The -C option is as well, and that is the option enabling C preprocessor use. SystemTap is Linux-only. Others ignore all of this… It just works for everyone… Anyone stumbling upon this code in years to come would see there is some complicated story right away because dtrace invocations differ. There is no silver bullet that would not introduce more complexity (i.e., additional m4 macros, macro dependencies, …) and I believe that’s something everyone should strive for to avoid unless necessary.

diff --git a/build/php.m4 b/build/php.m4
index 921a78eb78..73dd1e8e27 100644
--- a/build/php.m4
+++ b/build/php.m4
@@ -2389,7 +2389,7 @@ dnl overwritten (Bug 61268).
 $abs_srcdir/$ac_provsrc:;
 
 $ac_bdir[$]ac_hdrobj: $abs_srcdir/$ac_provsrc
-	CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -h -C -s $ac_srcdir[$]ac_provsrc -o \$[]@.bak && \$(SED) -e 's,PHP_,DTRACE_,g' \$[]@.bak > \$[]@
+	CPP="\$(CPP) -xc" dtrace -h -C -s $ac_srcdir[$]ac_provsrc -o \$[]@.bak && \$(SED) -e 's,PHP_,DTRACE_,g' \$[]@.bak > \$[]@
 
 \$(PHP_DTRACE_OBJS): $ac_bdir[$]ac_hdrobj
 
@@ -2409,12 +2409,12 @@ EOF
 $ac_bdir[$]ac_provsrc.lo: \$(PHP_DTRACE_OBJS)
 	echo "[#] Generated by Makefile for libtool" > \$[]@
 	@test -d "$dtrace_lib_dir" || mkdir $dtrace_lib_dir
-	if CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $dtrace_d_obj -s $abs_srcdir/$ac_provsrc $dtrace_lib_objs 2> /dev/null && test -f "$dtrace_d_obj"; then [\\]
+	if CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $dtrace_d_obj -s $abs_srcdir/$ac_provsrc $dtrace_lib_objs 2> /dev/null && test -f "$dtrace_d_obj"; then [\\]
 	  echo "pic_object=['].libs/$dtrace_prov_name[']" >> \$[]@ [;\\]
 	else [\\]
 	  echo "pic_object='none'" >> \$[]@ [;\\]
 	fi
-	if CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $ac_bdir[$]ac_provsrc.o -s $abs_srcdir/$ac_provsrc $dtrace_nolib_objs 2> /dev/null && test -f "$ac_bdir[$]ac_provsrc.o"; then [\\]
+	if CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $ac_bdir[$]ac_provsrc.o -s $abs_srcdir/$ac_provsrc $dtrace_nolib_objs 2> /dev/null && test -f "$ac_bdir[$]ac_provsrc.o"; then [\\]
 	  echo "non_pic_object=[']$dtrace_prov_name[']" >> \$[]@ [;\\]
 	else [\\]
 	  echo "non_pic_object='none'" >> \$[]@ [;\\]
@@ -2426,7 +2426,7 @@ EOF
   *)
 cat>>Makefile.objects<<EOF
 $ac_bdir[$]ac_provsrc.o: \$(PHP_DTRACE_OBJS)
-	CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o \$[]@ -s $abs_srcdir/$ac_provsrc $dtrace_objs
+	CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o \$[]@ -s $abs_srcdir/$ac_provsrc $dtrace_objs
 
 EOF
     ;;
diff --git a/ext/oci8/config.m4 b/ext/oci8/config.m4
index 883e272fb1..6fcc6d9eab 100644
--- a/ext/oci8/config.m4
+++ b/ext/oci8/config.m4
@@ -124,7 +124,7 @@ dnl overwritten (Bug 61268).
 PHP_EXT_SRCDIR([oci8])/$ac_provsrc:;
 
 $ac_bdir[$]ac_hdrobj: $ac_srcdir[$]ac_provsrc
-	CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -h -C -s $ac_srcdir[$]ac_provsrc -o \$[]@.bak && \$(SED) -e 's,PHPOCI_,DTRACE_,g' \$[]@.bak > \$[]@
+	CPP="\$(CPP) -xc" dtrace -h -C -s $ac_srcdir[$]ac_provsrc -o \$[]@.bak && \$(SED) -e 's,PHPOCI_,DTRACE_,g' \$[]@.bak > \$[]@
 
 \$(OCI8_DTRACE_OBJS): $ac_bdir[$]ac_hdrobj
 
@@ -145,12 +145,12 @@ EOF
 $ac_bdir[$]ac_provsrc.lo: \$(OCI8_DTRACE_OBJS)
 	echo "[#] Generated by Makefile for libtool" > \$[]@
 	@test -d "$dtrace_lib_dir" || mkdir $dtrace_lib_dir
-	if CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $dtrace_d_obj -s $ac_srcdir[$]ac_provsrc $dtrace_oci8_lib_objs 2> /dev/null && test -f "$dtrace_d_obj"; then [\\]
+	if CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $dtrace_d_obj -s $ac_srcdir[$]ac_provsrc $dtrace_oci8_lib_objs 2> /dev/null && test -f "$dtrace_d_obj"; then [\\]
 	  echo "pic_object=['].libs/$dtrace_prov_name[']" >> \$[]@ [;\\]
 	else [\\]
 	  echo "pic_object='none'" >> \$[]@ [;\\]
 	fi
-	if CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $ac_bdir[$]ac_provsrc.o -s $ac_srcdir[$]ac_provsrc $dtrace_nolib_objs 2> /dev/null && test -f "$ac_bdir[$]ac_provsrc.o"; then [\\]
+	if CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o $ac_bdir[$]ac_provsrc.o -s $ac_srcdir[$]ac_provsrc $dtrace_nolib_objs 2> /dev/null && test -f "$ac_bdir[$]ac_provsrc.o"; then [\\]
 	  echo "non_pic_object=[']$dtrace_prov_name[']" >> \$[]@ [;\\]
 	else [\\]
 	  echo "non_pic_object='none'" >> \$[]@ [;\\]
@@ -162,7 +162,7 @@ EOF
     AC_MSG_WARN([OCI8 extension: OCI8 DTrace support is not confirmed on this platform])
 cat>>Makefile.objects<<EOF
 $ac_bdir[$]ac_provsrc.o: \$(OCI8_DTRACE_OBJS)
-	CPP="\$(CPP)" CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o \$[]@ -s $ac_srcdir[$]ac_provsrc $dtrace_oci8_objs
+	CC="\$(CC)" CFLAGS="\$(CFLAGS_CLEAN)" dtrace -G -o \$[]@ -s $ac_srcdir[$]ac_provsrc $dtrace_oci8_objs
 
 EOF
     ;;

But… I’m talking here as a user of PHP build system who builds PHP regularly, including cross-compiling. There may be other consideration I am not aware of. From the outside, I prefer a simple solution which works and if there is a need for something obscure, people needing it are expected to understand how build systems work and how to modify them — there is no need to cater to them at the expense of introducing bloat. I believe a use case of using completely separate CPP just for dtrace utility from SystemTap on Linux is such an obscure case. These are just C macros, it’s just that we have found some C preprocessor invocations which do not default to C when processing source files with .d extension… Luckily on Linux with SystemTap, there’s a safe and broadly-implemented way of forcing it via that -xc option.

@f4z4on
Copy link
Contributor

f4z4on commented Aug 2, 2023

Another option is to stop setting CPP. As I mentioned above, SystemTap’s dtrace defaults to cpp and that just works… If I'm not mistaken, header files generated by dtrace are architecture-independent. I think I tested this, when I was working on #11643 because I originally wanted to only add CC. Then I thought that CPP is part of the suit along with CC, so if we’re passing along customized CC, we should also respect customized CPP. But as far as I can tell, it’s not necessary. I don't like this option though.

I still prefer appending -xc to CC and setting only the necessary environment variables for SystemTap’s dtrace based on what we want from dtrace in that particular invocation.

@f4z4on
Copy link
Contributor

f4z4on commented Aug 2, 2023

I’ve submitted alternative pull request #11858 based on my comments here… I suggest continuing the discussion back at original issue #11847.

@petk
Copy link
Member Author

petk commented Aug 2, 2023

@f4z4on awesome, thanks. I'll check it out.

@petk
Copy link
Member Author

petk commented Aug 2, 2023

I'm closing this PR to have a solution #11858...

@petk petk closed this Aug 2, 2023
@petk petk deleted the patch-dtrace branch August 7, 2023 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DTrace enabled build is broken
3 participants