-
Notifications
You must be signed in to change notification settings - Fork 441
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
Improve PNA system library organization with smaller usable components #4752
base: main
Are you sure you want to change the base?
Conversation
5ad47eb
to
3d14cfb
Compare
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
@qobilidop The changes you have to separate out the relatively small different parts of the pna.p4 include files into types_metadata.p4 and blocks.p4 for v0_5, v0_7, and latest dev version look reasonable to me. The test failures you are seeing are likely because the P4_16 code output from the front-end and mid-end of the p4c compiler is changing as a result of your header file changes, and there are expected output files checked into the Do you know how to update the contents of the |
@jafingerhut Thanks for your review comments! I have done the test data updates a few times in the past, but it's been a while that I forgot the details. I'll review the instructions from https://github.com/p4lang/p4c/tree/main/docs#testing and update the sample outputs as the next step. |
|
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
#include <pna.p4> | ||
#include <pna/v0_5/types_metadata.p4> | ||
#include <pna/v0_5/blocks.p4> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is deeper than I originally thought. This transformation result is problematic because #include <pna/v0_5/types_metadata.p4>
will duplicate #include <pna.p4>
contents, leading to later compiling errors.
Maybe the correct behavior should be #include <pna.p4>
not expanded at all? I need to figure out the relevant frontend pass responsible for this transformation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pass that does this toP4
. SourceInfo contains information about the source path a node came from. Iirc if this source path is considered a systems file the include is added.
This is super janky and leads to problem like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've figured out a relatively hacky solution, similar to the #4742 approach.
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
Signed-off-by: Bili Dong <[email protected]>
@@ -1235,7 +1235,10 @@ const PNAEbpfGenerator *ConvertToEbpfPNA::build(const IR::ToplevelBlock *tlb) { | |||
!d->is<IR::Type_Error>()) { | |||
if (d->srcInfo.isValid()) { | |||
auto sourceFile = d->srcInfo.getSourceFile(); | |||
if (sourceFile.endsWith("/pna.p4")) { | |||
// TODO: Similar logic exists in frontends/p4/toP4/toP4.cpp. Consider replacing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the parser should already determine whether an object is source from a systems file and add this information to the source information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree this will be a better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ChrisDodd Do you think this could be done in the P4 parser? How much work would it be?
@@ -32,6 +32,7 @@ limitations under the License. | |||
// Standard include paths for .p4 header files. The values are determined by | |||
// `configure`. | |||
extern const char *p4includePath; | |||
extern const char *p4includeInternalPath; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The discussion in #4742 makes me thing we just should have this be a set. Do not know how difficult this is to implement with all the back ends.
@@ -72,6 +72,7 @@ class ToP4 : public Inspector { | |||
listTerminators.pop_back(); | |||
} | |||
bool isSystemFile(cstring file); | |||
bool isSystemInternalFile(cstring file); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the definition of a system internal file. Any private header include prefixed with _internal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the hacky part. What happened is that in the following loop, program->objects
would contain not only #include <pna.p4>
that is directly from the source program, but all the transitive <*.p4>
libraries, like <_internal/pna/dev/blocks.p4>
. This will lead to compiler errors of duplicate definitions on the ToP4
generated code.
p4c/frontends/p4/toP4/toP4.cpp
Line 154 in b27702f
for (auto a : program->objects) { |
My current solution is to define this special p4include/_internal/
("system internal") directory, and filter it out in the ToP4
looping. But I don't think this is a proper solution in the long term.
No description provided.