[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: I'll put up a new version of this PR with the memory leak fixed soon. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote:
Ok. Now I see what's happening.
These lines here are basically giving ownership of `LateParsedAttribute` to the
`LateParsedAttrList`
```
// Handle attributes with arguments that require late parsing.
LateParsedAttribute *LA =
new LateParsedAttribute(this, *AttrName, AttrNameLoc);
LateAttrs->push_back(LA);
```
However `LateParsedAttrList` is basically just a
```
class LateParsedAttrList: public SmallVector {
...
}
```
so it won't free the pointers when the list is destroyed. If we look at the C++
code that handles this we can see a bunch of manual memory management.
```
/// Parse all attributes in LAs, and attach them to Decl D.
void Parser::ParseLexedAttributeList(LateParsedAttrList , Decl *D,
bool EnterScope, bool OnDefinition) {
assert(LAs.parseSoon() &&
"Attribute list should be marked for immediate parsing.");
for (unsigned i = 0, ni = LAs.size(); i < ni; ++i) {
if (D)
LAs[i]->addDecl(D);
ParseLexedAttribute(*LAs[i], EnterScope, OnDefinition);
delete LAs[i];
}
LAs.clear();
}
```
This is a silly footgun. `LateParsedAttrList` should properly take ownership of
its pointers by deleting them when the list is destroyed.
For now we should just replicate what is done for C++ but once we're confident
we've fixed things we should make `LateParsedAttrList` properly own its
pointers.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: The leak via `clang::Parser::ParseLexedCAttribute` is ```c++ LA.Toks.push_back(AttrEnd); ``` and the leak via `clang::Parser::ParseGNUAttributes` is ``` LateParsedAttribute *LA = new LateParsedAttribute(this, *AttrName, AttrNameLoc); ``` which is really old code so I doubt this is directly responsible for the leak. I'm a little confused because LSan is telling us that these are indirect leaks which means "reachable from other leaked blocks" but LSan isn't showing any direct leaks (not reachable from anywhere). This might suggest the leak is some sort of cycle (multiple objects leaked that point to each other). https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: Hmm. Apparently there's a memory leak. https://lab.llvm.org/buildbot/#/builders/239/builds/7043 ``` -- Testing: 79948 of 79949 tests, 48 workers -- Testing: FAIL: Clang :: AST/attr-counted-by-late-parsed-struct-ptrs.c (480 of 79948) TEST 'Clang :: AST/attr-counted-by-late-parsed-struct-ptrs.c' FAILED Exit Code: 1 Command Output (stderr): -- RUN: at line 1: /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/lib/clang/19/include -nostdsysteminc -fexperimental-late-parse-attributes /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c -ast-dump | /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c + /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/clang -cc1 -internal-isystem /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/lib/clang/19/include -nostdsysteminc -fexperimental-late-parse-attributes /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c -ast-dump + /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c = ==1477834==ERROR: LeakSanitizer: detected memory leaks Indirect leak of 432 byte(s) in 2 object(s) allocated from: #0 0xbdec8684 in malloc /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:68:3 #1 0xc36532a8 in safe_malloc /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/Support/MemAlloc.h:26:18 #2 0xc36532a8 in llvm::SmallVectorBase::grow_pod(void*, unsigned long, unsigned long) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/SmallVector.cpp:143:15 #3 0xc82d9960 in grow_pod /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:151:11 #4 0xc82d9960 in grow /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:538:41 #5 0xc82d9960 in reserveForParamAndGetAddressImpl > /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:256:11 #6 0xc82d9960 in reserveForParamAndGetAddress /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:543:12 #7 0xc82d9960 in push_back /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/llvm/include/llvm/ADT/SmallVector.h:575:23 #8 0xc82d9960 in clang::Parser::ParseLexedCAttribute(clang::Parser::LateParsedAttribute&, clang::ParsedAttributes*) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:4943:11 #9 0xc82db118 in clang::Parser::ParseStructUnionBody(clang::SourceLocation, clang::TypeSpecifierType, clang::RecordDecl*) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:5128:5 #10 0xc827170c in clang::Parser::ParseClassSpecifier(clang::tok::TokenKind, clang::SourceLocation, clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, bool, clang::Parser::DeclSpecContext, clang::ParsedAttributes&) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDeclCXX.cpp:2275:7 #11 0xc82c4734 in clang::Parser::ParseDeclarationSpecifiers(clang::DeclSpec&, clang::Parser::ParsedTemplateInfo&, clang::AccessSpecifier, clang::Parser::DeclSpecContext, clang::Parser::LateParsedAttrList*, clang::ImplicitTypenameContext) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:4598:7 #12 0xc81b99dc in ParseDeclarationSpecifiers /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/include/clang/Parse/Parser.h:2495:12 #13 0xc81b99dc in clang::Parser::ParseDeclOrFunctionDefInternal(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:1153:3 #14 0xc81b8ffc in clang::Parser::ParseDeclarationOrFunctionDefinition(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /b/sanitizer-aarch64-linux-bootstrap-asan/build/llvm-project/clang/lib/Parse/Parser.cpp:1271:12 #15 0xc81b63e4 in clang::Parser::ParseExternalDeclaration(clang::ParsedAttributes&, clang::ParsedAttributes&, clang::ParsingDeclSpec*)
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote: Thank you. I wrote to the author. I hope he'll be able to come up with a change on his end. Or at least an explanation that makes sense :-) https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: @kees @bwendling @rapidsna The workaround to downgrade this error to a warning has landed https://github.com/llvm/llvm-project/commit/cef6387e52578366c2332275dad88b9953b55336 https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote: Ah! I see what you mean. I'll bring this up with the developer. (Actually, that construct makes me nervous about their code in general...) https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: @bwendling This is unfortunate ``` drivers/gpu/drm/radeon/pptable.h:442:5: error: 'counted_by' cannot be applied to an array with element of unknown size because 'ATOM_PPLIB_STATE_V2' (aka 'struct _ATOM_PPLIB_STATE_V2') is a struct type with a flexible array member 442 | ATOM_PPLIB_STATE_V2 states[] __counted_by(ucNumEntries); | ^~~~ 1 error generated. ``` Based on our previous discussion (https://github.com/llvm/llvm-project/pull/90786#issuecomment-2108855275) I hope we can agree using the attribute this way is problematic. I don't see how `states` could be have its bounds correctly computed when the compiler doesn't know how big `struct _ATOM_PPLIB_STATE_V2` actually is. To unbreak this particular use quickly I'll downgrade the struct with FAM case to a warning and from there we'll need to decide how to proceed. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
rapidsna wrote: @bwendling @kees Likely, we should not put `__counted_by` in that case. Could we fix the source? https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
rapidsna wrote: @bwendling @kees Wait. `ATOM_PPLIB_STATE_V2` is also a struct with flexible array member? This is concerning because `ucNumEntries * sizeof(ATOM_PPLIB_STATE_V2)` is not the correct size anyway. Do you know the semantics of this structure? https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
rapidsna wrote: @bwendling Thanks for reporting. We will relax the restrictions for arrays to not break the existing users. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote: This seems to have broken the Linux build: https://github.com/llvm/llvm-project/commit/0ec3b972e58bcbcdc1bebe1696ea37f2931287c3 breaks the build for Linux, added by https://git.kernel.org/linus/781d41fed19caf900c8405064676813dc9921d32: https://paste.debian.net/plainh/b192bcd1 https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: Test fixed by `112eadd55f06bee15caadff688ea0b45acbfa804`. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: Looks like I broke the `clang/test/Misc/pragma-attribute-supported-attributes-list.test` test. I'll push a follow up fix to that test once I've confirmed I've fixed it. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher closed https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/90786
>From 80dbab4c4b43eb78f29b7b8fa577f04772a7f52c Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 1 May 2024 13:56:52 -0700
Subject: [PATCH] [BoundsSafety] Allow 'counted_by' attribute on pointers in
structs in C
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g.
for `_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
---
clang/docs/ReleaseNotes.rst | 21 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 17 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 10 +
clang/lib/Parse/ParseDecl.cpp | 104 ++-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 82 --
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117
.../Sema/attr-counted-by-late-parsed-off.c| 26 ++
.../attr-counted-by-late-parsed-struct-ptrs.c | 254 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 224 +++
.../Sema/attr-counted-by-vla-sizeless-types.c | 11 +
clang/test/Sema/attr-counted-by-vla.c | 193 +
clang/test/Sema/attr-counted-by.c | 112
21 files changed, 1117 insertions(+), 148 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-vla-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-vla.c
delete mode 100644 clang/test/Sema/attr-counted-by.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 7af5869d21768..2f83f5c6d54e9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -317,7 +317,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: @kees Thanks for approving. I'm going to resolve the merge conflict in `clang/docs/ReleaseNotes.rst` and then merge. I'll start looking at supporting `__counted_by()` on incomplete pointee types next. @hnrklssn is going to start working on upstreaming the `__sized_by` attribute soon. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/90786
>From e6fb7a3374ada3d02b4c89263ffd14a037a7e56a Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 1 May 2024 13:56:52 -0700
Subject: [PATCH] [BoundsSafety] Allow 'counted_by' attribute on pointers in
structs in C
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g.
for `_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
---
clang/docs/ReleaseNotes.rst | 22 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 17 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 10 +
clang/lib/Parse/ParseDecl.cpp | 104 ++-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 82 --
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117
.../Sema/attr-counted-by-late-parsed-off.c| 26 ++
.../attr-counted-by-late-parsed-struct-ptrs.c | 254 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 224 +++
.../Sema/attr-counted-by-vla-sizeless-types.c | 11 +
clang/test/Sema/attr-counted-by-vla.c | 193 +
clang/test/Sema/attr-counted-by.c | 112
21 files changed, 1118 insertions(+), 148 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-vla-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-vla.c
delete mode 100644 clang/test/Sema/attr-counted-by.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4c0fe5bcf6b12..09119394cfad6 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -258,7 +258,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/kees approved this pull request. Thanks for the updates! Let's get this in and continue with the rest of the support. :) https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote: @bwendling @kees Any further feedback? If not, can you approve? As @rapidsna said we'll follow up this PR with additional PRs to address the two major concerns you had. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -0,0 +1,187 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define __counted_by(f) __attribute__((counted_by(f)))
+
+struct bar;
+
+struct not_found {
+ int count;
+ struct bar *fam[] __counted_by(bork); // expected-error {{use of undeclared
identifier 'bork'}}
+};
+
+struct no_found_count_not_in_substruct {
+ unsigned long flags;
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct A {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_not_in_unnamed_substruct {
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_not_in_unnamed_substruct_2 {
+ struct {
+unsigned char count; // expected-note {{'count' declared here}}
+ };
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_in_other_unnamed_substruct {
+ struct {
+unsigned char count;
+ } a1;
+
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+ };
+};
+
+struct not_found_count_in_other_substruct {
+ struct _a1 {
+unsigned char count;
+ } a1;
+
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+ };
+};
+
+struct not_found_count_in_other_substruct_2 {
+ struct _a2 {
+unsigned char count;
+ } a2;
+
+ int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+};
+
+struct not_found_suggest {
+ int bork;
+ struct bar *fam[] __counted_by(blork); // expected-error {{use of undeclared
identifier 'blork'}}
+};
+
+int global; // expected-note {{'global' declared here}}
+
+struct found_outside_of_struct {
+ int bork;
+ struct bar *fam[] __counted_by(global); // expected-error {{field 'global'
in 'counted_by' not inside structure}}
+};
+
+struct self_referrential {
+ int bork;
+ struct bar *self[] __counted_by(self); // expected-error {{use of undeclared
identifier 'self'}}
+};
+
+struct non_int_count {
+ double dbl_count;
+ struct bar *fam[] __counted_by(dbl_count); // expected-error {{'counted_by'
requires a non-boolean integer type argument}}
+};
+
+struct array_of_ints_count {
+ int integers[2];
+ struct bar *fam[] __counted_by(integers); // expected-error {{'counted_by'
requires a non-boolean integer type argument}}
+};
+
+struct not_a_fam {
+ int count;
+ // expected-error@+1{{'counted_by' cannot be applied to a pointer with
pointee of unknown size because 'struct bar' is an incomplete type}}
+ struct bar *non_fam __counted_by(count);
+};
+
+struct not_a_c99_fam {
+ int count;
+ struct bar *non_c99_fam[0] __counted_by(count); // expected-error
{{'counted_by' on arrays only applies to C99 flexible array members}}
+};
+
+struct annotated_with_anon_struct {
+ unsigned long flags;
+ struct {
+unsigned char count;
+int array[] __counted_by(crount); // expected-error {{use of undeclared
identifier 'crount'}}
+ };
+};
+
+//==
+// __counted_by on a struct VLA with element type that has unknown size
+//==
+
+struct size_unknown; // expected-note 2{{forward declaration of 'struct
size_unknown'}}
+struct on_member_arr_incomplete_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'struct
size_unknown'}}
+ struct size_unknown buf[] __counted_by(count);
+};
+
+struct on_member_arr_incomplete_const_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'const struct
size_unknown'}}
+ const struct size_unknown buf[] __counted_by(count);
+};
+
+struct on_member_arr_void_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'void'}}
+ void buf[] __counted_by(count);
+};
+
+typedef void(fn_ty)(int);
+
+struct on_member_arr_fn_ty {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{'buf' declared as array of functions of type 'fn_ty'
(aka 'void (int)')}}
+ fn_ty buf[] __counted_by(count);
+};
delcypher wrote:
@kees Good catch. Fixed
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/90786
>From 1f4d924768409d6bc61d160c6161e6acebf62b60 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 1 May 2024 13:56:52 -0700
Subject: [PATCH 1/4] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g.
for `_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
---
clang/docs/ReleaseNotes.rst | 19 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 15 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 12 ++
clang/lib/Parse/ParseDecl.cpp | 104 +-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 71 +--
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117 +++
.../Sema/attr-counted-by-late-parsed-off.c| 26 +++
.../attr-counted-by-late-parsed-struct-ptrs.c | 192 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 170
clang/test/Sema/attr-counted-by.c | 5 +-
19 files changed, 787 insertions(+), 38 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4c0fe5bcf6b12..81d0a89b29487 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -258,7 +258,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental feature to
allow late parsing certain attributes in specific contexts where they would
- not normally be late parsed.
+ not normally be late parsed. Currently this allows late parsing the
+ `counted_by` attribute in C. See `Attribute Changes in Clang`_.
Deprecated Compiler Flags
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote:
> > It's not a lie, because the contents of a pointer don't contribute to the
> > size of the struct containing that pointer.
>
> Consider this example. It tries to illustrate why putting `__counted_by()` on
> a pointer to a structs containing flexible array members doesn't make sense.
>
> ```c
> struct HasFAM {
> int count;
> char buffer[] __counted_by(count); // This is OK
> };
>
> struct BufferOfFAMS {
> int count;
> struct HasFAM* fams __counted_by(count); // This is invalid
> };
>
> struct BufferOfFAMS* setup(void) {
> const int numFams = 2;
> const int famSizes[] = { 8, 16};
>
> struct BufferOfFAMS *f = (struct BufferOfFAMS *)malloc(
> sizeof(struct BufferOfFAMS) + (sizeof(struct HasFam *) * numFams));
>
> f->count = numFams;
>
> size_t famsBufferSize = 0;
> for (int i=0; i < numFams; ++i) {
> famsBufferSize += sizeof(struct HasFAM) + sizeof(char)* famSizes[i];
> }
> f->fams = (struct HasFAM*) malloc(famsBufferSize);
> memset(f->fams, 0, famsBufferSize);
>
> size_t byteOffset = 0;
> for (int i=0; i < numFams; ++i) {
> struct HasFAM* cur = (struct HasFAM*) (((char*)f->fams) + byteOffset);
> cur->count = famSizes[i];
> byteOffset = sizeof(struct HasFAM) + (sizeof(char) * famSizes[i]);
> }
> return f;
> }
>
> int main(void) {
> // How would we compute the bounds of f::fams here??
> // `sizeof(struct HasFAM) * f->count` is WRONG. It's too small but this
> // is what writing `__counted_by(count)` on `HasFAM::buffer` means.
> //
> // It's actually
> // `(sizeof(struct HasFAM) * f->count) + (sizeof(char) * (8 + 16))`
> //
> // This means we can't use the `__counted_by()` attribute on
> // `BufferOfFAMS::fams` to compute its bounds. But allowing the bounds
> // to be computed is the whole point of the attribute.
> struct BufferOfFAMS* f = setup();
>
> // Similary doing any kind of indexing on the pointer
> // is extremely problematic for exactly the same reason.
> //
> // The address is completely wrong because the offset is computed using
> // `sizeof(struct HasFAM)` which won't include the array at the end.
> //
> // So `bogus_ptr` points to the first byte of the first `HasFAM::buffer`,
> // instead of the second `struct HasFAM`.
> struct HasFAM* bogus_ptr = >fams[1];
> return 0;
> }
> ```
I agree that it doesn't make sense, but not every struct will have a flexible
array member, of course. :-)
I guess as long as there's a followup with the support @kees mentioned above
then I have no issues with this PR also.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
rapidsna wrote: > The main concern I have with delaying support for this is that header files > could find themselves in a state where they could not be refactored without > removing counted_by attributes that refer to now-incomplete structs. @kees Agreed. We will work on a follow up patch to support this, and a separate patch to support `sized_by` for `void *` and cases where it needs a byte size. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -0,0 +1,187 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define __counted_by(f) __attribute__((counted_by(f)))
+
+struct bar;
+
+struct not_found {
+ int count;
+ struct bar *fam[] __counted_by(bork); // expected-error {{use of undeclared
identifier 'bork'}}
+};
+
+struct no_found_count_not_in_substruct {
+ unsigned long flags;
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct A {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_not_in_unnamed_substruct {
+ unsigned char count; // expected-note {{'count' declared here}}
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_not_in_unnamed_substruct_2 {
+ struct {
+unsigned char count; // expected-note {{'count' declared here}}
+ };
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{'counted_by' field
'count' isn't within the same struct as the flexible array}}
+ } a;
+};
+
+struct not_found_count_in_other_unnamed_substruct {
+ struct {
+unsigned char count;
+ } a1;
+
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+ };
+};
+
+struct not_found_count_in_other_substruct {
+ struct _a1 {
+unsigned char count;
+ } a1;
+
+ struct {
+int dummy;
+int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+ };
+};
+
+struct not_found_count_in_other_substruct_2 {
+ struct _a2 {
+unsigned char count;
+ } a2;
+
+ int array[] __counted_by(count); // expected-error {{use of undeclared
identifier 'count'}}
+};
+
+struct not_found_suggest {
+ int bork;
+ struct bar *fam[] __counted_by(blork); // expected-error {{use of undeclared
identifier 'blork'}}
+};
+
+int global; // expected-note {{'global' declared here}}
+
+struct found_outside_of_struct {
+ int bork;
+ struct bar *fam[] __counted_by(global); // expected-error {{field 'global'
in 'counted_by' not inside structure}}
+};
+
+struct self_referrential {
+ int bork;
+ struct bar *self[] __counted_by(self); // expected-error {{use of undeclared
identifier 'self'}}
+};
+
+struct non_int_count {
+ double dbl_count;
+ struct bar *fam[] __counted_by(dbl_count); // expected-error {{'counted_by'
requires a non-boolean integer type argument}}
+};
+
+struct array_of_ints_count {
+ int integers[2];
+ struct bar *fam[] __counted_by(integers); // expected-error {{'counted_by'
requires a non-boolean integer type argument}}
+};
+
+struct not_a_fam {
+ int count;
+ // expected-error@+1{{'counted_by' cannot be applied to a pointer with
pointee of unknown size because 'struct bar' is an incomplete type}}
+ struct bar *non_fam __counted_by(count);
+};
+
+struct not_a_c99_fam {
+ int count;
+ struct bar *non_c99_fam[0] __counted_by(count); // expected-error
{{'counted_by' on arrays only applies to C99 flexible array members}}
+};
+
+struct annotated_with_anon_struct {
+ unsigned long flags;
+ struct {
+unsigned char count;
+int array[] __counted_by(crount); // expected-error {{use of undeclared
identifier 'crount'}}
+ };
+};
+
+//==
+// __counted_by on a struct VLA with element type that has unknown size
+//==
+
+struct size_unknown; // expected-note 2{{forward declaration of 'struct
size_unknown'}}
+struct on_member_arr_incomplete_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'struct
size_unknown'}}
+ struct size_unknown buf[] __counted_by(count);
+};
+
+struct on_member_arr_incomplete_const_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'const struct
size_unknown'}}
+ const struct size_unknown buf[] __counted_by(count);
+};
+
+struct on_member_arr_void_ty_ty_pos {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{array has incomplete element type 'void'}}
+ void buf[] __counted_by(count);
+};
+
+typedef void(fn_ty)(int);
+
+struct on_member_arr_fn_ty {
+ int count;
+ // expected-error@+2{{'counted_by' only applies to pointers or C99 flexible
array members}}
+ // expected-error@+1{{'buf' declared as array of functions of type 'fn_ty'
(aka 'void (int)')}}
+ fn_ty buf[] __counted_by(count);
+};
kees wrote:
Please add a positive test for `typedef
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
kees wrote:
> Consider this example. It tries to illustrate why putting `__counted_by()` on
> a pointer to a structs containing flexible array members doesn't make sense.
>
> ```c
> struct HasFAM {
> int count;
> char buffer[] __counted_by(count); // This is OK
> };
>
> struct BufferOfFAMS {
> int count;
> struct HasFAM* fams __counted_by(count); // This is invalid
> };
> ```
Agreed: structs with flexible array members must be considered to be
singletons. This property is actually important for being able to have
`__builtin_dynamic_object_size()` work on pointers to flexible array structs.
i.e.:
```
size_t func(struct foo *p)
{
return__builtin_dynamic_object_size(p, 0);
}
```
This must always return `SIZE_MAX` for fixed-size arrays since the pointer may
be in the middle of a larger array of `struct foo`s, but if it is a struct with
a flexible array marked with `counted_by`, then we know deterministically what
the size is, since it must be a single complete object.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote:
> It's not a lie, because the contents of a pointer don't contribute to the
> size of the struct containing that pointer.
Consider this example. It tries to illustrate why putting `__counted_by()` on a
pointer to a structs containing flexible array members doesn't make sense.
```
struct HasFAM {
int count;
char buffer[] __counted_by(count); // This is OK
};
struct BufferOfFAMS {
int count;
struct HasFAM* fams __counted_by(count); // This is invalid
};
struct BufferOfFAMS* setup(void) {
const int numFams = 2;
const int famSizes[] = { 8, 16};
struct BufferOfFAMS *f = (struct BufferOfFAMS *)malloc(
sizeof(struct BufferOfFAMS) + (sizeof(struct HasFam *) * numFams));
f->count = numFams;
size_t famsBufferSize = 0;
for (int i=0; i < numFams; ++i) {
famsBufferSize += sizeof(struct HasFAM) + sizeof(char)* famSizes[i];
}
f->fams = (struct HasFAM*) malloc(famsBufferSize);
memset(f->fams, 0, famsBufferSize);
size_t byteOffset = 0;
for (int i=0; i < numFams; ++i) {
struct HasFAM* cur = (struct HasFAM*) (((char*)f->fams) + byteOffset);
cur->count = famSizes[i];
byteOffset = sizeof(struct HasFAM) + (sizeof(char) * famSizes[i]);
}
return f;
}
int main(void) {
// How would we compute the bounds of f::fams here??
// `sizeof(struct HasFAM) * f->count` is WRONG. It's too small but this
// is what writing `__counted_by(count)` on `HasFAM::buffer` means.
//
// It's actually
// `(sizeof(struct HasFAM) * f->count) + (sizeof(char) * (8 + 16))`
//
// This means we can't use the `__counted_by()` attribute on
// `BufferOfFAMS::fams` to compute its bounds. That's the whole
// point of the attribute.
struct BufferOfFAMS* f = setup();
// Similary doing any kind of indexing on the pointer
// is extremely problematic for exactly the same reason.
//
// The address is completely wrong because the offset is computed using
// `sizeof(struct HasFAM)` which won't include the array at the end.
//
// So `bogus_ptr` points to the first byte of the first `HasFAM::buffer`,
// instead of the second `struct HasFAM`.
struct HasFAM* bogus_ptr = >fams[1];
return 0;
}
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
kees wrote: > As @apple-fcloutier @rapidsna noted this is how `-fbounds-safety` is > currently implemented (because its much simpler) but it is a restriction that > could be lifted in future by only requiring `struct bar` to be defined at the > point that `foo::bar` is used rather than when the `__counted_by` attribute > is applied. Given that this PR is allowing `__counted_by` in a new place > (pointers in structs) I think its reasonable for us to place restrictions on > that until we've proved its actually necessary to have the more complicated > implementation. The main concern I have with delaying support for this is that header files could find themselves in a state where they could not be refactored without removing counted_by attributes that refer to now-incomplete structs. For example, in Linux we've been separating structs from implementations, and that usually means using incomplete structs as much as possible to avoid lots of #includes. So, no objection on this PR, but I think the more complete behavior needs to follow soonish. :) https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote:
> @rapidsna @delcypher @apple-fcloutier @kees:
>
> Okay, I think I see what the complication is. Are you trying to prevent the
> use case of someone writing something like:
>
> ```c
> struct bar;
>
> struct foo {
> size_t count;
> struct bar *ptr __counted_by(count);
> };
> ```
>
> where `ptr` is a pointer to one large space that the user splits up into
> `count` number of `struct bar` objects?
>
> ```c
> struct foo *alloc_foo(size_t count) {
> struct foo *p = malloc(sizeof(struct foo));
>
> p->count;
> p->ptr = malloc(sizeof(struct bar) * count); // struct bar can't be
> incomplete here
> return p;
> }
> ```
With the PR in its current form we **are** preventing this code but **only**
because `struct bar` is an incomplete type at the point the attribute is
applied. If `struct bar` was defined at the point `struct foo` was defined then
this would be allowed. So this restriction has nothing to do with how the user
wants to allocate their memory. It's all about the pointee type having a
defined size when the `__counted_by` attribute is applied.
As @apple-fcloutier @rapidsna noted this is how `-fbounds-safety` is currently
implemented (because its much simpler) but it is a restriction that could be
lifted in future by only requiring `struct bar` to be defined at the point that
`foo::bar` is used rather than when the `__counted_by` attribute is applied.
Given that this PR is allowing `__counted_by` in a new place (pointers in
structs) I think its reasonable for us to place restrictions on that until
we've proved its actually necessary to have the more complicated implementation.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote:
@rapidsna @delcypher @apple-fcloutier @kees:
Okay, I think I see what the complication is. Are you trying to prevent the use
case of someone writing something like:
```c
struct bar;
struct foo {
size_t count;
struct bar *ptr __counted_by(count);
};
```
where `ptr` is a pointer to one large space where that the user splits up into
`count` number of `struct bar` objects?
```c
struct foo *alloc_foo(size_t count) {
struct foo *p = malloc(sizeof(struct foo));
p->count;
p->ptr = malloc(sizeof(struct bar) * count); // struct bar can't be
incomplete here
return p;
}
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote:
@apple-fcloutier:
> think that there's room to allow `__counted_by` on incomplete types so that a
> TU where it's complete could use it (and we have use cases where that would
> be handy), but our implementation doesn't support it at this time. This can
> be added without disruptions at a later time. FWIW, the point of
> bounds-checking an index is moot at least while the type is incomplete, since
> it's an error to dereference a pointer to an incomplete type. Aside from
> indexing, operations that would cast the pointer (such as `memset(x->foo, 0,
> some_value)`, assuming `memset(void *__sized_by(n), int, size_t n)`) would
> still have to be illegal when `struct foo` is incomplete because there's no
> way to know how many bytes we have at x->foo without `sizeof(struct foo)`.
What I'm thinking is something like:
```c
a.c:
struct foo;
struct bar {
size_t count;
struct foo *ptr __counted_by(count);
};
struct bar *alloc_bar(size_t num_elems) {
struct bar *p = malloc(sizeof(bar));
p->count = num_elems;
p->ptr = malloc(sizeof(struct foo *) * num_elems);
return p;
}
/* later on, or in another TU */
struct foo {
/* fields */
};
void bork(size_t num_elems) {
struct bar *p = alloc_bar(num_elems);
for (size_t i = 0; i < num_elems; ++i) {
p->ptr[i] = alloc_foo();
}
/* some use of p */
}
}
```
I was incorrect about `void *`. It's valid to cast any pointer to `void *` and
back to the original type.
> For structs with a flexible array member, the reason is that sizeof(struct
> bar) is a lie. How would we reconcile, for instance, a count of 1 (implying
> there's like 4 bytes at x->bar) with a x->bar->a of 10 (implying that
> x->bar->fam[8] is in bounds)? How do we feel that x->bar[0]->fam[0] aliases
> with x->bar[1]->a?
It's not a lie, because the contents of a pointer don't contribute to the size
of the struct containing that pointer.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
rapidsna wrote: > I've been thinking about this restriction. Why is this necessary? My > assumption was that applying counted_by to a pointer causes a bounds check on > an index into the pointer rather than its underlying type. @bwendling It's because these types are not indexable really. **void:** `void` doesn't have a size and C standard doesn't allow indexing into `void *`. I understand `void *` can be indexable under a GNU extension, but I don't see not supporting it is a problem because we can use `__sized_by` to annotate `void *` to clearly indicate the byte size. We will upstream `__sized_by` support soon so you can use it for `void *`. **function types** Although, again, the GNU extension allows it, we don't really want to index into function pointers. We can still use `__sized_by` if we really need to. **Incomplete structs** You can't really index into an incomplete struct. Though as @apple-fcloutier mentioned, by the point when the pointer is actually indexed, you should have the complete type definition. Otherwise, indexing will be an error anyway. So we have been considering relaxing this requirement, and move the error point to where the pointer is actually used in a way it requires the concrete element size (e.g, places you would insert `__dynamic_builtin_object_size`, you need the element size to calculate the byte size; indexing into a pointer to incomplete struct is already an error). https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
delcypher wrote:
@bwendling
> I've been thinking about this restriction. Why is this necessary? My
> assumption was that applying counted_by to a pointer causes a bounds check on
> an index into the pointer rather than its underlying type.
@rapidsna Please add additional points if I accidentally miss something.
I can try to explain the restrictions from the perspective of the
`-fbounds-safety` perspective. Essentially the reason these constructs are
illegal in `-fbounds-safety` is because at runtime we need to be able to
construct a wide pointer from `__counted_by()` pointers. In the cases I've
made illegal in this PR, this is either impossible to do correctly if we don't
know the size of the pointee.
What I mean specifically by wide pointer is `__bidi_indexable` which is
`-fbounds-safety`'s wide pointer that can be indexed positively and negatively.
In `-fbounds-safety`'s model all pointers on the stack are `__bidi_indexable`
by default and all `__counted_by()` pointers are implicitly converted to wide
pointers when they are used inside a function.
`-fbounds-safety`'s `__bidi_indexable` is **very roughly** equivalent to this
C++ code
```c++
template struct WidePtr {
T *ptr; // The address that is dereferenced
// The bounds of the memory that forms the range of valid bytes to access
// [lower_bound, upper_bound)
void *lower_bound;
void *upper_bound;
WidePtr(T * /*__counted_by(count)*/ p, size_t count) {
ptr = p;
lower_bound = (void *)p;
// NEED sizeof(T) here!
upper_bound = ((char *)p) + sizeof(T) * count;
}
T& operator*() {
check_bounds();
return *ptr;
}
void check_bounds() {
if (ptr < lower_bound)
__builtin_trap();
uintptr_t LastByteAccessedPlusOne;
// NEED sizeof(T) here!
if (__builtin_uadd_overflow(ptr, sizeof(T), ))
__builtin_trap();
if (LastByteAccessedPlusOne > upper_bound)
__builtin_trap();
}
// Lots of other operators are overloaded (e.g. +, -[], ++, -- and ->).
};
```
We need to know what `sizeof(T)` is in two places:
1. Everywhere you want to do a bounds check. E.g. when dereferencing the
pointer.
2. When converting a `T* __counted_by()` pointer to a `WidePointer` (or `T*
__bidi_indexable` in `-fbounds-safety` parlance). If you look at the
constructor **we need to know** `sizeof(T)` when computing the upper bound of
the wide pointer. Given that this conversion happens whenever a pointer of the
type `T* __counted_by()` is used inside a function in the `-fbounds-safety`
model, the model requires that `T` have a known size. Technically this PR is
being even stricter. It's forbidding declaring a `T* __counted_by()` even if it
isn't used. However, this is a restriction we can lift later once more of the
implementation is in place.
I've tried to annotate your code below with the specific reasons they are
forbidden.
```c
// Both `foo` and `bar` are legal
struct foo;
struct bar {
int a;
int fam[] __counted_by(a);
};
struct x {
// Illegal in `-fbounds-safety`. We can't compute the upper bound of this
pointer
// because `sizeof(void)*count` isn't valid. In `-fbounds-safety` the
attribute you
// would use here is `__sized_by()` which is a byte count rather than an
element count.
void *p __counted_by(count);
// Illegal in `-fbounds-safety`. We can't compute the upper bound of this
pointer because
// `sizeof(struct foo)*count` isn't valid. In particular `sizeof(struct
foo)` is invalid because
// `struct foo` is an incomplete type.
struct foo *f __counted_by(count);
// Illegal in `-fbounds-safety`. While we can technically compute
`sizeof(struct bar)*count`
// that computation is completely wrong unless the `bar::a` is `0`. To
actually get the correct
// count we would need to walk every `struct bar` object in the buffer
pointed to by `b` to
// read `b::a`. This could be expensive (and prone to race conditions) so
we simply
// don't support this right now.
struct bar *b __counted_by(count);
int count;
};
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
apple-fcloutier wrote: I think that there's room to allow `__counted_by` on incomplete types so that a TU where it's complete could use it (and we have use cases where that would be handy), but our implementation doesn't support it at this time. This can be added without disruptions at a later time. FWIW, the point of bounds-checking an index is moot at least while the type is incomplete, since it's an error to dereference a pointer to an incomplete type. Aside from indexing, operations that would cast the pointer (such as `memset(x->foo, 0, some_value)`, assuming `memset(void *__sized_by(n), int, size_t n)`) would still have to be illegal when `struct foo` is incomplete because there's no way to know how many bytes we have at x->foo without `sizeof(struct foo)`. `void *__counted_by` could be made legal because clang defines `sizeof(void) == 1` in C mode, as an extension to the C standard. I think it's still best avoided: "how many voids do I have at p?" is not a great question to ask, whereas the same with `__sized_by` just makes sense. It's also a little bit easier to filter out `void` with the rest of the incomplete types. For structs with a flexible array member, the reason is that `sizeof(struct bar)` is a lie. How would we reconcile, for instance, a count of 1 (implying there's like 4 bytes at `x->bar`) with a `x->bar->a` of 10 (implying that `x->bar->fam[8]`Â is in bounds)? How do we feel that `x->bar[0]->fam[0]` aliases with `x->bar[1]->a`? https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
bwendling wrote:
> Note the attribute is prevented on pointee types where the size isn't known
> at compile time. In particular pointee types that are:
>
> * Incomplete (e.g. `void`) and sizeless types
> * Function types (e.g. the pointee of a function pointer)
> * Struct types with a flexible array member
I've been thinking about this restriction. Why is this necessary? My assumption
was that applying `counted_by` to a pointer causes a bounds check on an index
into the __pointer__ rather than its underlying type. So something like:
```c
struct foo;
struct bar {
int a;
int fam[] __counted_by(a);
};
struct x {
void *p __counted_by(count); // void * is treated like char *, I
think.
struct foo *f __counted_by(count); // sizeof(f) is the size of a general
pointer.
struct bar *b __counted_by(count); // a list of pointers to 'struct bar's
should be okay.
int count;
};
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -335,6 +336,22 @@ Attribute Changes in Clang - Clang now warns that the ``exclude_from_explicit_instantiation`` attribute is ignored when applied to a local class or a member thereof. +- The ``counted_by`` attribute can now be late parsed in C when bwendling wrote: I don't have a strong opinion either. :-) It's fine to keep the blurb in. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -335,6 +336,22 @@ Attribute Changes in Clang - Clang now warns that the ``exclude_from_explicit_instantiation`` attribute is ignored when applied to a local class or a member thereof. +- The ``counted_by`` attribute can now be late parsed in C when delcypher wrote: So to add some context here. The original reason there was a release note is because in #88596 (which this patch builds upon) @AaronBallman asked for there to be a release note about the new `-fexperimental-late-parse-attributes` flag. This patch means `-fexperimental-late-parse-attributes` actually does something now (before it did nothing) so I updated the release notes on that flag and it "felt" like it made sense to explain in the release notes the change on the attributes in relation to the flag. I don't have strong opinions here. I'm ok to drop the changes to the release notes or keep them or something in between. @AaronBallman any opinions here? https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -6534,6 +6536,15 @@ def err_counted_by_attr_refer_to_union : Error<
"'counted_by' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
+def err_counted_by_attr_pointee_unknown_size : Error<
+ "'counted_by' cannot be applied a pointer with pointee with unknown size "
+ "because %0 is %select{"
+"an incomplete type|" // CountedByInvalidPointeeTypeKind::INCOMPLETE
+"a sizeless type|" // CountedByInvalidPointeeTypeKind::SIZELESS
+"a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
bwendling wrote:
I think that as long as it's a pointer we're okay, at least w.r.t. the
`counted_by` attribute. We know the size of pointers at least. :-)
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -631,6 +631,18 @@ bool Type::isStructureType() const {
return false;
}
+bool Type::isStructureTypeWithFlexibleArrayMember() const {
+ const auto *RT = getAs();
+ if (!RT)
+return false;
+ const auto *Decl = RT->getDecl();
+ if (!Decl->isStruct())
+return false;
+ if (!Decl->hasFlexibleArrayMember())
+return false;
+ return true;
bwendling wrote:
Maybe? The `isFlexibleArrayMemberLike` methods are more in depth analysis of
the struct and checks against the `-fstrict-flex-arrays=` flag, so it's a more
expensive call. But if the `hasFlexibleArrayMember` is always set "correctly,"
then it's probably sufficient.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -6534,6 +6536,15 @@ def err_counted_by_attr_refer_to_union : Error<
"'counted_by' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
+def err_counted_by_attr_pointee_unknown_size : Error<
+ "'counted_by' cannot be applied a pointer with pointee with unknown size "
+ "because %0 is %select{"
+"an incomplete type|" // CountedByInvalidPointeeTypeKind::INCOMPLETE
+"a sizeless type|" // CountedByInvalidPointeeTypeKind::SIZELESS
+"a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
delcypher wrote:
Side note: Given the above I expected pointer arithmetic on a function pointer
to be forbidden but it's not...
```c
typedef void(fn_ty)(int);
typedef void(*fn_ptr_ty)(int);
void test(fn_ty* fn) {
fn(5); // ok
// WHAT!?: How can pointer arithmetic be possible when the size of
// the function isn't a defined thing...
//
// Turns out clang codegens this as if `sizeof(*fn) == 1`
_Static_assert(sizeof(*fn) == 1, "oh boy...we're in for a wild ride");
++fn;
// Let's jump somewhere into the function that's 1 byte along from the
start.
// what could possibly go wrong!?
fn(5); // CRASH
return;
}
```
It's fine to do pointer arithmetic on an array of function pointers though
```c
typedef void(fn_ty)(int);
typedef void(*fn_ptr_ty)(int);
void do_something(int i) { printf("hello %d\n", i);}
void do_something2(int i) { printf("hello2 %d\n", i);}
fn_ptr_ty Funcs[] = { do_something, do_something2};
void test2(fn_ty** fn) {
// fn == do_something
(*fn)(5);
++fn;
// Now fn == do_something2
(*fn)(5);
return;
}
int main(void) {
test2(Funcs);
return 0;
}
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -6534,6 +6536,15 @@ def err_counted_by_attr_refer_to_union : Error<
"'counted_by' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
+def err_counted_by_attr_pointee_unknown_size : Error<
+ "'counted_by' cannot be applied a pointer with pointee with unknown size "
+ "because %0 is %select{"
+"an incomplete type|" // CountedByInvalidPointeeTypeKind::INCOMPLETE
+"a sizeless type|" // CountedByInvalidPointeeTypeKind::SIZELESS
+"a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
bwendling wrote:
> A "function type" and "function pointer type" are different.
Ah! my mistake. :-)
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -335,6 +336,22 @@ Attribute Changes in Clang - Clang now warns that the ``exclude_from_explicit_instantiation`` attribute is ignored when applied to a local class or a member thereof. +- The ``counted_by`` attribute can now be late parsed in C when bwendling wrote: If it doesn't have a use yet, maybe you don't need to add it in the release notes? https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/90786
>From 1f4d924768409d6bc61d160c6161e6acebf62b60 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 1 May 2024 13:56:52 -0700
Subject: [PATCH 1/3] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g.
for `_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
---
clang/docs/ReleaseNotes.rst | 19 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 15 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 12 ++
clang/lib/Parse/ParseDecl.cpp | 104 +-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 71 +--
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117 +++
.../Sema/attr-counted-by-late-parsed-off.c| 26 +++
.../attr-counted-by-late-parsed-struct-ptrs.c | 192 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 170
clang/test/Sema/attr-counted-by.c | 5 +-
19 files changed, 787 insertions(+), 38 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4c0fe5bcf6b122..81d0a89b294878 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -258,7 +258,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental feature to
allow late parsing certain attributes in specific contexts where they would
- not normally be late parsed.
+ not normally be late parsed. Currently this allows late parsing the
+ `counted_by` attribute in C. See `Attribute Changes in Clang`_.
Deprecated Compiler Flags
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -631,6 +631,18 @@ bool Type::isStructureType() const {
return false;
}
+bool Type::isStructureTypeWithFlexibleArrayMember() const {
+ const auto *RT = getAs();
+ if (!RT)
+return false;
+ const auto *Decl = RT->getDecl();
+ if (!Decl->isStruct())
+return false;
+ if (!Decl->hasFlexibleArrayMember())
+return false;
+ return true;
delcypher wrote:
@bwendling I just noticed there's this `Decl::isFlexibleArrayMemberLike()`
should we be using that instead? It looks like its semantics are a little
different.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -6534,6 +6536,15 @@ def err_counted_by_attr_refer_to_union : Error<
"'counted_by' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
+def err_counted_by_attr_pointee_unknown_size : Error<
+ "'counted_by' cannot be applied a pointer with pointee with unknown size "
+ "because %0 is %select{"
+"an incomplete type|" // CountedByInvalidPointeeTypeKind::INCOMPLETE
+"a sizeless type|" // CountedByInvalidPointeeTypeKind::SIZELESS
+"a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
delcypher wrote:
@bwendling I was also confused by this initially. A "function type" and
"function pointer type" are **different**. A function type doesn't have a size
so we have to forbid it, but a function pointer does have a size (it's the size
of a pointer).
Hopefully these examples illustrate the problem:
```c
#define __counted_by(f) __attribute__((counted_by(f)))
typedef void(fn_ty)(int);
typedef void(*fn_ptr_ty)(int);
struct a {
int count;
// Buffer of functions is invalid. A "function" has no size
fn_ty* buffer __counted_by(count);
fn_ptr_ty buffer2 __counted_by(count);
};
struct b {
int count;
// Valid: A buffer of function pointers is allowed
fn_ty** buffer __counted_by(count);
fn_ptr_ty* buffer2 __counted_by(count);
};
```
A similar thing exists for arrays. If you write this
```c
struct c {
fn_ty Arr[];
};
```
this produces the following error:
```
error: 'Arr' declared as array of functions of type 'fn_ty' (aka 'void (int)')
22 | fn_ty Arr[];
| ^
```
However these two struct definitions are allowed by clang:
```c
struct d {
fn_ty* Arr[];
};
struct e {
fn_ptr_ty Arr2[];
};
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -335,6 +336,22 @@ Attribute Changes in Clang - Clang now warns that the ``exclude_from_explicit_instantiation`` attribute is ignored when applied to a local class or a member thereof. +- The ``counted_by`` attribute can now be late parsed in C when delcypher wrote: I can mention it but I'm wondering if it should be in the release notes at all because this patch only makes parsing work. I've not done anything on the codegen side to make use of the attribute on pointers. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher updated
https://github.com/llvm/llvm-project/pull/90786
>From 1f4d924768409d6bc61d160c6161e6acebf62b60 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 1 May 2024 13:56:52 -0700
Subject: [PATCH 1/2] [BoundsSafety] Allow 'counted_by' attribute on pointers
in structs in C
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g. parameters, returns types)
when `-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g.
for `_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
---
clang/docs/ReleaseNotes.rst | 19 +-
clang/include/clang/AST/Type.h| 1 +
clang/include/clang/Basic/Attr.td | 3 +-
.../clang/Basic/DiagnosticSemaKinds.td| 15 +-
clang/include/clang/Parse/Parser.h| 7 +-
clang/include/clang/Sema/Sema.h | 3 +-
clang/lib/AST/Type.cpp| 12 ++
clang/lib/Parse/ParseDecl.cpp | 104 +-
clang/lib/Parse/ParseObjc.cpp | 10 +-
clang/lib/Sema/SemaDeclAttr.cpp | 71 +--
clang/lib/Sema/SemaType.cpp | 6 +-
clang/lib/Sema/TreeTransform.h| 2 +-
.../attr-counted-by-late-parsed-struct-ptrs.c | 45
clang/test/AST/attr-counted-by-struct-ptrs.c | 117 +++
.../Sema/attr-counted-by-late-parsed-off.c| 26 +++
.../attr-counted-by-late-parsed-struct-ptrs.c | 192 ++
...tr-counted-by-struct-ptrs-sizeless-types.c | 17 ++
clang/test/Sema/attr-counted-by-struct-ptrs.c | 170
clang/test/Sema/attr-counted-by.c | 5 +-
19 files changed, 787 insertions(+), 38 deletions(-)
create mode 100644 clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/AST/attr-counted-by-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-off.c
create mode 100644 clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c
create mode 100644 clang/test/Sema/attr-counted-by-struct-ptrs.c
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4c0fe5bcf6b122..81d0a89b294878 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -258,7 +258,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental feature to
allow late parsing certain attributes in specific contexts where they would
- not normally be late parsed.
+ not normally be late parsed. Currently this allows late parsing the
+ `counted_by` attribute in C. See `Attribute Changes in Clang`_.
Deprecated Compiler Flags
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/bwendling edited https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -3282,6 +3282,19 @@ void Parser::ParseAlignmentSpecifier(ParsedAttributes
,
}
}
+void Parser::DistributeCLateParsedAttrs(Decl *Dcl,
+LateParsedAttrList *LateAttrs) {
+ assert(Dcl);
bwendling wrote:
Could you add a string to the assert?
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -8588,31 +8588,71 @@ static const RecordDecl
*GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
return RD;
}
-static bool
-CheckCountExpr(Sema , FieldDecl *FD, Expr *E,
- llvm::SmallVectorImpl ) {
+enum class CountedByInvalidPointeeTypeKind {
+ INCOMPLETE,
+ SIZELESS,
+ FUNCTION,
+ FLEXIBLE_ARRAY_MEMBER,
+ VALID,
+};
+
+static bool CheckCountedByAttrOnField(
+Sema , FieldDecl *FD, Expr *E,
+llvm::SmallVectorImpl ) {
+ // Check the context the attribute is used in
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
<< FD->getSourceRange();
return true;
}
- if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
-S.Diag(E->getBeginLoc(), diag::err_counted_by_attr_argument_not_integer)
-<< E->getSourceRange();
+ const auto FieldTy = FD->getType();
+ if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
+<< FD->getLocation();
return true;
}
LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
LangOptions::StrictFlexArraysLevelKind::IncompleteOnly;
-
- if (!Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FD->getType(),
+ if (FieldTy->isArrayType() &&
+ !Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FieldTy,
StrictFlexArraysLevel, true)) {
-// The "counted_by" attribute must be on a flexible array member.
-SourceRange SR = FD->getLocation();
-S.Diag(SR.getBegin(),
- diag::err_counted_by_attr_not_on_flexible_array_member)
-<< SR;
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_on_array_not_flexible_array_member)
+<< FD->getLocation();
+return true;
+ }
+
+ if (FieldTy->isPointerType()) {
bwendling wrote:
> @isanbard Is the above code pattern something you actually want to support?
> In -fbounds-safety we disallow it because it means computing the bounds of
> Arr is not constant time because the size of Has_VLA is not a compile time
> constant. To compute the bounds of Arr at runtime it would require walking
> every Has_VLA object in Arr to get its count field. That seems expensive and
> is also at risk of running into a race condition (i.e. the size of a VLA
> changes while the bounds are being computed).
Is it even valid C? I *suppose* it could be valid if `struct Has_VLA` wasn't
allocated with extra bytes, i.e. `count` is 0 and `buffer` has no size.
But that's a horrible use case and I think it's fine to flag it as an error.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -631,6 +631,18 @@ bool Type::isStructureType() const {
return false;
}
+bool Type::isStructureTypeWithFlexibleArrayMember() const {
+ const auto *RT = getAs();
+ if (!RT)
+return false;
+ const auto *Decl = RT->getDecl();
+ if (!Decl->isStruct())
+return false;
+ if (!Decl->hasFlexibleArrayMember())
+return false;
+ return true;
bwendling wrote:
The last three lines could be simplified as:
```c
return Decl->hasFlexibleArrayMember();
```
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -6534,6 +6536,15 @@ def err_counted_by_attr_refer_to_union : Error<
"'counted_by' argument cannot refer to a union member">;
def note_flexible_array_counted_by_attr_field : Note<
"field %0 declared here">;
+def err_counted_by_attr_pointee_unknown_size : Error<
+ "'counted_by' cannot be applied a pointer with pointee with unknown size "
+ "because %0 is %select{"
+"an incomplete type|" // CountedByInvalidPointeeTypeKind::INCOMPLETE
+"a sizeless type|" // CountedByInvalidPointeeTypeKind::SIZELESS
+"a function type|" // CountedByInvalidPointeeTypeKind::FUNCTION
bwendling wrote:
I'm a bit confused why a function type is excluded. Isn't it just a pointer?
@kees can correct me, but I think the point of `counted_by` on a pointer is
that it could be a list of pointers, and we don't want to allow someone to
access beyond that list:
```c
struct s {
int count;
int *ptr __counted_by(count);
};
struct s *alloc(size_t num_elems) {
struct s *p = malloc(sizeof(struct s));
p->count = num_elems;
p->ptr = calloc(sizeof(int), num_elems);
return p;
}
```
If that's the case, then any pointer should be okay.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -335,6 +336,22 @@ Attribute Changes in Clang - Clang now warns that the ``exclude_from_explicit_instantiation`` attribute is ignored when applied to a local class or a member thereof. +- The ``counted_by`` attribute can now be late parsed in C when bwendling wrote: You should mention that `counted_by` was extended to work with pointers in structs first. Otherwise, "last parsing" of the attribute doesn't make a lot of sense since a flexible array member is at the end of a struct. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -8588,31 +8588,71 @@ static const RecordDecl
*GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
return RD;
}
-static bool
-CheckCountExpr(Sema , FieldDecl *FD, Expr *E,
- llvm::SmallVectorImpl ) {
+enum class CountedByInvalidPointeeTypeKind {
+ INCOMPLETE,
+ SIZELESS,
+ FUNCTION,
+ FLEXIBLE_ARRAY_MEMBER,
+ VALID,
+};
+
+static bool CheckCountedByAttrOnField(
+Sema , FieldDecl *FD, Expr *E,
+llvm::SmallVectorImpl ) {
+ // Check the context the attribute is used in
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
<< FD->getSourceRange();
return true;
}
- if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
-S.Diag(E->getBeginLoc(), diag::err_counted_by_attr_argument_not_integer)
-<< E->getSourceRange();
+ const auto FieldTy = FD->getType();
+ if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
+<< FD->getLocation();
return true;
}
LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
LangOptions::StrictFlexArraysLevelKind::IncompleteOnly;
-
- if (!Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FD->getType(),
+ if (FieldTy->isArrayType() &&
+ !Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FieldTy,
StrictFlexArraysLevel, true)) {
-// The "counted_by" attribute must be on a flexible array member.
-SourceRange SR = FD->getLocation();
-S.Diag(SR.getBegin(),
- diag::err_counted_by_attr_not_on_flexible_array_member)
-<< SR;
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_on_array_not_flexible_array_member)
+<< FD->getLocation();
+return true;
+ }
+
+ if (FieldTy->isPointerType()) {
delcypher wrote:
Good catch!
## Flexible array member
```
struct Has_VLA {
int count;
char buffer[] __counted_by(count);
};
struct Test {
int count;
struct Has_VLA Arr[] __counted_by(count);
};
```
There's no diagnostic here currently. I think the code above is something we
should disallow (at least for `-fbounds-safety`). I'll adjust the patch to
disallow this.
@isanbard Is the above code pattern something you actually want to support? In
`-fbounds-safety` we disallow it because it means computing the bounds of `Arr`
is not constant time because the size of `Has_VLA` is not a compile time
constant. To compute the bounds of `Arr` at runtime it would require walking
every `Has_VLA` object in `Arr` to get its `count` field. That seems expensive
and is also at risk of running into a race condition (i.e. the size of a VLA
changes while the bounds are being computed).
## Incomplete type
```
struct Test {
int count;
void Arr[] __counted_by(count);
};
```
We already have these diagnostics:
```
tmp/arr_sizeless_ty.c:5:13: error: array has incomplete element type 'void'
5 | void Arr[] __counted_by(count);
| ^
tmp/arr_sizeless_ty.c:5:5: error: 'counted_by' only applies to pointers or C99
flexible array members
5 | void Arr[] __counted_by(count);
| ^~~~
2 errors generated.
```
I don't think we need to add another.
## Sizeless types
```
struct Test {
int count;
__SVInt8_t Arr[] __counted_by(count);
};
```
We already have these diagnostics:
```
tmp/arr_sizeless_ty.c:5:19: error: array has sizeless element type '__SVInt8_t'
5 | __SVInt8_t Arr[] __counted_by(count);
| ^
tmp/arr_sizeless_ty.c:5:5: error: 'counted_by' only applies to pointers or C99
flexible array members
5 | __SVInt8_t Arr[] __counted_by(count);
| ^ ~~~
2 errors generated.
```
I don't think we need to add another.
## Function Type
```
typedef void(foo_fn)(int);
struct Test {
int count;
foo_fn Arr[] __counted_by(count);
};
```
We already have these diagnostics:
```
tmp/arr_sizeless_ty.c:7:15: error: 'Arr' declared as array of functions of type
'foo_fn' (aka 'void (int)')
7 | foo_fn Arr[] __counted_by(count);
| ^
tmp/arr_sizeless_ty.c:7:5: error: 'counted_by' only applies to pointers or C99
flexible array members
7 | foo_fn Arr[] __counted_by(count);
| ^ ~~~
2 errors generated.
```
I don't think we need to add another.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
@@ -8588,31 +8588,71 @@ static const RecordDecl
*GetEnclosingNamedOrTopAnonRecord(const FieldDecl *FD) {
return RD;
}
-static bool
-CheckCountExpr(Sema , FieldDecl *FD, Expr *E,
- llvm::SmallVectorImpl ) {
+enum class CountedByInvalidPointeeTypeKind {
+ INCOMPLETE,
+ SIZELESS,
+ FUNCTION,
+ FLEXIBLE_ARRAY_MEMBER,
+ VALID,
+};
+
+static bool CheckCountedByAttrOnField(
+Sema , FieldDecl *FD, Expr *E,
+llvm::SmallVectorImpl ) {
+ // Check the context the attribute is used in
+
if (FD->getParent()->isUnion()) {
S.Diag(FD->getBeginLoc(), diag::err_counted_by_attr_in_union)
<< FD->getSourceRange();
return true;
}
- if (!E->getType()->isIntegerType() || E->getType()->isBooleanType()) {
-S.Diag(E->getBeginLoc(), diag::err_counted_by_attr_argument_not_integer)
-<< E->getSourceRange();
+ const auto FieldTy = FD->getType();
+ if (!FieldTy->isArrayType() && !FieldTy->isPointerType()) {
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_not_on_ptr_or_flexible_array_member)
+<< FD->getLocation();
return true;
}
LangOptions::StrictFlexArraysLevelKind StrictFlexArraysLevel =
LangOptions::StrictFlexArraysLevelKind::IncompleteOnly;
-
- if (!Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FD->getType(),
+ if (FieldTy->isArrayType() &&
+ !Decl::isFlexibleArrayMemberLike(S.getASTContext(), FD, FieldTy,
StrictFlexArraysLevel, true)) {
-// The "counted_by" attribute must be on a flexible array member.
-SourceRange SR = FD->getLocation();
-S.Diag(SR.getBegin(),
- diag::err_counted_by_attr_not_on_flexible_array_member)
-<< SR;
+S.Diag(FD->getBeginLoc(),
+ diag::err_counted_by_attr_on_array_not_flexible_array_member)
+<< FD->getLocation();
+return true;
+ }
+
+ if (FieldTy->isPointerType()) {
hnrklssn wrote:
Does this need to be limited to pointers? Seems to me that it could apply to
arrays also.
https://github.com/llvm/llvm-project/pull/90786
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/Endilll commented: `Sema.h` changes look good to me. https://github.com/llvm/llvm-project/pull/90786 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: Dan Liew (delcypher)
Changes
Previously the attribute was only allowed on flexible array members. This patch
patch changes this to also allow the attribute on pointer fields in structs and
also allows late parsing of the attribute in some contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't known at
compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in the
declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late parsing
here is to avoid breaking the data layout of structs in existing code that want
to use the `counted_by` attribute. This patch is the first use of
`LateAttrParseExperimentalExt` in `Attr.td` that was introduced in a previous
patch.
Note by allowing the attribute on struct member pointers this now allows the
possiblity of writing the attribute in the type attribute position. For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed immediately
rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not been
done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be addressed
in future patches:
* Make late parsing working with anonymous structs (see `on_pointer_anon_buf`
in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types) when
`-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
---
Patch is 38.84 KiB, truncated to 20.00 KiB below, full version:
https://github.com/llvm/llvm-project/pull/90786.diff
18 Files Affected:
- (modified) clang/docs/ReleaseNotes.rst (+18-1)
- (modified) clang/include/clang/AST/Type.h (+1)
- (modified) clang/include/clang/Basic/Attr.td (+2-1)
- (modified) clang/include/clang/Basic/DiagnosticSemaKinds.td (+13-2)
- (modified) clang/include/clang/Parse/Parser.h (+6-1)
- (modified) clang/include/clang/Sema/Sema.h (+2-1)
- (modified) clang/lib/AST/Type.cpp (+12)
- (modified) clang/lib/Parse/ParseDecl.cpp (+97-7)
- (modified) clang/lib/Parse/ParseObjc.cpp (+6-4)
- (modified) clang/lib/Sema/SemaDeclAttr.cpp (+56-15)
- (modified) clang/lib/Sema/SemaType.cpp (+3-3)
- (modified) clang/lib/Sema/TreeTransform.h (+1-1)
- (added) clang/test/AST/attr-counted-by-late-parsed-struct-ptrs.c (+45)
- (added) clang/test/Sema/attr-counted-by-late-parsed-off.c (+26)
- (added) clang/test/Sema/attr-counted-by-late-parsed-struct-ptrs.c (+185)
- (added) clang/test/Sema/attr-counted-by-struct-ptrs-sizeless-types.c (+17)
- (added) clang/test/Sema/attr-counted-by-struct-ptrs.c (+163)
- (modified) clang/test/Sema/attr-counted-by.c (+3-2)
``diff
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index 4c0fe5bcf6b122..81d0a89b294878 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -258,7 +258,8 @@ New Compiler Flags
- ``-fexperimental-late-parse-attributes`` enables an experimental feature to
allow late parsing certain attributes in specific contexts where they would
- not normally be late parsed.
+ not normally be late parsed. Currently this allows late parsing the
+ `counted_by` attribute in C. See `Attribute Changes in Clang`_.
Deprecated Compiler Flags
-
@@ -335,6 +336,22 @@ Attribute Changes in Clang
- Clang now warns that the ``exclude_from_explicit_instantiation`` attribute
is ignored when applied to a local class or a member thereof.
+- The ``counted_by`` attribute can now be late parsed in C when
+ ``-fexperimental-late-parse-attributes`` is passed but only when attribute is
+ used in the declaration attribute position. This allows using the
+ attribute on existing code where it previously impossible to do so without
+ re-ordering struct field declarations would break ABI as shown below.
+
+ .. code-block:: c
+
+ struct
[clang] [BoundsSafety] Allow 'counted_by' attribute on pointers in structs in C (PR #90786)
https://github.com/delcypher created
https://github.com/llvm/llvm-project/pull/90786
Previously the attribute was only allowed on flexible array members. This patch
patch changes this to also allow the attribute on pointer fields in structs and
also allows late parsing of the attribute in some contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't known at
compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used in the
declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late parsing
here is to avoid breaking the data layout of structs in existing code that want
to use the `counted_by` attribute. This patch is the first use of
`LateAttrParseExperimentalExt` in `Attr.td` that was introduced in a previous
patch.
Note by allowing the attribute on struct member pointers this now allows the
possiblity of writing the attribute in the type attribute position. For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed immediately
rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not been
done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be addressed
in future patches:
* Make late parsing working with anonymous structs (see `on_pointer_anon_buf`
in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjects (e.g. parameters, returns types) when
`-fbounds-safety` is enabled.
* Make use of the attribute on pointer types in code gen (e.g. for
`_builtin_dynamic_object_size` and UBSan's array-bounds checks).
This work is heavily based on a patch originally written by Yeoul Na.
rdar://125400257
>From a89cca7fecae5ef5130cd9c8da834a186e99dbb5 Mon Sep 17 00:00:00 2001
From: Dan Liew
Date: Wed, 1 May 2024 13:56:52 -0700
Subject: [PATCH] [BoundsSafety] Allow 'counted_by' attribute on pointers in
structs in C
Previously the attribute was only allowed on flexible array members.
This patch patch changes this to also allow the attribute on pointer
fields in structs and also allows late parsing of the attribute in some
contexts.
For example this previously wasn't allowed:
```
struct BufferTypeDeclAttributePosition {
size_t count;
char* buffer __counted_by(count); // Now allowed
}
```
Note the attribute is prevented on pointee types where the size isn't
known at compile time. In particular pointee types that are:
* Incomplete (e.g. `void`) and sizeless types
* Function types (e.g. the pointee of a function pointer)
* Struct types with a flexible array member
This patch also introduces late parsing of the attribute when used
in the declaration attribute position. For example
```
struct BufferTypeDeclAttributePosition {
char* buffer __counted_by(count); // Now allowed
size_t count;
}
```
is now allowed but **only** when passing
`-fexperimental-late-parse-attributes`. The motivation for using late
parsing here is to avoid breaking the data layout of structs in existing
code that want to use the `counted_by` attribute. This patch is the
first use of `LateAttrParseExperimentalExt` in `Attr.td` that was
introduced in a previous patch.
Note by allowing the attribute on struct member pointers this now allows
the possiblity of writing the attribute in the type attribute position.
For example:
```
struct BufferTypeAttributePosition {
size_t count;
char *__counted_by(count) buffer; // Now allowed
}
```
However, the attribute in this position is still currently parsed
immediately rather than late parsed. So this will not parse currently:
```
struct BufferTypeAttributePosition {
char *__counted_by(count) buffer; // Fails to parse
size_t count;
}
```
The intention is to lift this restriction in future patches. It has not
been done in this patch to keep this size of this commit small.
There are also several other follow up changes that will need to be
addressed in future patches:
* Make late parsing working with anonymous structs (see
`on_pointer_anon_buf` in `attr-counted-by-late-parsed-struct-ptrs.c`).
* Allow `counted_by` on more subjedts (e.g.
