[#5152] Write preprocessed P4 to `<program_name>.p4pp` file when `--save-temps` option is provided by kfcripps · Pull Request #5153 · p4lang/p4c

kfcripps · 2025-02-27T21:42:35Z

Closes #5152.

Marking as draft until we decide:

What to name the option - I have named it -P (for "Preprocessed") for now, but @asl also suggested --save-temps.
What to name the generated file - I named it repro.p4 (short for "reproducer"), but maybe there is a better name.

asl · 2025-02-27T23:23:02Z

Not sure why the output should be named repro.p4. Normally for gcc / clang we're having:

Option is named --save-temps
For an input file named "foo.c" the preprocessed output is named foo.i.

kfcripps · 2025-02-27T23:27:50Z

Not sure why the output should be named repro.p4. Normally for gcc / clang we're having:

Option is named --save-temps

For an input file named "foo.c" the preprocessed output is named foo.i.

Sure, that sounds reasonable to me.

ChrisDodd · 2025-02-28T00:32:07Z

Isn't there already a -E option that just runs the preprocessor and writes that to stdout?

kfcripps · 2025-02-28T00:36:09Z

@ChrisDodd Yes, but the compiler exits immediately after doing so - the purpose of this option is to be able to save the preprocessed P4 and continue with compilation instead of exiting.

vgurevich · 2025-02-28T05:43:53Z

Not sure if it matters, but the existing Tofino implementation names this file as foo.p4pp

vgurevich · 2025-02-28T05:49:27Z

The other thing I'd strongly recommend if you want to do it in the frontend: sanitize the preprocessed file by not including the standard system includes (i.e. core.p4 and/or your own architectural files), or at least have an option to do that. That means the file will have the user's code preprocessed, but will still use #include <core.p4> and #include <arch.p4>.

This will allow to reproduce old issues much easier or notice compatibility issues whenever you change your arch files. Believe me, you'd like you did it.

kfcripps · 2025-03-03T17:45:55Z

Not sure if it matters, but the existing Tofino implementation names this file as foo.p4pp

Giving the generated preprocessed file a .p4pp suffix sounds good to me.

The other thing I'd strongly recommend if you want to do it in the frontend: sanitize the preprocessed file by not including the standard system includes (i.e. core.p4 and/or your own architectural files), or at least have an option to do that. That means the file will have the user's code preprocessed, but will still use #include <core.p4> and #include <arch.p4>.

This will allow to reproduce old issues much easier or notice compatibility issues whenever you change your arch files. Believe me, you'd like you did it.

I currently have no strong motivation to do that, but I am not opposed if someone else wants to add an option to do that in the future.

vlstill · 2025-03-03T20:20:43Z

My opinion is that --save-temps sounds like a reasonable option and I would say .p4pp is better than just .p4.

I have no strong opinion about leaving system includes, although having it configurable (maybe with default being "leaving" #include) could be best. The disadvantage of desugaring back to #include is that it can hide modifications (which is probably irrelevant for just preprocessing) and obviously it excludes the text of the includes so some issues might be accidentally resolved by later changes.

vgurevich · 2025-03-06T23:10:22Z

frontends/common/parser_options.cpp

-    newName += baseSuffix;
-    newName += name.extension();
+    if (savePreprocessed) {
+        std::filesystem::path fileName = makeFileName(dumpFolder, "repro.p4", "");


I think that using <program_name>.p4pp would be better, compared to having a fairly arbitrary fixed name.

vgurevich · 2025-03-06T23:11:14Z

frontends/common/parser_options.h

    /// if true preprocess only
    bool doNotCompile = false;
+    /// if true save preprocessed P4 to repro.p4
+    bool savePreprocessed = false;


If you decide to address my first comment, then, obviously, this comment needs to be changed too.

kfcripps · 2025-03-18T17:54:22Z

Updates:

Renamed the option to --save-temps
Renamed the generated file to filename.p4pp
Original code was broken because in was consumed for repro.p4 generation, and was empty by the time actual program parsing occurred. The new code runs cpp twice and uses a second FILE stream for filename.p4pp generation instead.

vlstill

I think the double preprocessing is a bit unfortunate (it is wasteful and in an extreme case, they could produce different results because of changes on the filesystem). Maybe instead if we have --save-temps we should direct the preprocessor output to the p4pp file and then open that file for the parser.

vlstill · 2025-03-26T06:51:25Z

frontends/common/parser_options.cpp

 #ifdef __clang__
-        std::string cmd("cc -E -x c -Wno-comment");
+        cmd = "cc -E -x c -Wno-comment";
 #else
-        std::string cmd("cpp");
+        cmd = "cpp";
 #endif


Why was the cmd var moved outside the if? When you are already touching this, I suggest converting it to a plain C++ if. Both branches are valid C++ after all. It could even be a ternary op: std::string cmd = (__clang__) ? ... : ...; (I would add the extra brackets because it is a macro, but maybe formatter will not like them).

I moved the cmd declaration back inside the #ifdef, but I'm not sure that macros can be used inside of regular if statements / ternary operations. When I tried doing this, I got the error:

../../../frontends/common/parser_options.cpp: In member function 'std::optional<std::unique_ptr<_IO_FILE, void (*)(_IO_FILE*)> > P4::ParserOptions::preprocess() const': ../../../frontends/common/parser_options.cpp:435:27: error: '__clang__' was not declared in this scope 435 | std::string cmd = __clang__ ? "cc -E -x c -Wno-comment" : "cpp"; | ^~~~~~~~~ ninja: build stopped: subcommand failed.

kfcripps · 2025-05-12T22:39:52Z

I think the double preprocessing is a bit unfortunate (it is wasteful and in an extreme case, they could produce different results because of changes on the filesystem). Maybe instead if we have --save-temps we should direct the preprocessor output to the p4pp file and then open that file for the parser.

This does seem better to me too. Done.

Signed-off-by: Kyle Cripps <kyle@pensando.io>

…program.p4pp. Signed-off-by: Kyle Cripps <kyle@pensando.io>

Signed-off-by: kfcripps <kyle@pensando.io>

Signed-off-by: Kyle Cripps <kyle@pensando.io>

frontends/common/parser_options.cpp

Signed-off-by: Kyle Cripps <60898032+kfcripps@users.noreply.github.com>

Copilot

Pull Request Overview

This PR implements the --save-temps option to save preprocessed P4 code to a <program_name>.p4pp file without exiting compilation, addressing issue #5152.

Key Changes:

Adds --save-temps command-line option to save preprocessed output
Implements logic to write preprocessed content to a .p4pp file and continue compilation
Updates help text across test output files to document the new option

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.

Show a summary per file

File	Description
`frontends/common/parser_options.h`	Adds `savePreprocessed` boolean flag to track when the option is enabled
`frontends/common/parser_options.cpp`	Implements `--save-temps` option registration and preprocessing logic to save and reopen the preprocessed file
`testdata/p4_16_errors_outputs/issue4365_no_disable_error.p4-stderr`	Updates help text to include the new `--save-temps` option
`testdata/p4_16_errors_outputs/issue4365_no_demote_error_to_warning.p4-stderr`	Updates help text to include the new `--save-temps` option
`testdata/p4_16_errors_outputs/issue4365_no_demote_error_to_info.p4-stderr`	Updates help text to include the new `--save-temps` option

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-13T18:29:29Z

frontends/common/parser_options.cpp

+
+        in = fopen(fileName.c_str(), "r");
+        if (in == nullptr) {
+            ::P4::error(ErrorType::ERR_IO, "Error invoking preprocessor");


The error message "Error invoking preprocessor" is misleading here since the preprocessor already succeeded. The actual error is that the saved preprocessed file cannot be reopened for reading. Consider a more accurate error message like "Error opening preprocessed file for reading" or "Failed to reopen saved preprocessed file".

Suggested change

::P4::error(ErrorType::ERR_IO, "Error invoking preprocessor");

::P4::error(ErrorType::ERR_IO, "Error opening preprocessed file for reading");

Copilot · 2025-11-13T18:29:29Z

frontends/common/parser_options.cpp

+            savePreprocessed = true;
+            return true;
+        },
+        "Saves preprocessed P4 to filename.p4pp and do not exit compilation.");


Grammatical error: "do not exit compilation" should be "does not exit compilation" to match the singular subject of the sentence.

Suggested change

"Saves preprocessed P4 to filename.p4pp and do not exit compilation.");

"Saves preprocessed P4 to filename.p4pp and does not exit compilation.");

