Skip to content

[#5152] Write preprocessed P4 to <program_name>.p4pp file when --save-temps option is provided #5153

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.

Sign up for GitHub

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

Open
kfcripps wants to merge 6 commits into
base: main
Choose a base branch
from
Open

[#5152] Write preprocessed P4 to <program_name>.p4pp file when --save-temps option is provided #5153

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f486d15
Add -P option
kfcripps Feb 27, 2025
de515d5
Run cpp twice; Change option name -P -> --save-temps; Rename file to …
kfcripps Mar 18, 2025
36b8060
Do not move cmd decl outside of #ifdef __clang__
kfcripps May 12, 2025
26673d0
Open and read from program.p4pp instead of running cpp twice
kfcripps May 12, 2025
07c43e4
clang-format
kfcripps May 12, 2025
a6ce9bd
remove unintended changes
kfcripps May 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 51 additions & 13 deletions frontends/common/parser_options.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ limitations under the License.
#include <sys/wait.h>

#include <filesystem>
#include <fstream>
#include <memory>
#include <regex>
#include <sstream>
#include <unordered_set>

#include "absl/strings/escaping.h"
Expand Down Expand Up @@ -121,6 +123,13 @@ ParserOptions::ParserOptions(std::string_view defaultMessage) : Util::Options(de
return true;
},
"Output `make` dependency rule only (passed to preprocessor)");
registerOption(
"--save-temps", nullptr,
[this](const char *) {
savePreprocessed = true;
return true;
},
"Saves preprocessed P4 to filename.p4pp and do not exit compilation.");
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
registerOption(
"-MD", nullptr,
[this](const char *) {
Expand Down Expand Up @@ -405,16 +414,28 @@ const char *ParserOptions::getIncludePath() const {
return path.c_str();
}

// From (folder, file.ext, suffix) returns
// folder/file-suffix.ext
static std::filesystem::path makeFileName(const std::filesystem::path &folder,
const std::filesystem::path &name,
std::string_view baseSuffix) {
std::filesystem::path newName(name.stem());
newName += baseSuffix;
newName += name.extension();

return folder / newName;
}

std::optional<ParserOptions::PreprocessorResult> ParserOptions::preprocess() const {
FILE *in = nullptr;

if (file == "-") {
in = stdin;
} else {
#ifdef __clang__
std::string cmd("cc -E -x c -Wno-comment");
std::string cmd = "cc -E -x c -Wno-comment";
#else
std::string cmd("cpp");
std::string cmd = "cpp";
#endif
Comment on lines 453 to 439
Copy link
Copy Markdown
Member

@vlstill vlstill Mar 26, 2025

Choose a reason for hiding this comment

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

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).

Copy link
Copy Markdown
Contributor Author

@kfcripps kfcripps May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.


cmd += " -C -undef -nostdinc -x assembler-with-cpp " + preprocessor_options.string() +
Expand All @@ -439,19 +460,36 @@ std::optional<ParserOptions::PreprocessorResult> ParserOptions::preprocess() con
}
return std::nullopt;
}
return ParserOptions::PreprocessorResult(in, &closeFile);
}

// From (folder, file.ext, suffix) returns
// folder/file-suffix.ext
static std::filesystem::path makeFileName(const std::filesystem::path &folder,
const std::filesystem::path &name,
std::string_view baseSuffix) {
std::filesystem::path newName(name.stem());
newName += baseSuffix;
newName += name.extension();
if (savePreprocessed) {
std::stringstream stream;
char *line = nullptr;
size_t len = 0;
ssize_t read = 0;
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[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 uses AI. Check for mistakes.

return folder / newName;
while ((read = getline(&line, &len, in)) != -1) {
stream << line;
}
Comment on lines +466 to +472
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
closeFile(in);
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

std::filesystem::path fileName(file.stem());
fileName += ".p4pp";
Comment on lines +475 to +476
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

Bug: When file == "-" (stdin), calling file.stem() on a path containing just "-" will produce unexpected results. This will likely create a file named ".p4pp" in the dump folder, which is not a meaningful filename. Consider using a default filename like "stdin.p4pp" when reading from stdin.

Suggested change
std::filesystem::path fileName(file.stem());
fileName += ".p4pp";
std::filesystem::path fileName;
if (file == "-") {
fileName = "stdin.p4pp";
} else {
fileName = file.stem();
fileName += ".p4pp";
}

Copilot uses AI. Check for mistakes.
fileName = makeFileName(dumpFolder, fileName, "");
Comment on lines +475 to +477
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

[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 uses AI. Check for mistakes.
std::ofstream filestream{fileName};
if (filestream) {
if (Log::verbose()) std::cerr << "Writing preprocessed P4 to " << fileName << std::endl;
filestream << stream.str();
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
}
filestream.close();

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

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
perror("");
return std::nullopt;
}
}
return ParserOptions::PreprocessorResult(in, &closeFile);
}

bool ParserOptions::isv1() const { return langVersion == ParserOptions::FrontendVersion::P4_14; }
Expand Down
2 changes: 2 additions & 0 deletions frontends/common/parser_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,8 @@ class ParserOptions : public Util::Options {
std::filesystem::path file;
/// if true preprocess only
bool doNotCompile = false;
/// if true save preprocessed P4 to filename.p4pp
bool savePreprocessed = false;
Copy link
Copy Markdown

@vgurevich vgurevich Mar 6, 2025

Choose a reason for hiding this comment

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

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

/// Compiler version.
cstring compilerVersion;
/// if true skip preprocess
Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_errors_outputs/issue4365_no_demote_error_to_info.p4-stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
-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.
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
-MD Output `make` dependency rule to file as side effect (passed to preprocessor)
-MF file With -M, specify output file for dependencies (passed to preprocessor)
-MG with -M, suppress errors for missing headers (passed to preprocessor)
Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_errors_outputs/issue4365_no_demote_error_to_warning.p4-stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
-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.
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
-MD Output `make` dependency rule to file as side effect (passed to preprocessor)
-MF file With -M, specify output file for dependencies (passed to preprocessor)
-MG with -M, suppress errors for missing headers (passed to preprocessor)
Expand Down
1 change: 1 addition & 0 deletions testdata/p4_16_errors_outputs/issue4365_no_disable_error.p4-stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
-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.
Copy link

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

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

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 uses AI. Check for mistakes.
-MD Output `make` dependency rule to file as side effect (passed to preprocessor)
-MF file With -M, specify output file for dependencies (passed to preprocessor)
-MG with -M, suppress errors for missing headers (passed to preprocessor)
Expand Down
Loading