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

Fix compiler crash reported by user. #488

Merged
merged 2 commits into from May 10, 2018
Merged

Fix compiler crash reported by user. #488

merged 2 commits into from May 10, 2018

Conversation

dtarditi
Copy link
Contributor

@dtarditi dtarditi commented May 10, 2018

This fixes issue #487, a debug compiler crash reported by a user. In
the last change to SemaBounds.cpp, a call to
MakeAssignmentImplicitCastExplicit was removed from the code that
substitutes argument expressions into parameter bounds expressions.
This led to a subtle problem. The substitution code uses
lib/Sema/TreeTransform. When an argument expression was an implicit
cast expression, TreeTransform modified it. The argument expression
is used elsewhere in the AST, so this is incorrect.

TreeTransform assumes that implicit casts are being regenerated by
semantic analysis and that it is OK to modify them. During
substitution, it rebuilds the bounds expression. This calls semantic
analysis, which may call ImpCastExprToType. This implicitly casts an
expression to a new type. If the expression is already an implicit
cast expression, it modifies the expression. The fix is to convert the
argument expression to an explicit cast expression when it is
substituted.

Testing:

  • Added two regression cases: the user-reported case and a
    cut-down version of the user code.
  • Passing local testing on Windows. Problem no longer occurs.

This fixes issue #487, a debug compiler crash reported by a user.  In
the last change to SemaBounds.cpp, a call to
MakeAssignmentImplicitCastExplicit was removed from the code that
substitutes argument expressions into parameter bounds expressions.
This led to a subtle problem.  The substitution code uses
lib/Sema/TreeTransform.  When an argument expression was an implicit
cast expression, TreeTransform modified it.  The argument expression
is used elsewhere in the AST, so this is incorrect.

TreeTransform assumes that implicit casts are being regenerated by
semantic analysis and that it is OK to modify them.  During
substitution, it rebuilds the bounds expression.  This calls semantic
analysis, which may call ImpCastExprToType. This implicitly casts an
expression to a new type.  If the expression is already an implicit
cast expression, it modifies the expression. The fix is to convert the
argument expression to an explicit cast expression when it is
substituted.

Testing:
- Added two regression cases: the user-reported case and a
cut-down version of the user code.
@dtarditi dtarditi merged commit 76dc14d into master May 10, 2018
@dtarditi dtarditi deleted the issue487 branch May 22, 2018 17:05
sulekhark pushed a commit that referenced this pull request Jul 8, 2021
* prototype without Diagnostics

* enableSourceFileDiagnostics

* only enable diags once per ast

* add fail case

* FileEntry path before CanonicalFilePath

* Diagnostic errors cause non-zero exit code

* setup multipass verifier

* remove unused prefixes

* delete unused code; create ASTs in seperate function

* rework canonical file path

* add test refactoring TODOs

* :disable diag_verifier tests

* Wording

Co-authored-by: John Kastner <john@correctcomputation.com>

* Slight improvements and comments for file path canonicalization (for possible addition to #488) (#508)

* Slight improvements and comments for file path canonicalization.

In particular, getCanonicalFilePath (the version that asserts) is no
longer used.

* Add comments about blocks around diagnostic generation.

Co-authored-by: John Kastner <john@correctcomputation.com>
Co-authored-by: Matt McCutchen (Correct Computation) <matt@correctcomputation.com>
sulekhark pushed a commit that referenced this pull request Jul 8, 2021
PR #488 made `3c -verify` cover only the compiler diagnostics, but none
of our regression tests actually use that functionality. Instead, one
regression test (macro_function_call) used `3c -verify` to try to test
the absence of 3C warnings, and we were unaware that the test wasn't
testing what it was supposed to. I think it's best to make `3c -verify`
an error for now so we don't make that mistake again.
sulekhark pushed a commit that referenced this pull request Jul 8, 2021
* Reverse name of 3c -disccty option to reflect what it actually does.

By default, 3c adds the -f3c-tool compiler option to disable the Checked
C type checker. 3c's -disccty option (apparently an abbreviation for
"disable Checked C type checker") made 3c _not_ pass -f3c-tool, hence
leaving the type checker _enabled_. 3C's internal flag variable,
DisableCCTypeChecker, was also backwards, though interestingly, the
`3c -help` text was correct ("Do not disable checked c type checker").

This commit renames -disccty to -enccty and renames DisableCCTypeChecker
to EnableCCTypeChecker. We take the opportunity to further improve the
`3c -help` text too.

* Completely remove the old `3c -verify` option.

Now that 3C only runs one compiler "invocation" per translation unit,
with the new diagnostic verifier implementation, callers will be able to
just use the `-Xclang -verify` compiler option. And given that we may
want to use other diagnostic verifier options (e.g., `-verify=PREFIX` or
`-verify-ignore-unexpected`), let's standardize on using the original
compiler options rather than trying to define `3c` options wrapping all
of them.

* Switch from ClangTool::buildASTs to a custom _3CASTBuilderAction.

Replace the existing ArgumentsAdjusters with modifications to the option
data structures in _3CASTBuilderAction. More will be added to
_3CASTBuilderAction in the subsequent commits.

Fixes #536.

* Canonicalize -I directory paths in _3CASTBuilderAction.

This works per translation unit, so hopefully it is a complete and
correct fix to #515 that benefits all callers of 3C.

Remove the previous crude workaround from convert_project.

Fixes #515.

* Make diagnostic verification work after the redesign of #488.

Re-enable diagnostic verification in regression tests where it still
succeeds. It remains disabled in some tests in which failures were
introduced while it was disabled; #609 is for that.

Fixes #503.

* Migrate the difftypes* tests to the diagnostic verifier.

This feels a little cleaner than manually scanning the stderr and lets
us check the diagnostics more precisely, for what that's worth. We
couldn't do this before #488 both because 3C exited before the
diagnostics could be verified and because 3C didn't support custom
verify prefixes. #488 solved the first problem, and this PR solves the
second.

* Document diagnostic verification in 3C's CONTRIBUTING.md.
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

Successfully merging this pull request may close these issues.

None yet

1 participant