Skip to content

[clang] Fix 71315698c9 in presence of incomplete types #114095

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.

Sign up for GitHub

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 1 commit into from
Oct 30, 2024
Merged

[clang] Fix 71315698c9 in presence of incomplete types #114095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 8 additions & 3 deletions clang/lib/Sema/SemaChecking.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8900,7 +8900,12 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
<< Call->getCallee()->getSourceRange());
else if (const auto *RT = PointeeTy->getAs<RecordType>()) {

bool IsTriviallyCopyableCXXRecord =
// FIXME: Do not consider incomplete types even though they may be
// completed later. GCC does not diagnose such code, but we may want to
// consider diagnosing it in the future, perhaps under a different, but
// related, diagnostic group.
bool MayBeTriviallyCopyableCXXRecord =
RT->isIncompleteType() ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this just makes this a 'not foolproof' diagnostic here in C++ mode, which is somewhat unfortunate. We could perhaps provide a second diagnostic (in a different group?) about using incomplete CXXRecordType types, but IDK how that use case works.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So you're basically talking about this case:

struct S;

void foo(S *s1, S *s2) {
  memcpy(s1, s2, 10);
}

struct S {
  S(const S&);
  S& operator=(const S &);
};

I'm not certain that's worth splitting off as its own diagnostic as memcpy, memset, etc all need you to pass in a size parameter and... anything the user puts there in case of an incomplete type is pretty suspect. Based on that, I think we may want the diagnostic for incomplete types, but that the diagnostic only makes sense in C++ regardless of whether the type is complete or not.

However, GCC silences the diagnostic for incomplete types: https://godbolt.org/z/bsPnrKf19 so I may be missing some use case where this construct is safer than I'm thinking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

According to https://godbolt.org/z/xcnb154de having fields that point to incomplete types does not make you non trivially copyable, so I'm not even sure it reduces the quality of the original warning.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to https://godbolt.org/z/xcnb154de having fields that point to incomplete types does not make you non trivially copyable, so I'm not even sure it reduces the quality of the original warning.

Why WOULD it? Those are pointers, which are complete types.

So you're basically talking about this case:

struct S;

void foo(S *s1, S *s2) {
  memcpy(s1, s2, 10);
}

struct S {
  S(const S&);
  S& operator=(const S &);
};

I'm not certain that's worth splitting off as its own diagnostic as memcpy, memset, etc all need you to pass in a size parameter and... anything the user puts there in case of an incomplete type is pretty suspect. Based on that, I think we may want the diagnostic for incomplete types, but that the diagnostic only makes sense in C++ regardless of whether the type is complete or not.

However, GCC silences the diagnostic for incomplete types: https://godbolt.org/z/bsPnrKf19 so I may be missing some use case where this construct is safer than I'm thinking.

Yeah, something like that. It ends up causing someone to be able to 'side-step' this diagnostic inadvertently by using an incomplete type. So a separate warning of, "you're using an incomplete type here that might be non-trivially-copyable in the future ya know!" might be useful to some, though if we put it in the same group as this one, it might cause folks to throw the baby with the bath water.

More 'mulling' than having a request.

Copy link
Collaborator

@AaronBallman AaronBallman Oct 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Trivial copyability is about whether you can copy the object itself (aka, its members), and you can't create an object of incomplete class type or which contains an object of incomplete class type. So a pointer to an incomplete type is fine in that regard; it's just a pointer, you're not dereferencing it. However, for this diagnostic, memcpy and friends are touching the bytes of the object (so it's a dereference, effectively), and passing a pointer to an incomplete type means those are touching bytes of an unknown type which may be completed later.

Edit: hah, it seems Erich posted something similar while I was typing my comment.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should I mention that as a comment in the code?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should I mention that as a comment in the code?

There were a few things, what do you mean by 'that'? Perhaps something that "note, this makes the diagnostic imperfect because... but shrug"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Something along those lines?

FIXME: Do not consider incomplete type even though they may be completed later.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can live with that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Something along those lines?

FIXME: Do not consider incomplete type even though they may be completed later.

I'd prefer something more like:

FIXME: Do not consider incomplete types even though they may be completed later. GCC does
not diagnose such code, but we may want to consider diagnosing it in the future, perhaps under
a different, but related, diagnostic group.

so it's more clear why it's a fixme. But yeah, I can live with that.

RT->desugar().isTriviallyCopyableType(Context);

if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
Expand All @@ -8910,7 +8915,7 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
<< ArgIdx << FnName << PointeeTy << 0);
SearchNonTrivialToInitializeField::diag(PointeeTy, Dest, *this);
} else if ((BId == Builtin::BImemset || BId == Builtin::BIbzero) &&
!IsTriviallyCopyableCXXRecord && ArgIdx == 0) {
!MayBeTriviallyCopyableCXXRecord && ArgIdx == 0) {
// FIXME: Limiting this warning to dest argument until we decide
// whether it's valid for source argument too.
DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
Expand All @@ -8923,7 +8928,7 @@ void Sema::CheckMemaccessArguments(const CallExpr *Call,
<< ArgIdx << FnName << PointeeTy << 1);
SearchNonTrivialToCopyField::diag(PointeeTy, Dest, *this);
} else if ((BId == Builtin::BImemcpy || BId == Builtin::BImemmove) &&
!IsTriviallyCopyableCXXRecord && ArgIdx == 0) {
!MayBeTriviallyCopyableCXXRecord && ArgIdx == 0) {
// FIXME: Limiting this warning to dest argument until we decide
// whether it's valid for source argument too.
DiagRuntimeBehavior(Dest->getExprLoc(), Dest,
Expand Down
2 changes: 0 additions & 2 deletions clang/test/SemaCXX/constexpr-string.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,8 +670,6 @@ namespace MemcpyEtc {
constexpr bool test_address_of_incomplete_struct_type() { // expected-error {{never produces a constant}}
struct Incomplete;
extern Incomplete x, y;
// expected-warning@+2 {{first argument in call to '__builtin_memcpy' is a pointer to non-trivially copyable type 'Incomplete'}}
// expected-note@+1 {{explicitly cast the pointer to silence this warning}}
__builtin_memcpy(&x, &x, 4);
// expected-note@-1 2{{cannot constant evaluate 'memcpy' between objects of incomplete type 'Incomplete'}}
return true;
Expand Down
25 changes: 21 additions & 4 deletions clang/test/SemaCXX/warn-memaccess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,17 @@ extern "C" void *memcpy(void *s1, const void *s2, unsigned n);

class TriviallyCopyable {};
class NonTriviallyCopyable { NonTriviallyCopyable(const NonTriviallyCopyable&);};
struct Incomplete;

void test_bzero(TriviallyCopyable* tc,
NonTriviallyCopyable *ntc) {
NonTriviallyCopyable *ntc,
Incomplete* i) {
// OK
bzero(tc, sizeof(*tc));

// OK
bzero(i, 10);

// expected-warning@+2{{first argument in call to 'bzero' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
// expected-note@+1{{explicitly cast the pointer to silence this warning}}
bzero(ntc, sizeof(*ntc));
Expand All @@ -22,10 +27,14 @@ void test_bzero(TriviallyCopyable* tc,
}

void test_memset(TriviallyCopyable* tc,
NonTriviallyCopyable *ntc) {
NonTriviallyCopyable *ntc,
Incomplete* i) {
// OK
memset(tc, 0, sizeof(*tc));

// OK
memset(i, 0, 10);

// expected-warning@+2{{first argument in call to 'memset' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
// expected-note@+1{{explicitly cast the pointer to silence this warning}}
memset(ntc, 0, sizeof(*ntc));
Expand All @@ -36,10 +45,14 @@ void test_memset(TriviallyCopyable* tc,


void test_memcpy(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1) {
NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1,
Incomplete *i0, Incomplete *i1) {
// OK
memcpy(tc0, tc1, sizeof(*tc0));

// OK
memcpy(i0, i1, 10);

// expected-warning@+2{{first argument in call to 'memcpy' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
// expected-note@+1{{explicitly cast the pointer to silence this warning}}
memcpy(ntc0, ntc1, sizeof(*ntc0));
Expand All @@ -52,10 +65,14 @@ void test_memcpy(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
}

void test_memmove(TriviallyCopyable* tc0, TriviallyCopyable* tc1,
NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1) {
NonTriviallyCopyable *ntc0, NonTriviallyCopyable *ntc1,
Incomplete *i0, Incomplete *i1) {
// OK
memmove(tc0, tc1, sizeof(*tc0));

// OK
memmove(i0, i1, 10);

// expected-warning@+2{{first argument in call to 'memmove' is a pointer to non-trivially copyable type 'NonTriviallyCopyable'}}
// expected-note@+1{{explicitly cast the pointer to silence this warning}}
memmove(ntc0, ntc1, sizeof(*ntc0));
Expand Down
Loading