[P4Testgen] Fix behavior of coverage on edge cases with no nodes to cover #4275

vlstill · 2023-12-05T08:07:44Z

This PR changes behavior of testgen on various edge cases with regard to node coverage:

If there are no coverable nodes, then set coverage to 100 %.
- This could be a bit controversial, but I'd say it makes sense to have an empty program be considered fully covered. It does get a bit more tricky if --track-coverage TABLE_ENTRIES is enabled (without STATEMENTS) and there are no tables.
the --assert-min-coverage can no longer be enabled unless --track-coverage is enabled as it does not make sense without anything to cover.
The test "banner" is now different for the case the coverage is disabled and for the case coverage is enabled and there are no coverable nodes:
- ============ Test 0 ============ (coverage disabled)
- ============ Test 0: No coverable nodes ============

vlstill · 2023-12-05T08:09:02Z

midend/coverage.h

+
+    /// @returns true if and only if at least one of the coverage-collection options (cover*) is
+    /// enabled
+    bool coverageEnabled() const { return coverStatements || coverTableEntries; }


I've opted to add this getter to make it more likely this will be properly updated when new coverage types are added.

Instead, I would add an option to CoverageOptions that sets this value true, whenever one of the values is set.

I am not sure I understand what you mean. Do you suggest to have a derived boolean argument in the struct? That would seem like needless duplication of information at risk of inconsistency to me.

Yes, set the value here: https://github.com/p4lang/p4c/blob/main/backends/p4tools/modules/testgen/options.cpp#L256

You can easily precompute the boolean value here. It will also automatically pick up any new coverage option that might be added. This way you do not need to hard-code all the possible coverage options in the helper.

I don't like that all that much. The CoverageOptions struct is not strictly bound to be filled by options only, it is not even defined in the headers that process options. Therefore, having a derived attribute adds an invariant to the class that needs to hold when the class values are modified/set. Having the getter needs an invariant too (that it ORs all the cover* values), but this one is at least localized to the class itself.

You could also add this flag to the testgen options class instead of the options structure.

OK, let's do it that way. Although I still think having the member function did make sense.

fruffy · 2023-12-05T13:26:40Z

backends/p4tools/modules/testgen/testgen.cpp

