clang-tidy: bugprone-multi-level-implicit-pointer-conversion should ignore malloc/calloc/realloc/free

[seanm]

clang-tidy's bugprone-multi-level-implicit-pointer-conversion gives 48 warnings in my codebase. None are actual bugs. All but 2 of them are warning about malloc/calloc/realloc/reallocf/free.

Seems to me that warnings on these functions is not often going to be useful:

• malloc returns void*, but usually you're not assigning to an actual void*
• free takes void*, but often one passes some specific type, not an actual void*

Might it be appropriate to not issue such warnings when these function are involved?

Or perhaps add on option allowing such functions to be listed for suppression?

[llvmbot]

@llvm/issue-subscribers-clang-tidy

Author: Sean McBride (seanm)

clang-tidy's `bugprone-multi-level-implicit-pointer-conversion` gives 48 warnings in my codebase. None are actual bugs. All but 2 of them are warning about malloc/calloc/realloc/reallocf/free.

Seems to me that warnings on these functions is not often going to be useful:

• malloc returns void*, but usually you're not assigning to an actual void*
• free takes void*, but often one passes some specific type, not an actual void*

Might it be appropriate to not issue such warnings when these function are involved?

Or perhaps add on option allowing such functions to be listed for suppression?

[carlosgalvezp]

Can you show some example code? The warning is intended for explicit casts to void, which one shouldn't do with these functions.

[carlosgalvezp]

For example, this code does not trigger warnings:
https://godbolt.org/z/od7W6E4jT

[seanm]

@carlosgalvezp sorry for the delay, it's been a busy day :)

Here is an example:

https://godbolt.org/z/d4W3hMhf4

typedef struct
{} Foo;

int main()
{
    Foo** list = malloc(5 * sizeof(Foo*));
    free(list);
}

