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.

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

Jump to bottom

[#5152] Write preprocessed P4 to repro.p4 file when -P option is provided #5153

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

[#5152] Write preprocessed P4 to repro.p4 file when -P option is provided #5153

wants to merge 1 commit into from

Conversation

kfcripps
Copy link
Contributor

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.

@kfcripps
Add -P option
1f3d241 
Signed-off-by: Kyle Cripps <[email protected]>
@asl
Copy link
Contributor

asl commented Feb 27, 2025

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
Copy link
Contributor Author

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
Copy link
Contributor

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

@kfcripps
Copy link
Contributor Author

@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
Copy link

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

@vgurevich
Copy link

vgurevich commented Feb 28, 2025 •
edited
Loading

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.

@fruffy fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Feb 28, 2025
@kfcripps
Copy link
Contributor Author

kfcripps commented Mar 3, 2025

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
Copy link
Contributor

vlstill commented Mar 3, 2025

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
vgurevich suggested changes Mar 6, 2025
View reviewed changes
frontends/common/parser_options.cpp
newName += baseSuffix;
newName += name.extension();
if (savePreprocessed) {
std::filesystem::path fileName = makeFileName(dumpFolder, "repro.p4", "");
Copy link

Choose a reason for hiding this comment

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

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

frontends/common/parser_options.h
@@ -81,6 +81,8 @@ class ParserOptions : public Util::Options {
std::filesystem::path file;
/// if true preprocess only
bool doNotCompile = false;
/// if true save preprocessed P4 to repro.p4
bool savePreprocessed = false;
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vgurevich vgurevich vgurevich requested changes

@asl asl Awaiting requested review from asl

@vlstill vlstill Awaiting requested review from vlstill

@fruffy fruffy Awaiting requested review from fruffy

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
core Topics concerning the core segments of the compiler (frontend, midend, parser)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add option to print preprocessed P4 and continue with compilation
6 participants
@kfcripps @asl @ChrisDodd @vgurevich @vlstill @fruffy