@@ -115,6 +115,14 @@ int Testgen::mainImpl(const IR::P4Program *program) {
    if (seed) {
        printFeature("test_info", 4, "============ Program seed %1% =============\n", *seed);
    }
+    if (testgenOptions.minCoverage > 0 && !testgenOptions.coverageOptions.coverageEnabled()) {


This is something I would catch in the options early, if possible.

This seemed quite early to me, but I've now moved it even earlier just after option processing.

Options should be processed as early as possible to get immediate feedback. Otherwise, you run your tests for a couple of minutes then get this error.

We typically validate options already for each lambda we add. For example, here you could throw an error if you want to enable minCoverage without previously enable coverage in some way. This way you also do not require an extra validate call. This requires ordering, but that seems fine to me. This is a limitation of the way our options work.

I am also okay with adding validate, but not sure how much usage that function call will see.

I don't like requiring one to be set before another, that is why I did not want to put the validation into the lambdas.

backends/p4tools/modules/testgen/lib/test_backend.cpp

fruffy · 2023-12-05T13:30:11Z

midend/coverage.h

+
+    /// @returns true if and only if at least one of the coverage-collection options (cover*) is
+    /// enabled
+    bool coverageEnabled() const { return coverStatements || coverTableEntries; }


Instead, I would add an option to CoverageOptions that sets this value true, whenever one of the values is set.

fruffy · 2023-12-07T16:45:38Z

backends/p4tools/modules/testgen/options.cpp

+            "It is not allowed to have --assert-min-coverage set to non-zero without a coverage "
+            "tracking enabled with --track-coverage option. Without coverage tracking, the "
+            "--assert-min-coverage is meaningless.");
+        FATAL_ERROR("Incompatible options given.");


I approved but I do not think this is necessary. We will exist if we encounter an error during processing. If not, that is a bug.

Seems like the check is not early enough and before the compiler context is set it is not possible to check error count either.

tests/empty_1/p4/main.p4:19: fatal error: when writing output to : Broken pipe 19 | | compilation terminated.

Hmm that is a little odd, we have been using ::error just fine in the options classes and the compiler context is set up before the validate call.

The entire way options are handled in P4Testgen is a little funky (there are a bunch of hacks to set up context). So there might be something awry there.

Maybe the exception thrown with FATAL_ERROR causes the problem? We have not tested any exception handling that early.

I think the handler for errors in the option parsing works differently, it also prints the usage if the argument parsing fails. What I was testing (and what produced this weird error) was just having the error there and no FATAL_ERROR, so there were no exceptions thrown.

Did this get resolved?

Unless we change error handling during process, the FATAL_ERROR is the best bet to cleanly end the processing (in the compiler, there are only warnings, so this does not matter).

I looked into this, basically we need to do this

if (::errorCount() > 0) { return std::nullopt; }

after the validate call to not return a broken compile context. We could also return true/false in validate.

Made it return. ::errorCount() is not available before the context is set (it could be extracted from the context, but this seems cleaner to me).

vlstill · 2023-12-11T13:34:53Z

Rebased + using the same name as is used in the ParserOptions (predecessor of CompilerOptions) for this purpose (it is not a base class, so the defintions are independent).

…ing coverable with coverage enabled

Co-authored-by: Fabian Ruffy <5960321+fruffy@users.noreply.github.com>

vlstill commented Dec 5, 2023

View reviewed changes

vlstill marked this pull request as ready for review December 5, 2023 08:12

vlstill requested a review from fruffy December 5, 2023 08:12

vlstill changed the title ~~[P4Testgen] Fix behaviour of coverage on edge cases with no nodes to cover~~ [P4Testgen] Fix behavior of coverage on edge cases with no nodes to cover Dec 5, 2023

fruffy requested changes Dec 5, 2023

View reviewed changes

fruffy added the p4tools Topics related to the P4Tools back end label Dec 6, 2023

vlstill requested a review from fruffy December 7, 2023 13:25

fruffy approved these changes Dec 7, 2023

View reviewed changes

fruffy reviewed Dec 7, 2023

View reviewed changes

vlstill force-pushed the testgen-cov-empty branch from bad034d to 67bc233 Compare December 11, 2023 13:34

vlstill force-pushed the testgen-cov-empty branch from 67bc233 to d57ba39 Compare December 11, 2023 16:25

vlstill requested a review from fruffy December 11, 2023 17:52

vlstill and others added 11 commits December 15, 2023 14:52

Set coverage to 100 % if there are no nodes to cover

1b840ec

Add coverageEnabled() to CoverageOptions struct

49310bb

Prohibit --assert-min-coverage without --track-coverage

7a8cbc5

Differenciate between output with coverage disable and output on noth…

f1b8417

…ing coverable with coverage enabled

Update backends/p4tools/modules/testgen/lib/test_backend.cpp

5ea1e0a

Co-authored-by: Fabian Ruffy <5960321+fruffy@users.noreply.github.com>

Add a hook for validating P4Tools options

7340dc1

Move testgen options validation earlier

04e1e5e

Fix clang-format

a9569aa

Move the generic coverage-tracking indicator to testgen options

b80a2d7

Use the same name for validator function as is in ParserOptions

fde885d

Avoid FATAL_ERROR in option validator

485812a

vlstill force-pushed the testgen-cov-empty branch from 995a62a to 485812a Compare December 15, 2023 13:52

vlstill merged commit 2d7550a into p4lang:main Dec 17, 2023
13 checks passed

vlstill deleted the testgen-cov-empty branch December 17, 2023 15:05

fruffy mentioned this pull request Jan 12, 2024

[P4Testgen] Validate options as early as possible #4333

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[P4Testgen] Fix behavior of coverage on edge cases with no nodes to cover #4275

[P4Testgen] Fix behavior of coverage on edge cases with no nodes to cover #4275

vlstill commented Dec 5, 2023

vlstill Dec 5, 2023

fruffy Dec 5, 2023

vlstill Dec 5, 2023

fruffy Dec 5, 2023

vlstill Dec 6, 2023

fruffy Dec 6, 2023

vlstill Dec 7, 2023

fruffy Dec 5, 2023

vlstill Dec 5, 2023

fruffy Dec 5, 2023 •

edited

Loading

vlstill Dec 6, 2023

fruffy Dec 5, 2023

fruffy Dec 7, 2023

vlstill Dec 11, 2023

fruffy Dec 11, 2023

fruffy Dec 11, 2023

vlstill Dec 11, 2023

fruffy Dec 11, 2023

vlstill Dec 12, 2023

fruffy Dec 12, 2023

vlstill Dec 15, 2023

vlstill commented Dec 11, 2023

[P4Testgen] Fix behavior of coverage on edge cases with no nodes to cover #4275

[P4Testgen] Fix behavior of coverage on edge cases with no nodes to cover #4275

Conversation

vlstill commented Dec 5, 2023

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

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

fruffy Dec 5, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlstill commented Dec 11, 2023

fruffy Dec 5, 2023 •

edited

Loading