Copilot · 2025-11-13T18:29:29Z

testdata/p4_16_errors_outputs/issue4365_no_demote_error_to_info.p4-stderr

 -U arg                                  Undefine macro (passed to preprocessor)
 -E                                      Preprocess only, do not compile (prints program on stdout)
 -M                                      Output `make` dependency rule only (passed to preprocessor)
+--save-temps                            Saves preprocessed P4 to filename.p4pp and do not exit compilation.


Grammatical error: "do not exit compilation" should be "does not exit compilation" to match the singular subject of the sentence.

Suggested change

--save-temps Saves preprocessed P4 to filename.p4pp and do not exit compilation.

--save-temps Saves preprocessed P4 to filename.p4pp and does not exit compilation.

Copilot · 2025-11-13T18:29:30Z

frontends/common/parser_options.cpp

+        std::filesystem::path fileName(file.stem());
+        fileName += ".p4pp";
+        fileName = makeFileName(dumpFolder, fileName, "");


[nitpick] The code unnecessarily constructs fileName in lines 475-476 only to pass it to makeFileName which deconstructs and reconstructs it. This can be simplified by calling makeFileName(dumpFolder, file, ".p4pp") directly, which would be clearer and avoid the intermediate construction.

Suggested change

std::filesystem::path fileName(file.stem());

fileName += ".p4pp";

fileName = makeFileName(dumpFolder, fileName, "");

std::filesystem::path fileName = makeFileName(dumpFolder, file, ".p4pp");

Copilot · 2025-11-13T18:29:30Z

frontends/common/parser_options.cpp

+        std::stringstream stream;
+        char *line = nullptr;
+        size_t len = 0;
+        ssize_t read = 0;


[nitpick] Unused variable: The variable read is assigned but never used. The return value of getline is only needed to check for end-of-file, so you can remove the variable declaration and check the condition directly: while (getline(&line, &len, in) != -1).

Copilot · 2025-11-13T18:29:31Z

testdata/p4_16_errors_outputs/issue4365_no_disable_error.p4-stderr

 -U arg                                  Undefine macro (passed to preprocessor)
 -E                                      Preprocess only, do not compile (prints program on stdout)
 -M                                      Output `make` dependency rule only (passed to preprocessor)
+--save-temps                            Saves preprocessed P4 to filename.p4pp and do not exit compilation.


Grammatical error: "do not exit compilation" should be "does not exit compilation" to match the singular subject of the sentence.

Suggested change

--save-temps Saves preprocessed P4 to filename.p4pp and do not exit compilation.

--save-temps Saves preprocessed P4 to filename.p4pp and does not exit compilation.

Copilot · 2025-11-13T18:29:31Z

testdata/p4_16_errors_outputs/issue4365_no_demote_error_to_warning.p4-stderr

 -U arg                                  Undefine macro (passed to preprocessor)
 -E                                      Preprocess only, do not compile (prints program on stdout)
 -M                                      Output `make` dependency rule only (passed to preprocessor)
+--save-temps                            Saves preprocessed P4 to filename.p4pp and do not exit compilation.


Grammatical error: "do not exit compilation" should be "does not exit compilation" to match the singular subject of the sentence.

Suggested change

--save-temps Saves preprocessed P4 to filename.p4pp and do not exit compilation.

--save-temps Saves preprocessed P4 to filename.p4pp and does not exit compilation.

Copilot · 2025-11-13T18:29:31Z

frontends/common/parser_options.cpp

