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

Duplicate Errors on assignment of *T expression to ptr<T> variable #242

Closed
lenary opened this issue Mar 31, 2017 · 2 comments
Closed

Duplicate Errors on assignment of *T expression to ptr<T> variable #242

lenary opened this issue Mar 31, 2017 · 2 comments
Labels
bug This labels issues that are bugs.

Comments

@lenary
Copy link
Collaborator

lenary commented Mar 31, 2017

When converting some benchmarks, I got the following duplicated error messages, in every place where I was assigning something of type *T into a ptr<T> typed variable.

In this particular case, localnode->from_values[i] has type *double and other_value has type ptr<double>

/Users/ashe2/Code/checkedc-llvm-test-suite/MultiSource/Benchmarks/Olden/em3d/em3d.c:27:21: error: expression has no
      bounds, right-hand side of assignment expected to have bounds because the left-hand side has bounds
      other_value = localnode->from_values[i];
                    ^~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ashe2/Code/checkedc-llvm-test-suite/MultiSource/Benchmarks/Olden/em3d/em3d.c:27:21: error: expression has no
      bounds, cast to ptr<T> expects source to have bounds
      other_value = localnode->from_values[i];
                    ^~~~~~~~~~~~~~~~~~~~~~~~~

We should definitely only be emitting one of these errors, not both. I think perhaps the second one is closer to the "truth" (or a truth that reveals the fix), but we should probably be aiming for something like "expression of type *T cannot be cast to ptr<T> implicitly". I'm not sure even mentioning bounds here is useful, as ptr<T>s don't have the same notion of bounds that array_ptr<T> has.

@lenary
Copy link
Collaborator Author

lenary commented Mar 31, 2017

Follow-up:

Attempting an explicit cast shows how these errors are slightly different. I maintain having both is not useful, and I think an explicit cast should get rid of both. My understanding is that explicit casts would not be checked except if you used the special new casts we were introducing.

/Users/ashe2/Code/checkedc-llvm-test-suite/MultiSource/Benchmarks/Olden/em3d/em3d.c:27:21: error: expression has no
      bounds, right-hand side of assignment expected to have bounds because the left-hand side has bounds
      other_value = (_Ptr<double>)localnode->from_values[i];
                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/ashe2/Code/checkedc-llvm-test-suite/MultiSource/Benchmarks/Olden/em3d/em3d.c:27:35: error: expression has no
      bounds, cast to ptr<T> expects source to have bounds
      other_value = (_Ptr<double>)localnode->from_values[i];
                                  ^~~~~~~~~~~~~~~~~~~~~~~~~

@dtarditi dtarditi added the bug This labels issues that are bugs. label May 31, 2017
dtarditi added a commit to dtarditi/checkedc-clang that referenced this issue Sep 29, 2017
This addresses issue checkedc#242.  There were duplicate messages when
a value of T * is cast to a ptr variable.  One error message
was generated for the cast; anoother one was generated for the
consumer of the cast.

The fix is to base the bounds of the cast expression on the target
type of the cast when the target type is a _Ptr.  This is the
correct fix because we should be narrowing bounds anyway.
dtarditi added a commit that referenced this issue Sep 29, 2017
This addresses issue #242.  There were duplicate messages when
a value of T * is cast to a ptr variable.  One error message
was generated for the cast.  Another one was generated for the
consumer of the cast.

The fix is to determine the bounds of the cast expression by using
bounds inferred from the target type of the cast, when the target
type is a _Ptr.     This is the correct fix because we should be 
narrowing bounds anyway.

Testing:
- Added regression test case that checks that only one error message
  is issued.
@dtarditi
Copy link
Member

This is fixed.

dopelsunce pushed a commit to dopelsunce/checkedc-clang that referenced this issue Sep 28, 2020
…ckedc#242)

This change addresses the suggestion from @mwhicks1 in issue checkedc#235 that we relax the bounds checking for null-terminated arrays to allow a write of 0 at the upper bound of a null-terminated array.  There will be a matching pull request in the checked-clang repo for compiler change.  

- Update the description of bounds checking in the Checked C specification.
- Add tests to nullterm_pointers.c that check this behavior.
-  Update the sample code in string-helpers.c to delete some special-case code in the "squeeze" function.  This function removes all occurrences of a character from a string.  If no characters are removed, it redundantly overwrites the existing null-terminator.  Before, this had to be special-cased to avoid an out-of-bounds write.   I also updated the sample code to avoid declaring default bounds of count(0) and clarified the comments.

I ran into a subtlety when revising the specification.  When we allow a 0 to be written at the upper bound, we have to expand the allowed range of memory to include the memory at the upper bound.  It is possible for even this expanded range to be empty.   The specification is careful to not allow a write of 0 in this situation., by including a comparison of the pointer address against the lower bound.
mgrang pushed a commit that referenced this issue Nov 23, 2020
sulekhark pushed a commit that referenced this issue Feb 27, 2021
This test started failing due to issue microsoft#870 which was later
fixed by PR microsoft#899. That pull request is now merged into our fork,
so this test should be passing again. Another bug slipped into this test
in PR #242 that caused constant array sizes to be dropped in compound
literal expressions. This has also been fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This labels issues that are bugs.
Projects
None yet
Development

No branches or pull requests

2 participants