IMHO, adding casts here to silence these warnings does not add to readability of the code. IMHO, because malloc/free are "special" in that they alloc/free anything, this warning has no value for these special functions. (it's certainly useful in general!)

[carlosgalvezp]

Hmm, if the intention is to allocate 5 objects of type Foo, wouldn't we do:

Foo* list = malloc(5 * sizeof(Foo));

?

I don't work with C so I don't know if this is standard practice, hopefully someone else can chime in. But coming from C++ I can't help but see warnings signs :)

[seanm]

No, it's creating a list of 5 pointers to Foo.

If this were C++, probably you wouldn't use malloc/free at all, per the cppcoreguidelines-no-malloc check.

[NagyDonat]

I don't work with C so I don't know if this is standard practice, hopefully someone else can chime in.

The example is a bit misleading with the literal 5, but with a dynamic size this is very standard practice, because in C there is no std::vector, so you always need malloc/free when you need an array (or other data structure) without knowing its size at compile time. There are many situations when you need a dynamically-sized collection of pointers:

• an array of strings i.e. char * pointers;
• an array of FILE * handles;
• an array of pointers to struct objects which are allocated from a central pool and handled via pointers because they are too large and copying them is ineffective;
• a collection of nodes in a graph...

... and in each of these situations you need to cast between void * (untyped memory) and some pointer-to-pointer type (because an array is represented by a pointer to its first element, and in this case the first element is already a pointer).

As this pattern is practically unavoidable in C (and AFAIK is customarily written without explicit casts), I definitely support whitelisting it in this tidy check.

In fact, I would suggest whitelisting all casts that involve the type void *, because it is semantically a "pointer-to-anything" type and should be compatible with anything that has 1 or more pointer levels. (And this is not just a theoretical question: if a project defines a custom wrapper around malloc that still returns void *, then this check still shouldn't report casting that void * to e.g. a char ** array-of-strings type.)

[carlosgalvezp]

Thanks for the input, I understand the use case better now.

It seems however directly conflicting with the purpose of the check; detecting this is one of the main use cases, as per the documentation:

For example, converting a pointer to a pointer to an int (int**) to a void* can result in the loss of information

I can turn the question around then and ask:

• Is there a use case in C where this check is actually useful?
• Would it make sense for this check to be enabled only in C++, not in C?

If we find more use cases than the void* one, we could update the documentation to include it, and provide an opt-in option to disable warnings when the cast involves void*.

[NagyDonat]

I re-read the documentation more carefully again, and it seems that detecting casts like void * to int ** is the main goal of this check. At first I thought that the "core" goal of this check was reporting e.g. int * to int ** conversions, but I was mistaken – those are already reported by the compiler itself (as "incompatible pointer types" compilation errors, unless -fpermissive is used), so there is no reason to have a tidy check for them.

Based on this, I would say that this is simply a heavily opinionated check, which may be useful for projects that prefer a more explicit coding style in this particular somewhat arbitrary area; but most of the developers should simply disable and ignore it. (Under C++ this restriction is much less painful, because void * is rarely needed, but I still wouldn't suggest this check for a "default" C++ project.)

I don't think that we could modify this checker extract a "useful for most projects" core from the current overzealous behavior because the useful core already exists as a compiler error. Instead, I would suggest clarifying the documentation to highlight that this check complains about implicit conversions which are perfectly well-defined and do appear in most C codebases as the idiomatic solution. (The current documentation is a propaganda piece, which is technically correct [as far as I see], but sounds as if this check was fighting against the root of all evil in software.)

Personally, I would also support outright removing this check, because I think that its limited usefulness for projects that really need these explicit casts for the added verbosity is outweighed by the pain and annoyance caused to developers who happen to use this check (either accidentally or as an overzealous company policy). Don't include footguns in our tool...

[seanm]

Well, I defer to you guys.

I can say that in all the various codebases I've run clang-tidy against, I don't think I've ever seen a true positive from this one.

As I said before, adding casts in the malloc/free cases (in C) I find too uglifying, which is what motivated me to file this ticket. But I can just as well exclude this check entirely from my .clang-tidy file I suppose.

[carlosgalvezp]

a heavily opinionated check

Yes, clang-tidy has many opinionated checks, and they don't necessarily need to point out bugs in the code. Some checks exist to make the code comply with style guidelines, or to add warning signs (in the form of NOLINT suppressions, casts, etc) for developers (and auditors) to easily spot dangerous code, which can hopefully help refactoring (now or in the future, by the original author or by someone else with new ideas).

But since they are opinionated, they won't suit everyone, so I think best is to simply not include that check in the .clang-tidy file.

I do see how the check can be annoying in C code since it's idiomatic and there are no other alternatives, so perhaps a good start could be to enable it only for C++ and see if we get more feedback?

Personally I find the check useful to detect people writing C-style code in our C++ project, instead of using safer C++ alternatives.

[NagyDonat]

I can say that in all the various codebases I've run clang-tidy against, I don't think I've ever seen a true positive from this one.

As I said before, adding casts in the malloc/free cases (in C) I find too uglifying, which is what motivated me to file this ticket.

@seanm I concur with your assessment that this check is a useless nuisance on a C project. The only difference is that you only suggest ignoring malloc/etc. (to "keep the good parts" of the check), while I feel that there are no "good parts" (especially for C code) so I'm suggesting more drastic solutions.

Personally I find the check useful to detect people writing C-style code in our C++ project, instead of using safer C++ alternatives.

@carlosgalvezp That's a good point! Restricting this checker to C++ could be a good way forward, however in this case the documentation should also be adjusted to highlight that the role of this checker is that it discourages C-style coding.

I'd still be uncomfortable about the arbitrary nature of focusing on "multi-level conversions", because in my opinion void * ⇌ int ** conversions are not inherently more problematic than e.g. void * ⇌ int *. (Note that void ** ⇌ int ** conversions are forbidden by the standard, so when you want to convert an int ** to a generic void pointer, then void * is the natural target type – and not void **.) A "don't implicitly convert pointers to/from void *" check would be a natural and reasonable tool for projects that want clean C++ and I would be happier to see that instead of this one.

[seanm]

Restricting this checker to C++ could be a good way forward, however in this case the documentation should also be adjusted to highlight that the role of this checker is that it discourages C-style coding.

This seems fine to me.