Add checks to annotation getters. #5052

fruffy · 2024-12-05T00:25:49Z

Provide more useful feedback when trying to retrieve content from an annotation the wrong way.

ChrisDodd · 2024-12-05T00:35:00Z

ir/base.def

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


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

Will add this. Are these all possible combinations?

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.

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.

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?

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

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.

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.

ChrisDodd · 2024-12-05T02:24:02Z

Might it be better to write these as

try {
    return std::get<...>(body);
} catch (std::bad_variant_access) {
    BUG("Annotation contains incorrect type");
}

Not a style we currently use, but perhaps we should go that way?

fruffy · 2024-12-05T19:06:22Z

Might it be better to write these as
try {
    return std::get<...>(body);
} catch (std::bad_variant_access) {
    BUG("Annotation contains incorrect type");
}
Not a style we currently use, but perhaps we should go that way?

Not opposed to it... is the hope better performance?

asl · 2024-12-05T19:09:23Z

ir/base.def

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


Suggested change

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

BUG_CHECK(needsParsing(), "Annotation has been parsed already.");

asl · 2024-12-05T19:09:34Z

ir/base.def

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


Suggested change

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

BUG_CHECK(needsParsing(), "Annotation has been parsed already.");

asl · 2024-12-05T19:10:03Z

ir/base.def

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


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

asl · 2024-12-05T19:10:27Z

ir/base.def

+        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.");


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

asl · 2024-12-05T19:11:17Z

ir/base.def

    inline Expression getExpr(size_t idx) const {
+        BUG_CHECK(annotationKind() == Kind::Unstructured || annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");


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

Signed-off-by: fruffy <fruffy@nyu.edu>

asl

Thanks!

fruffy added the core Topics concerning the core segments of the compiler (frontend, midend, parser) label Dec 5, 2024

fruffy requested a review from asl December 5, 2024 00:25

ChrisDodd reviewed Dec 5, 2024

View reviewed changes

fruffy linked an issue Dec 5, 2024 that may be closed by this pull request

std::get is not guarded in the IR::Annotation getters #5050

Closed

asl reviewed Dec 5, 2024

View reviewed changes

fruffy marked this pull request as ready for review December 11, 2024 14:19

fruffy added 3 commits December 11, 2024 14:19

Add annotation error messages.

30ff78d

Signed-off-by: fruffy <fruffy@nyu.edu>

Check for KVAnnotation instead of deferring to annotationKind().

2fb857e

Signed-off-by: fruffy <fruffy@nyu.edu>

Use try-catch instead of BUG_CHECK.

8390080

Signed-off-by: fruffy <fruffy@nyu.edu>

fruffy force-pushed the fruffy/annotation_error_messages branch from df09a6a to 8390080 Compare December 11, 2024 14:19

fruffy requested a review from asl December 12, 2024 11:10

asl approved these changes Dec 12, 2024

View reviewed changes

fruffy added this pull request to the merge queue Dec 12, 2024

Merged via the queue into main with commit b0f775c Dec 12, 2024
19 checks passed

fruffy deleted the fruffy/annotation_error_messages branch December 12, 2024 20:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add checks to annotation getters. #5052

Add checks to annotation getters. #5052

fruffy commented Dec 5, 2024 •

edited

Loading

ChrisDodd Dec 5, 2024

fruffy Dec 5, 2024

ChrisDodd Dec 5, 2024

fruffy Dec 5, 2024 •

edited

Loading

fruffy Dec 5, 2024

ChrisDodd Dec 5, 2024

ChrisDodd Dec 5, 2024

asl Dec 5, 2024

ChrisDodd commented Dec 5, 2024

fruffy commented Dec 5, 2024

asl Dec 5, 2024

asl Dec 5, 2024

asl Dec 5, 2024

asl Dec 5, 2024

asl Dec 5, 2024

asl left a comment

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

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

		inline Expression getExpr(size_t idx) const {
		BUG_CHECK(annotationKind() == Kind::Unstructured \|\| annotationKind() == Kind::StructuredExpressionList, "Annotation does not contain an expression list.");

Add checks to annotation getters. #5052

Add checks to annotation getters. #5052

Conversation

fruffy commented Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fruffy Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDodd commented Dec 5, 2024

fruffy commented Dec 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asl left a comment

Choose a reason for hiding this comment

fruffy commented Dec 5, 2024 •

edited

Loading

fruffy Dec 5, 2024 •

edited

Loading