[TVMScript] Avoid segfault from invalid TVMScript #17373
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tqchen
approved these changes
Sep 13, 2024
Prior to this commit, after the `DiagnosticContext` prints its error, it overwrites the `DiagnosticRenderer` with a NULL renderer. If a second call to `DiagnosticContext::Render` occurs, it will segfault. This appears to be intended to prevent double-printing of error messages, but double-printing error messages is much worse than a segfault. In addition, `DiagnosticContext::Render` should only be called once. There's a common pattern in the parser where it will wrap exceptions in `DiagnosticError`, but re-raise exceptions that are already a `DiagnosticError`. This requires every such location to include `except DiagnosticError: raise`, and can easily be missed. This PR makes two changes: First, the `DiagnosticRenderer` is updated to have a no-op callback rather than a NULL callback. Second, the re-raising of `DiagnosticError` is moved to `Parser.report_error`, so that it does not need to be handled separately at several independent locations in the TVMScript parser.
Lunderberg
force-pushed
the
relax_avoid_segfault_from_incorrect_tensor_annotation
branch
from
d62a4e9 to
ac86991
Compare
Member
|
@tvm-bot rerun |
Lunderberg
deleted the
relax_avoid_segfault_from_incorrect_tensor_annotation
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this commit, after the
DiagnosticContextprints its error, it overwrites theDiagnosticRendererwith a NULL renderer. If a second call toDiagnosticContext::Renderoccurs, it will segfault. This appears to be intended to prevent double-printing of error messages, but double-printing error messages is much worse than a segfault.In addition,
DiagnosticContext::Rendershould only be called once. There's a common pattern in the parser where it will wrap exceptions inDiagnosticError, but re-raise exceptions that are already aDiagnosticError. This requires every such location to includeexcept DiagnosticError: raise, and can easily be missed.This PR makes two changes: First, the
DiagnosticRendereris updated to have a no-op callback rather than a NULL callback. Second, the re-raising ofDiagnosticErroris moved toParser.report_error, so that it does not need to be handled separately at several independent locations in the TVMScript parser.