Skip to content

Conversation

@thejh
Copy link
Contributor

@thejh thejh commented Apr 8, 2025

Attr.td names the first alloc_size argument "ElemSizeParam" and the second optional argument "NumElemsParam"; but the semantics of how the two-argument version is used in practice is the opposite of that.

glibc declares calloc() like this, so the second alloc_size argument is the element size:

extern void *calloc (size_t __nmemb, size_t __size)
__THROW __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __wur;

The Linux kernel declares array allocation functions like kmalloc_array_noprof() the same way.

Add a comment explaining that the names used inside clang are misleading.

Attr.td names the first alloc_size argument "ElemSizeParam" and the
second optional argument "NumElemsParam"; but the semantics of how the
two-argument version is used in practice is the opposite of that.

glibc declares calloc() like this, so the second alloc_size argument is
the element size:
```
extern void *calloc (size_t __nmemb, size_t __size)
__THROW __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __wur;
```

The Linux kernel declares array allocation functions like
`kmalloc_array_noprof()` the same way.

Add a comment explaining that the names used inside clang are
misleading.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Apr 8, 2025
@llvmbot
Copy link
Member

llvmbot commented Apr 8, 2025

@llvm/pr-subscribers-clang

Author: Jann (thejh)

Changes

Attr.td names the first alloc_size argument "ElemSizeParam" and the second optional argument "NumElemsParam"; but the semantics of how the two-argument version is used in practice is the opposite of that.

glibc declares calloc() like this, so the second alloc_size argument is the element size:

extern void *calloc (size_t __nmemb, size_t __size)
__THROW __attribute_malloc__ __attribute_alloc_size__ ((1, 2)) __wur;

The Linux kernel declares array allocation functions like kmalloc_array_noprof() the same way.

Add a comment explaining that the names used inside clang are misleading.


Full diff: https://github.com/llvm/llvm-project/pull/134899.diff

1 Files Affected:

  • (modified) clang/include/clang/Basic/Attr.td (+7)
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index fd9e686485552..b7ad432738b29 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -1709,6 +1709,13 @@ def EmptyBases : InheritableAttr, TargetSpecificAttr<TargetMicrosoftCXXABI> {
 def AllocSize : InheritableAttr {
   let Spellings = [GCC<"alloc_size">];
   let Subjects = SubjectList<[HasFunctionProto]>;
+  // The parameter names here are a bit misleading.
+  // When used with a single argument, the first argument is obviously the
+  // allocation size; but when used with two arguments, the first argument is
+  // usually the number of elements, while the second argument is usually the
+  // element size - the reverse of how they are named here.
+  // The documentation of both GCC and clang does not describe any semantic
+  // difference between the first and second argument.
   let Args = [ParamIdxArgument<"ElemSizeParam">,
               ParamIdxArgument<"NumElemsParam", /*opt*/ 1>];
   let TemplateDependent = 1;

@thejh
Copy link
Contributor Author

thejh commented Apr 8, 2025

cc @gburgessiv who originally introduced the attribute in e376337

Copy link
Member

@gburgessiv gburgessiv left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding the clarity!

@thejh
Copy link
Contributor Author

thejh commented Apr 8, 2025

@gburgessiv I do not have commit access, please land the change for me.

@gburgessiv gburgessiv merged commit 8d90a54 into llvm:main Apr 8, 2025
14 checks passed
@gburgessiv
Copy link
Member

Done, thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants