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

Add checks to annotation getters. #5052

Merged
merged 3 commits into from
Dec 12, 2024
+50 −9
Merged

Add checks to annotation getters. #5052

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30ff78d
Add annotation error messages.
fruffy Dec 4, 2024
2fb857e
Check for KVAnnotation instead of deferring to annotationKind().
fruffy Dec 5, 2024
8390080
Use try-catch instead of BUG_CHECK.
fruffy Dec 9, 2024
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
Next Next commit
Add annotation error messages. 
Signed-off-by: fruffy <fruffy@nyu.edu>
  • Loading branch information
@fruffy
fruffy committed Dec 11, 2024
commit 30ff78dc449858ad3c406bd4ee009dcf93bfc695
31 changes: 25 additions & 6 deletions ir/base.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,17 +332,36 @@ class Annotation {
Vector<Expression>,
IndexedVector<NamedExpression>> body;

inline auto &getUnparsed() { return std::get<UnparsedAnnotation>(body); }
inline const auto &getUnparsed() const { return std::get<UnparsedAnnotation>(body); }
inline auto &getExpr() { return std::get<ExpressionAnnotation>(body); }
inline const auto &getExpr() const { return std::get<ExpressionAnnotation>(body); }
inline auto &getUnparsed() {
BUG_CHECK(annotationKind() == Kind::Unparsed, "Annotation has been parsed already.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BUG_CHECK(annotationKind() == Kind::Unparsed, "Annotation has been parsed already.");
BUG_CHECK(needsParsing(), "Annotation has been parsed already.");

return std::get<UnparsedAnnotation>(body);
}
inline const auto &getUnparsed() const {
BUG_CHECK(annotationKind() == Kind::Unparsed, "Annotation has been parsed already.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BUG_CHECK(annotationKind() == Kind::Unparsed, "Annotation has been parsed already.");
BUG_CHECK(needsParsing(), "Annotation has been parsed already.");

return std::get<UnparsedAnnotation>(body);
}
inline auto &getExpr() {
BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");
BUG_CHECK(std::holds_alternative<ExpressionAnnotation>(body) "Annotation does not contain an expression list.");

return std::get<ExpressionAnnotation>(body);
}
inline const auto &getExpr() const {
BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");
BUG_CHECK(std::holds_alternative<ExpressionAnnotation>(body), "Annotation does not contain an expression list.");

return std::get<ExpressionAnnotation>(body);
}
inline Expression getExpr(size_t idx) const {
BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");
BUG_CHECK(std::holds_alternative<ExpressionAnnotation>(body), "Annotation does not contain an expression list.");

const auto &expr = getExpr();
BUG_CHECK(idx < expr.size(), "invalid annotation expression index");
return expr[idx];
}
inline auto &getKV() { return std::get<KVAnnotation>(body); }
inline const auto &getKV() const { return std::get<KVAnnotation>(body); }
inline auto &getKV() {
BUG_CHECK(annotationKind() == Kind::StructuredKVList, "Annotation does not contain a key-value list.");
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the annotation is an unstructured kv-list (an unstructured annotation with PARSE_KV_LIST)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will add this. Are these all possible combinations?

Copy link
Contributor

Choose a reason for hiding this comment

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

When an unstructured annotation is parsed, it might be parsed into either the expr field or the kv field, or into something else, though I don't think we've run into that yet. The original intent was that a backend that needed such an annotation would define a subclass of Annotation to store the parsed version of the annotation.

Copy link
Collaborator Author

@fruffy fruffy Dec 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When do we generate a structured annotation? I have been looking at

IR::Annotation::Kind annotationKind() const {
    if (needsParsing())
        return Kind::Unparsed;
    if (!structured)
        return Kind::Unstructured;
    if (std::holds_alternative<ExpressionAnnotation>(body))
        return Kind::StructuredExpressionList;
    if (std::holds_alternative<KVAnnotation>(body))
        return Kind::StructuredKVList;

    BUG("Invalid annotation kind");
}

and it is a little misleading because an annotation can be both a Kind::StructuredKVList/Kind::StructuredExpressionList and a Kind::Unstructured. We just let unstructured dominate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Looking at https://p4.org/p4-spec/docs/P4-16-working-spec.html#sec-annotations it is possible that a compiler developers tries to parse an unstructured annotation as a KV-annotation. We do not restrict that, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like with Anton's change, one needs to check std::holds_alternative to check that it has the right kind (or does using std::get throw an exception if the wrong kind is present? That would probably be ok).

Copy link
Contributor

Choose a reason for hiding this comment

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

The structured flag is set if the code uses @foo [ ... ] or @foo { ... } and not set if it uses @foo( ... ). That's all it really tells you.

Copy link
Contributor

Choose a reason for hiding this comment

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

and it is a little misleading because an annotation can be both a

Yes, I was pretty confused by this distinction. Even more, it seems to be used just in one place – somewhere around p4 control plane. Overall, the things are multimensional:

  • Annotation could be parsed and unparsed
  • Parsed annotations could be structured and unstructured (1 dimension)
  • Parsed annotations could be expression lists and kv-lists (2 dimension)

I think it would be better to factor out everything in such way. I just do not know if there are any out of tree uses of structured annotations to get rid of annotationKind.

return std::get<KVAnnotation>(body);
}
inline const auto &getKV() const {
BUG_CHECK(annotationKind() == Kind::StructuredKVList, "Annotation does not contain a key-value list.");
return std::get<KVAnnotation>(body);
}

/// If this is true this is a structured annotation, and there are some
/// constraints on its contents.
Expand Down