+        std::ofstream filestream{fileName};
+        if (filestream) {
+            if (Log::verbose()) std::cerr << "Writing preprocessed P4 to " << fileName << std::endl;
+            filestream << stream.str();


Missing error handling for file write failure. If the file write fails (e.g., due to disk space issues or permission problems), the code silently continues. Consider adding error reporting if !filestream after opening or if the write operation fails.

Suggested change

filestream << stream.str();

filestream << stream.str();

if (!filestream) {

::P4::error(ErrorType::ERR_IO, "Failed to write preprocessed P4 to %s", fileName.c_str());

perror("");

filestream.close();

return std::nullopt;

}

Copilot · 2025-11-13T18:29:32Z

frontends/common/parser_options.cpp

+        char *line = nullptr;
+        size_t len = 0;
+        ssize_t read = 0;

-    return folder / newName;
+        while ((read = getline(&line, &len, in)) != -1) {
+            stream << line;
+        }


Memory leak: The line buffer allocated by getline is never freed. According to POSIX standards, getline allocates memory that must be freed by the caller. Add free(line) after the while loop to prevent memory leaks.

Copilot · 2025-11-13T18:29:32Z

frontends/common/parser_options.cpp

+        while ((read = getline(&line, &len, in)) != -1) {
+            stream << line;
+        }
+        closeFile(in);


Bug: Calling closeFile(in) on stdin will cause issues. When file == "-", in is set to stdin (line 433), but closeFile calls pclose() which should only be used on streams opened with popen(). This will result in undefined behavior. The code should check if in == stdin before calling closeFile(in), or handle stdin differently.

kfcripps requested review from asl, fruffy, vgurevich and vlstill February 27, 2025 21:42

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 28, 2025

vgurevich suggested changes Mar 6, 2025

View reviewed changes

kfcripps force-pushed the 5152 branch from 1f3d241 to 38b5bd2 Compare March 18, 2025 17:54

kfcripps marked this pull request as ready for review March 18, 2025 17:54

kfcripps requested a review from vgurevich March 18, 2025 17:54

kfcripps force-pushed the 5152 branch from 38b5bd2 to ebc4b0e Compare March 18, 2025 18:56

kfcripps changed the title ~~[#5152] Write preprocessed P4 to repro.p4 file when -P option is provided~~ [#5152] Write preprocessed P4 to <program_name>.p4pp file when --save-temps option is provided Mar 19, 2025

vlstill reviewed Mar 26, 2025

View reviewed changes

kfcripps force-pushed the 5152 branch from ebc4b0e to 27e084f Compare May 12, 2025 22:39

kfcripps requested a review from vlstill May 12, 2025 22:39

kfcripps added 5 commits May 23, 2025 07:32

Add -P option

f486d15

Signed-off-by: Kyle Cripps <kyle@pensando.io>

Run cpp twice; Change option name -P -> --save-temps; Rename file to …

de515d5

…program.p4pp. Signed-off-by: Kyle Cripps <kyle@pensando.io>

Do not move cmd decl outside of #ifdef __clang__

36b8060

Signed-off-by: kfcripps <kyle@pensando.io>

Open and read from program.p4pp instead of running cpp twice

26673d0

Signed-off-by: kfcripps <kyle@pensando.io>

clang-format

07c43e4

Signed-off-by: Kyle Cripps <kyle@pensando.io>

kfcripps force-pushed the 5152 branch from 61d0286 to 07c43e4 Compare May 23, 2025 14:32

kfcripps requested review from asl and vlstill and removed request for asl and vlstill May 27, 2025 17:23

kfcripps commented May 27, 2025

View reviewed changes

frontends/common/parser_options.cpp Outdated Show resolved Hide resolved

frontends/common/parser_options.cpp Outdated Show resolved Hide resolved

remove unintended changes

a6ce9bd

Signed-off-by: Kyle Cripps <60898032+kfcripps@users.noreply.github.com>

kfcripps requested a review from Copilot November 13, 2025 18:24

Copilot started reviewing on behalf of kfcripps November 13, 2025 18:24 View session

Copilot finished reviewing on behalf of kfcripps November 13, 2025 18:28

Copilot AI reviewed Nov 13, 2025

View reviewed changes

	::P4::error(ErrorType::ERR_IO, "Error invoking preprocessor");
	::P4::error(ErrorType::ERR_IO, "Error opening preprocessed file for reading");

	"Saves preprocessed P4 to filename.p4pp and do not exit compilation.");
	"Saves preprocessed P4 to filename.p4pp and does not exit compilation.");

	--save-temps Saves preprocessed P4 to filename.p4pp and do not exit compilation.
	--save-temps Saves preprocessed P4 to filename.p4pp and does not exit compilation.

-            filestream << stream.str();
+            filestream << stream.str();
+            if (!filestream) {
+                ::P4::error(ErrorType::ERR_IO, "Failed to write preprocessed P4 to %s", fileName.c_str());
+                perror("");
+                filestream.close();
+                return std::nullopt;
+            }

Conversation

kfcripps commented Feb 27, 2025

Uh oh!

asl commented Feb 27, 2025

Uh oh!

kfcripps commented Feb 27, 2025

Uh oh!

ChrisDodd commented Feb 28, 2025

Uh oh!

kfcripps commented Feb 28, 2025

Uh oh!

vgurevich commented Feb 28, 2025

Uh oh!

vgurevich commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfcripps commented Mar 3, 2025

Uh oh!

vlstill commented Mar 3, 2025

Uh oh!

vgurevich Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

vgurevich Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

kfcripps commented Mar 18, 2025

Uh oh!

vlstill left a comment

Choose a reason for hiding this comment

Uh oh!

vlstill Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

kfcripps May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfcripps commented May 12, 2025

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

vgurevich commented Feb 28, 2025 •

edited

Loading

kfcripps May 12, 2025 •

edited

Loading