Skip to content

Bugfix#349 #361

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

Merged
merged 6 commits into from
Jan 7, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion clang/lib/3C/ConstraintVariables.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,6 @@ PointerVariableConstraint::PointerVariableConstraint(
QualType QTy = QT;
const Type *Ty = QTy.getTypePtr();
auto &CS = I.getConstraints();
typedeflevelinfo = TypedefLevelFinder::find(QT);
// If the type is a decayed type, then maybe this is the result of
// decaying an array to a pointer. If the original type is some
// kind of array type, we want to use that instead.
Expand Down Expand Up @@ -253,6 +252,8 @@ PointerVariableConstraint::PointerVariableConstraint(
}
}

typedeflevelinfo = TypedefLevelFinder::find(QTy);

bool VarCreated = false;
bool IsArr = false;
bool IsIncompleteArr = false;
Expand Down
5 changes: 2 additions & 3 deletions clang/test/3C/hash.c
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// RUN: 3c -alltypes %s | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this test not passing at all, before? I see you have not changed the expectation of what -alltypes will produce, in terms of changing the source file. So I'm confused how you are testing the change that you made to ConstraintVariables.cpp.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was one of the tests that I previously identified for not running clang on the output of 3c. It turns out, that would cause an error, explained in more detail with simplified example in issue #349. This change was verified by the addition of the compilation line, and the lack of effect on the suite of regression tests. I was hesitant to change the test in other ways, since I didn't write it or analyze why it exists. But I can add a "CHECKALL" line at the location that allowed it to compile.


// Currently not possible to run clang on the output,
// there is an error reported at https://github.com/correctcomputation/checkedc-clang/issues/349
// RUN: 3c -alltypes %s | %clang -f3c-tool -c -fcheckedc-extension -x c -o %t1.unused -

/*
* Based on hash.c in Very Secure FTPd
Expand All @@ -25,6 +23,7 @@ extern int memcmp(const void *src1 : byte_count(n), const void *src2 : byte_coun

_Itype_for_any(T) void
vsf_sysutil_memclr(void* p_dest : itype(_Array_ptr<T>) byte_count(size), unsigned int size)
// CHECK_ALL: vsf_sysutil_memclr(_Array_ptr<T> p_dest : itype(_Array_ptr<T>) byte_count(size), unsigned int size)
{
/* Safety */
if (size == 0)
Expand Down
20 changes: 17 additions & 3 deletions clang/test/3C/type_params.c
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: 3c -addcr -alltypes %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s
// RUN: 3c -addcr %s -- | FileCheck -match-full-lines -check-prefixes="CHECK_NOALL","CHECK" %s
// RUN: 3c -addcr %s -- | %clang -c -fcheckedc-extension -x c -o /dev/null -
// RUN: 3c -addcr -alltypes %s | FileCheck -match-full-lines -check-prefixes="CHECK_ALL","CHECK" %s
// RUN: 3c -addcr %s | FileCheck -match-full-lines -check-prefixes="CHECK_NOALL","CHECK" %s
// RUN: 3c -addcr %s | %clang -c -fcheckedc-extension -x c -o %t1.unused -

// General demonstration
_Itype_for_any(T) void *test_single(void *a : itype(_Ptr<T>), void *b : itype(_Ptr<T>)) : itype(_Ptr<T>);
Expand Down Expand Up @@ -157,3 +157,17 @@ void *example1(void * ptr, unsigned int size) {
// CHECK: ret = realloc<void>(ptr, size);
return ret;
}

// Issue #349. Check that the parameter doesn't inherit the double pointer argument within do_doubleptr
_Itype_for_any(T)
void incoming_doubleptr(void* ptr : itype(_Array_ptr<T>)) {
// CHECK_ALL: void incoming_doubleptr(_Array_ptr<T> ptr : itype(_Array_ptr<T>)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this seems very weird to me. I'm not sure why the void * component is changing. As far as I am aware, we are assuming that ITypes will not change during conversion. Perhaps we are slowly moving away from that. @jackastner maybe you can comment?

Copy link
Collaborator

@john-h-kastner john-h-kastner Dec 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I does seem odd that this ends up with both _Array_ptr<T> and itype(_Array_ptr<T>)). For the most part, any itypes in the source code should be copied unmodified to the output.

I think the fix this PR makes means that the type of ptr is _Array_ptr<T> rather than _Array_ptr<_Ptr<T>>. This is definitely the better rewriting, but we might want to avoid rewriting the type altogether.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, before the fix it was being rewritten with two pointer levels. But it is safe, so shouldn't it be rewritten with the new type and no Itype? Or is that just how it will be done with the liberal itypes enhancement?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things: (1) It doesn't make sense to have a checked pointer to the left of the colon, on an itype. Having T: itype(T) ought to just be T. (2) We have not been, by default, rewriting pointers in itypes that appear in the source. I think we should stick with this for now. We can consider what changes make sense in a broader issue/PR later, that is disentangled from the issue here (wrong instantiations, and multiple levels of abstraction).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I misunderstood the bug. In regards to (2), this bugfix does not add the rewriting, it only makes it correct. It was being rewritten in the test originally, but there was no CHECK line to verify this. I can look into the rewrite code to try to prevent the rewriting. I assume... anytime PointerVariableConstraint::ItypeStr is non-empty?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. It shouldn't have been getting rewritten, I don't believe. So maybe that's a bug in addition? Are these bugs something one can tease apart?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you'll accept separate bugfixes for the two issues mentioned above, than this one as-is fixes the correctness issue, and is ready to merge pending any conflicts with code merged in the mean time. I'll make a new issue for the duplicated type, which is inconvenient but compiles correctly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Just to get it on the record: By "duplicated type" you mean that void * is being rewritten to _Array_ptr<T> which duplicates the type in : itype(_Array_ptr<T>) on the same line. The root type (here, void *) should not get rewritten, and that's a separate issue we can tackle (#367).

return;
}

void do_doubleptr(int count) {
int **arr = malloc(sizeof(int*) * count);
// CHECK_ALL: _Array_ptr<_Ptr<int>> arr : count(count) = malloc<_Ptr<int>>(sizeof(int*) * count);
incoming_doubleptr(arr);
// CHECK_ALL: incoming_doubleptr<_Ptr<int>>(arr);
}
28 changes: 28 additions & 0 deletions clang/test/3C/type_params_xfail1.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// RUN: 3c -addcr -alltypes %s | FileCheck -match-full-lines %s

// XFAIL: *

// This fails because the type variable is used to constrain both calls to `incoming_doubleptr`.
// To be correct, the usage of a type variable should be independent at each call site.

// adapted from type_params.c
#include <stddef.h>
_Itype_for_any(T) void *malloc(size_t size) : itype(_Array_ptr<T>) byte_count(size);

_Itype_for_any(T)
void incoming_doubleptr(_Array_ptr<T> ptr : itype(_Array_ptr<T>)) {
return;
}

void do_doubleptr(int count) {
int **arr = malloc(sizeof(int*) * count);
// CHECK: _Array_ptr<_Ptr<int>> arr : count(count) = malloc<_Ptr<int>>(sizeof(int*) * count);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: This is the rewriting that should happen, but does not? What happens instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is rewritten as _Array_ptr<_Array_ptr<int>> arr, because of interference from the other function.

incoming_doubleptr(arr);
}

// adding this function changes the infered type of the previous one unnecessarily
void interfere_doubleptr(void)
{
float fl _Checked[5][5] = {};
incoming_doubleptr(fl);
}