Update wording on __attribute__((malloc))

[Quuxplusone]

• Clang doesn't support the two-argument version. This is Missing support for GCC's attribute malloc (deallocator) and malloc (deallocator, ptr-index) llvm/llvm-project#53152 .
• Clang 21 isn't a thing yet.
• Emphasize the "optimization" we're talking about (noalias); I believe that's the only optimization we're talking about here (and if not, we should be even more explicit, because it's fooled me). Add an example of the optimization to the relevant Godbolt, since it was missing any example before.
• Fix two trivial "cplusplis" typos.

Addresses my comments in #201.

[thomasnyman]

Thanks for the contribution. Content-wise I think this looks good. Left a couple of nits on the formatting to make it consistent with the rest of the guide. In addition, for us to consider these changes, you would need to to confirm the LF DCO by leaving your sign-off in the commit message since the OpenSSF Charter requires DCOs for all inbound contributions (see https://github.com/ossf/tac/blob/main/dco.md).

[thesamesam]

I don't see why we're talking about noalias here, which is an LLVM IR term. Nor is it really useful I think to talk about specifics like that here, it's prone to becoming stale and isn't going to apply to GCC.

I also think it's kind of obvious that there'll be optimisations from something knowing is a malloc and noalias doesn't particularly tell us much.

[Quuxplusone]

Changed "associated noalias optimization" to "no-alias optimization." I think it's important to

Emphasize the "optimization" we're talking about (noalias); I believe that's the only optimization we're talking about here (and if not, we should be even more explicit, because it's fooled me).

I don't think it's important to shy away from the term "noalias," since that is literally the optimization we're talking about: the optimization that says "This thing that by default you think alias? It no alias." 😛 The LLVM IR terminology comes from the nature of the optimization, not the other way around (and actually — although this surprised me too — when you google "noalias optimization," the first two hits are in Visual Studio docs, not LLVM docs!).

I can't parse your second sentence: "I also think it's kind of obvious that there'll be optimisations from something knowing is a malloc and noalias doesn't particularly tell us much."

[thesamesam]

I think noalias is LLVM-specific terminology and I don't see a benefit to using it.

I can't parse your second sentence: "I also think it's kind of obvious that there'll be optimisations from something knowing is a malloc and noalias doesn't particularly tell us much."

I'm saying that it's either:
a) it's LLVM-specific terminology and you're using it to aid researching what LLVM does, or
b) it's obvious that the malloc attribute is going to help with that optimisation, so noalias isn't telling you anything

I really don't see a point in adding it. We could add a lot of details like this throughout the guide for e.g. the access attribute and GCC's object size pass, I don't see how it helps the reader. The guide is already quite verbose.

If you would really like to include the information, please explain it in detail instead of noalias, but I'd still say that's unnecessary and irrelevant for this document.

[thesamesam]

I think not having Clang here at all is confusing as it implies you can't safely use the attribute, no? You can safely use it, just it doesn't do anything.

[Quuxplusone]

It produces a warning (or an error with -Werror), which in practice should break your build. If you're not compiling with -Werror — or if you're letting warnings happen and not fixing them — I think that's vastly more of a departure from "security best practices" than omitting to use some attribute.

I guess one criterion to answer "Does Clang support this attribute?" is to see whether a cross-platform build system like CMake or Meson believes that Clang supports it. I grepped github for __attribute__((malloc, and one of the top hits was this commit to Meson: OpenRC/openrc@17de4e5 —

# Meson's has_function_attribute doesn't know about GCC's extended
# version (with arguments), so we have to build a test program instead.
malloc_attribute_test = '''#include<stdlib.h>
__attribute__ ((malloc (free, 1)))
int func() { return 0; }
'''
if cc.compiles(malloc_attribute_test, name : 'malloc attribute with arguments')
  add_project_arguments('-DHAVE_MALLOC_EXTENDED_ATTRIBUTE', language: 'c')
endif

When you feed this to Meson on a Clang system, does it indeed set -DHAVE_MALLOC_EXTENDED_ATTRIBUTE=1? If it does, then I'll (grudgingly :)) accept that in practice Clang trunk is regarded as supporting the extended malloc attribute, even though the Clang maintainers themselves say it's not supported yet. Sadly I don't have a Meson system to test this on myself.

[thomasnyman]

I can understand both viewpoints on this. I also see that listing Clang under the "Supported since" column give the wrong impression especially given the current behavior that issues a warning. What I might suggest is to add a note Clang in the description to the effect of "Clang trunk has this attribute, but lacks the corresponding diagnostic (resulting in -Wignored-attributes warning on use)." which we'll update to say Clang 21.0.0 once it is released.

[thesamesam]

was this commit to Meson

Right, my commit to a project using Meson (not Meson itself).

Sadly I don't have a Meson system to test this on myself.

I think we've maybe reached an impasse as I don't have a Clang trunk system to test it on ;)

If I extract the command line used and the program from meson-log.txt on godbolt, I see this:

<source>:2:17: warning: 'malloc' attribute only applies to return values that are pointers [-Wignored-attributes]
    2 | __attribute__ ((malloc (free, 1)))
      |                 ^
    3 | int func() { return 0; }
      | ~~~
1 warning generated.
Compiler returned: 0

so I think yes, it'll pass?

[eli-schwartz]

It produces a warning (or an error with -Werror), which in practice should break your build. If you're not compiling with -Werror — or if you're letting warnings happen and not fixing them — I think that's vastly more of a departure from "security best practices" than omitting to use some attribute.

The idea that -Werror must be used and fail the build on any form of error is an admirable one, but it necessarily has a rough interaction with the fact that C/C++ compilers don't have a structured way to distinguish between coding style (opinionated) warnings and semantic warnings.

In general, it is most emphatically NOT a code smell or a violation of security best practices to react to warnings -- or even -Werrors -- by determining the warning is too much of "opinionated lint" an silencing it via -Wno-ignored-attributes or similar.

So, eminently reasonable to safely use it everywhere?

[Quuxplusone]

Meh. I think OSSF and I just have different opinions about the meanings of the words "safe" and "best practices." Like, if a feature is not supported on one of my targeted compilers, I'd use a macro to avoid it; e.g.

#if MY_COMPILER_GCC
 #define MY_ATTR_MALLOC __attribute__((malloc, malloc(free)))
#else
 #define MY_ATTR_MALLOC
#endif

[...]

int *func() MY_ATTR_MALLOC { return malloc(sizeof(int)); }

Then I'd "treat warnings as errors" and make sure my code compiled cleanly on all of my targeted compilers.

It sounds like the OSSF way is to put the attribute unconditionally, and then use -Wno-ignored-attributes in the project file (because by definition the project cannot be made clean w.r.t. that warning anymore). In my style, that would be frowned on; we'd prefer to fix the warning rather than disable it for the whole team.

Anyway, as I'm not part of OSSF, I'll abandon this PR.

[thomasnyman]

@Quuxplusone: Thanks for updating the PR. I see that there's still an ongoing discussion related to how the capture the current Clang behavior without causing confusion one way or the other.

Also I need to remind you that for us to be able to merge these changes, you would need to to confirm the LF DCO by leaving your sign-off in the commit message since the OpenSSF Charter requires DCOs for all inbound contributions (see https://github.com/ossf/tac/blob/main/dco.md). (You can do this using git commit --amend --signoff as long as you agree with the DCO)