-
Notifications
You must be signed in to change notification settings - Fork 15k
[Clang][Sema] Allow counted_by on void* in GNU mode #164737
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -132,9 +132,20 @@ bool Sema::CheckCountedByAttrOnField(FieldDecl *FD, Expr *E, bool CountInBytes, | |
| // `BoundsSafetyCheckUseOfCountAttrPtr` | ||
| // | ||
| // * When the pointee type is always an incomplete type (e.g. | ||
| // `void`) the attribute is disallowed by this method because we know the | ||
| // type can never be completed so there's no reason to allow it. | ||
| InvalidTypeKind = CountedByInvalidPointeeTypeKind::INCOMPLETE; | ||
| // `void` in strict C mode) the attribute is disallowed by this method | ||
| // because we know the type can never be completed so there's no reason | ||
| // to allow it. | ||
| // | ||
| // Exception: In GNU mode, void has an implicit size of 1 byte for pointer | ||
| // arithmetic. Therefore, counted_by on void* is allowed as a GNU extension | ||
| // and behaves equivalently to sized_by (treating the count as bytes). | ||
| bool IsVoidPtrInGNUMode = PointeeTy->isVoidType() && getLangOpts().GNUMode; | ||
| if (IsVoidPtrInGNUMode) { | ||
| // Emit a warning that this is a GNU extension | ||
| Diag(FD->getBeginLoc(), diag::ext_gnu_counted_by_void_ptr) << Kind; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should emit a note diagnostic here that suggests using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have it say what it is being interpreted as. What language would you prefer in here? I'm happy to swap it to whatever: def ext_gnu_counted_by_void_ptr
: Extension<
"'%select{counted_by|sized_by|counted_by_or_null|sized_by_or_null}0' "
"on a pointer to void is a GNU extension, treated as "
"'%select{sized_by|sized_by|sized_by_or_null|sized_by_or_null}0'">,
InGroup<GNUPointerArith>; |
||
| } else { | ||
| InvalidTypeKind = CountedByInvalidPointeeTypeKind::INCOMPLETE; | ||
| } | ||
| } else if (PointeeTy->isSizelessType()) { | ||
| InvalidTypeKind = CountedByInvalidPointeeTypeKind::SIZELESS; | ||
| } else if (PointeeTy->isFunctionType()) { | ||
|
|
@@ -272,6 +283,11 @@ GetCountedByAttrOnIncompletePointee(QualType Ty, NamedDecl **ND) { | |
| if (!PointeeTy->isIncompleteType(ND)) | ||
| return {}; | ||
|
|
||
| // If counted_by is on void*, it was already validated at declaration time | ||
| // as a GNU extension. No need to re-check GNU mode here. | ||
| if (PointeeTy->isVoidType()) | ||
| return {}; | ||
|
|
||
| return {CATy, PointeeTy}; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // RUN: %clang_cc1 -std=gnu11 -triple x86_64-unknown-linux-gnu -O2 -emit-llvm -o - %s | FileCheck %s | ||
|
|
||
| // Test that counted_by on void* in GNU mode treats void as having size 1 (byte count) | ||
|
|
||
| #define __counted_by(f) __attribute__((counted_by(f))) | ||
| #define __sized_by(f) __attribute__((sized_by(f))) | ||
|
|
||
| struct with_counted_by_void { | ||
| int count; | ||
| void* buf __counted_by(count); | ||
| }; | ||
|
|
||
| struct with_sized_by_void { | ||
| int size; | ||
| void* buf __sized_by(size); | ||
| }; | ||
|
|
||
| struct with_counted_by_int { | ||
| int count; | ||
| int* buf __counted_by(count); | ||
| }; | ||
|
|
||
| // CHECK-LABEL: define dso_local {{.*}}@test_counted_by_void( | ||
| // CHECK: %[[COUNT:.*]] = load i32, ptr %s | ||
| // CHECK: %[[NARROW:.*]] = tail call i32 @llvm.smax.i32(i32 %[[COUNT]], i32 0) | ||
| // CHECK: %[[ZEXT:.*]] = zext nneg i32 %[[NARROW]] to i64 | ||
| // CHECK: ret i64 %[[ZEXT]] | ||
| // | ||
| // Verify: counted_by on void* returns the count directly (count * 1 byte) | ||
| long long test_counted_by_void(struct with_counted_by_void *s) { | ||
| return __builtin_dynamic_object_size(s->buf, 0); | ||
| } | ||
|
|
||
| // CHECK-LABEL: define dso_local {{.*}}@test_sized_by_void( | ||
| // CHECK: %[[SIZE:.*]] = load i32, ptr %s | ||
| // CHECK: %[[NARROW:.*]] = tail call i32 @llvm.smax.i32(i32 %[[SIZE]], i32 0) | ||
| // CHECK: %[[ZEXT:.*]] = zext nneg i32 %[[NARROW]] to i64 | ||
| // CHECK: ret i64 %[[ZEXT]] | ||
| // | ||
| // Verify: sized_by on void* returns the size directly | ||
| long long test_sized_by_void(struct with_sized_by_void *s) { | ||
| return __builtin_dynamic_object_size(s->buf, 0); | ||
| } | ||
|
|
||
| // CHECK-LABEL: define dso_local {{.*}}@test_counted_by_int( | ||
| // CHECK: %[[COUNT:.*]] = load i32, ptr %s | ||
| // CHECK: %[[SEXT:.*]] = sext i32 %[[COUNT]] to i64 | ||
| // CHECK: %[[SIZE:.*]] = shl nsw i64 %[[SEXT]], 2 | ||
| // CHECK: ret i64 | ||
| // | ||
| // Verify: counted_by on int* returns count * sizeof(int) = count * 4 | ||
| long long test_counted_by_int(struct with_counted_by_int *s) { | ||
| return __builtin_dynamic_object_size(s->buf, 0); | ||
| } | ||
|
|
||
| // CHECK-LABEL: define dso_local ptr @test_void_ptr_arithmetic( | ||
| // CHECK: %[[BUF:.*]] = load ptr, ptr | ||
| // CHECK: %[[EXT:.*]] = sext i32 %offset to i64 | ||
| // CHECK: %[[PTR:.*]] = getelementptr inbounds i8, ptr %[[BUF]], i64 %[[EXT]] | ||
| // CHECK: ret ptr %[[PTR]] | ||
| // | ||
| // Verify: pointer arithmetic on void* uses i8 (byte offsets), not i32 or other sizes | ||
| void* test_void_ptr_arithmetic(struct with_counted_by_void *s, int offset) { | ||
| return s->buf + offset; // GNU extension: void* arithmetic | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| // RUN: %clang_cc1 -fexperimental-late-parse-attributes -fsyntax-only -verify %s | ||
| // RUN: %clang_cc1 -std=c11 -fexperimental-late-parse-attributes -fsyntax-only -verify %s | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clang defaults to GNU mode AFAICT, so the negative tests fail. |
||
|
|
||
| #define __counted_by(f) __attribute__((counted_by(f))) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| // RUN: %clang_cc1 -std=gnu11 -fsyntax-only -verify=expected-nowarn %s | ||
| // RUN: %clang_cc1 -std=gnu11 -Wpointer-arith -fsyntax-only -verify=expected-warn %s | ||
| // RUN: %clang_cc1 -std=c11 -fsyntax-only -verify=expected-strict %s | ||
|
|
||
| // expected-nowarn-no-diagnostics | ||
|
|
||
| #define __counted_by(f) __attribute__((counted_by(f))) | ||
| #define __counted_by_or_null(f) __attribute__((counted_by_or_null(f))) | ||
| #define __sized_by(f) __attribute__((sized_by(f))) | ||
|
|
||
| //============================================================================== | ||
| // Test: counted_by on void* is allowed in GNU mode, rejected in strict mode | ||
| //============================================================================== | ||
|
|
||
| struct test_void_ptr_gnu { | ||
| int count; | ||
| // expected-warn-warning@+2{{'counted_by' on a pointer to void is a GNU extension, treated as 'sized_by'}} | ||
| // expected-strict-error@+1{{'counted_by' cannot be applied to a pointer with pointee of unknown size because 'void' is an incomplete type}} | ||
| void* buf __counted_by(count); | ||
| }; | ||
|
|
||
| struct test_const_void_ptr_gnu { | ||
| int count; | ||
| // expected-warn-warning@+2{{'counted_by' on a pointer to void is a GNU extension, treated as 'sized_by'}} | ||
| // expected-strict-error@+1{{'counted_by' cannot be applied to a pointer with pointee of unknown size because 'const void' is an incomplete type}} | ||
| const void* buf __counted_by(count); | ||
| }; | ||
|
|
||
| struct test_volatile_void_ptr_gnu { | ||
| int count; | ||
| // expected-warn-warning@+2{{'counted_by' on a pointer to void is a GNU extension, treated as 'sized_by'}} | ||
| // expected-strict-error@+1{{'counted_by' cannot be applied to a pointer with pointee of unknown size because 'volatile void' is an incomplete type}} | ||
| volatile void* buf __counted_by(count); | ||
| }; | ||
|
|
||
| struct test_const_volatile_void_ptr_gnu { | ||
| int count; | ||
| // expected-warn-warning@+2{{'counted_by' on a pointer to void is a GNU extension, treated as 'sized_by'}} | ||
| // expected-strict-error@+1{{'counted_by' cannot be applied to a pointer with pointee of unknown size because 'const volatile void' is an incomplete type}} | ||
| const volatile void* buf __counted_by(count); | ||
| }; | ||
|
|
||
| // Verify sized_by still works the same way (always allowed, no warning) | ||
| struct test_sized_by_void_ptr { | ||
| int size; | ||
| void* buf __sized_by(size); // OK in both modes, no warning | ||
| }; | ||
|
|
||
| //============================================================================== | ||
| // Test: counted_by_or_null on void* behaves the same | ||
| //============================================================================== | ||
|
|
||
| struct test_void_ptr_or_null_gnu { | ||
| int count; | ||
| // expected-warn-warning@+2{{'counted_by_or_null' on a pointer to void is a GNU extension, treated as 'sized_by_or_null'}} | ||
| // expected-strict-error@+1{{'counted_by_or_null' cannot be applied to a pointer with pointee of unknown size because 'void' is an incomplete type}} | ||
| void* buf __counted_by_or_null(count); | ||
| }; | ||
|
|
||
| struct test_const_void_ptr_or_null_gnu { | ||
| int count; | ||
| // expected-warn-warning@+2{{'counted_by_or_null' on a pointer to void is a GNU extension, treated as 'sized_by_or_null'}} | ||
| // expected-strict-error@+1{{'counted_by_or_null' cannot be applied to a pointer with pointee of unknown size because 'const void' is an incomplete type}} | ||
| const void* buf __counted_by_or_null(count); | ||
| }; |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rapidsna What do you think about changing this to
?
This would mean in the
-fbounds-safetylanguage model we would still disallow this. Or for consistency with-fno-bounds-safetydo you think we should allow this language extension?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does
-fbounds-safetyneed this? I'd like to be bringing-fbounds-safetyincrementally to Linux...Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @kees. From my perspective, allowing
counted_byonvoid *without-fbounds-safetymeans we would have to allow it with-fbounds-safetyas well (in GNU mode), since we want the same annotated headers to be consumable by the compiler with or without-fbounds-safety(except in some inevitable cases like inline function bodies in headers).I think this is actually a reasonable compromise because treating
void *as having a stride of1is a de facto standard in C.However, in general, we cannot change the behavior of -fbounds-safety solely because the Linux community pushes back strongly. I think the issue is that they don't have the full context for -fbounds-safety, so decisions about individual attributes without that broader context can easily become opinionated (while this change isn't not unreasonable). I will be working to improve this situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, this is just my opinion. This change may lead to more of a radical shift than I initially thought because it affects the default behavior of
-fbounds-safetyin Clang (since GNU mode is on by default). So this is something I should discuss with our core-fbounds-safetyengineers before making any decisions.