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.

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 mechanism for "non-fatal assertions" in 3C code #745

Open
mattmccutchen-cci opened this issue Nov 18, 2021 · 0 comments
Open

Add mechanism for "non-fatal assertions" in 3C code #745

mattmccutchen-cci opened this issue Nov 18, 2021 · 0 comments

Comments

@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Nov 18, 2021

Problem statement

In #657, I identified several cases (1, 2, 3, 4, 5, 6) that I didn't think could happen, so I didn't want to spend time thinking about what might constitute "correct" 3C behavior in those cases, but there was a reasonably natural approach to recover and allow 3C to continue running (although the output might be incorrect). I hesitated to put in a normal assert call because it might unnecessarily block end users from making progress on their work when 3C otherwise might have a good chance of producing mostly correct output. But if the unexpected case isn't reported at all, there's a risk that the user wouldn't notice any problems in the output, and even if they do notice, they might have a hard time finding the root cause of the problems. And if we're lucky enough that an "unexpected case" that can actually happen is revealed by our tests (regression, benchmark, or any other kinds of tests we might add in the future), we'd like to know about it; the easiest way to achieve that is probably just to make the test fail.

Ideally, 3C should have a mechanism for reporting such "unexpected cases" that meets all of the above design goals. I'll call that mechanism a "non-fatal assertion". This issue tracks addition of the non-fatal assertion API and addition of as many non-fatal assertions to the existing 3C code (or conversion of existing fatal assertions to non-fatal) as we see fit at that time.

Behavior on failure

In our tests, we'd generally want a non-fatal assertion to cause a nonzero exit code in order to make the test fail, though there might be some exceptions. In end-user builds, it's debatable whether a non-fatal assertion should cause a nonzero exit code by default; whichever default we choose, we could offer a command-line option for the other behavior. If the default is a nonzero exit code, then that option would be a "downgrade" like the existing -allow-* options. We could decide whether non-fatal assertions cause a nonzero exit code by default based on our experience of how often they indicate a problem serious enough to justify blocking the user's workflow until the user reviews them and uses a downgrade option if desired. Of course, the 3C code will likely have many different non-fatal assertions, some of which may be more serious than others, so the option should probably be finer-grained than "downgrade all non-fatal assertions". We may give each non-fatal assertion a "tag" name and have the option specify a list of tags to downgrade; that's probably more manageable than defining many different command-line options. We could consider migrating the existing -allow-* options to tags in the new framework.

If we decide that certain non-fatal assertions should cause our tests to fail but shouldn't produce a nonzero exit code by default in end-user builds of 3C, we'll need to find an appropriate way to configure that behavior difference. Basing it on build flags, such as CMAKE_BUILD_TYPE (Debug or Release) or LLVM_ENABLE_ASSERTIONS or maybe a new flag just for this purpose, might be convenient but inflexible. Using a 3C command-line option in the tests would be more flexible but would add a huge amount of clutter to the RUN lines in the regression tests and might be easy to forget, though a change like #355 would mitigate these problems to some extent. If we're OK with end-user builds having the same default behavior as our tests, then we won't have to deal with this.

API design considerations

We'll have to decide on the API to perform a non-fatal assertion. Of course, there's a trade-off between making non-fatal assertions easier to write so we can use them more liberally and providing more useful functionality when they fail: mainly, additional information to identify the problem (e.g., source locations) and tag names to support finer-grained downgrades. On one extreme, we could have a function nonfatal_assert(condition) that returns the value of condition (for use in an if statement for the recovery path), but if condition is false, nonfatal_assert additionally prints a message and sets a global variable that affects the exit code; this would be as easy to use as a traditional assert(condition). On the other extreme, we could use Clang diagnostics and make a simple wrapper around reportCustomDiagnostic that adds any functionality we want, such as tags. That might require a lot of code at each call site. It would also require access to a DiagnosticsEngine, which may not be readily available at every call site, though we may be able to save the current DiagnosticsEngine in a global variable at some appropriate point in a high-level function call in the 3C code.

Is there prior art in the LLVM monorepo or even in other open-source projects that we could learn from?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant