Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvement: better diagnostics for undefined behavior #134372

Closed
pmor13 opened this issue Apr 4, 2025 · 4 comments
Closed

Improvement: better diagnostics for undefined behavior #134372

pmor13 opened this issue Apr 4, 2025 · 4 comments
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer quality-of-implementation wontfix Issue is real, but we can't or won't fix it. Not invalid

Comments

@pmor13
Copy link

pmor13 commented Apr 4, 2025

Compiling this code:

void foo(void)
{
    struct x* y;
    *y;
}

with -O3 -std=c11 -pedantic -Wall -Wextra leads to:

<source>:4:5: error: incomplete type 'struct x' where a complete type is required
    4 |     *y;

For people who learned C11 by reading C11 this diagnostic may be confusing because C11 does not require the operand of the unary * operator to be pointer type, which points to an object of a complete type.

Expected diagnostics:

<source>:4:5: error: lvalue conversion: lvalue has an incomplete type
    4 |     *y;

@AaronBallman

@AaronBallman AaronBallman added clang Clang issues not falling into any other category quality-of-implementation clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer and removed new issue labels Apr 4, 2025
@AaronBallman
Copy link
Collaborator

I'm not convinced the new diagnostic is better than the old one; the wording is saying the same thing for the same reasons. A complete type is required because *y dereferences a pointer and then lvalue conversion happens which requires the object to be a complete type. So an incomplete type is showing up where a complete type is required, and it's doing so because of lvalue conversion. However, I don't think average users really should have to think in terms of "lvalue" and "rvalue" as those are more language lawyer-type terms.

@EugeneZelenko EugeneZelenko removed the clang Clang issues not falling into any other category label Apr 4, 2025
@pmor13
Copy link
Author

pmor13 commented Apr 4, 2025

@AaronBallman A complete type is not required because the C11 does not require it. (Therefore, people who learned C11 by reading C11 this "is required" diagnostic may be confusing.) Lvalue conversion does not require the object to be a complete type.

C11, 6.3.2.1p2:

If the lvalue has an incomplete type and does not have array type, the behavior is undefined.

The code above triggers UB.

The "is required" diagnostic may be interpreted as "is required by an implementation". (So, I'm OK with that.)

@AaronBallman
Copy link
Collaborator

@AaronBallman A complete type is not required because the C11 does not require it. (Therefore, people who learned C11 by reading C11 this "is required" diagnostic may be confusing.) Lvalue conversion does not require the object to be a complete type.

C11, 6.3.2.1p2:

If the lvalue has an incomplete type and does not have array type, the behavior is undefined.

The code above triggers UB.

The "is required" diagnostic may be interpreted as "is required by an implementation". (So, I'm OK with that.)

A complete type is required in order to have a well-defined program, so yes, lvalue conversion does require the type to be complete. Also, I don't think we should worry about people who learned C11 by reading the standard, that's not the average user we're optimizing diagnostic wording for. Therefore, I think this issue should be closed as Won't Fix; the existing diagnostic wording is correct and explains the issue, the proposed wording doesn't help the programmer to understand what's wrong with their code.

Also worth noting that while this was UB in older standards mode, it's a constraint violation in C2y (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3481.pdf)

@pmor13
Copy link
Author

pmor13 commented Apr 4, 2025

Thanks for n3481.pdf. (I wondered "why UB rather than a constraint?", so the n3481.pdf fixes it.)

OK for "Won't Fix".

@AaronBallman AaronBallman added the wontfix Issue is real, but we can't or won't fix it. Not invalid label Apr 7, 2025
@AaronBallman AaronBallman closed this as not planned Won't fix, can't repro, duplicate, stale Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:diagnostics New/improved warning or error message in Clang, but not in clang-tidy or static analyzer quality-of-implementation wontfix Issue is real, but we can't or won't fix it. Not invalid
Projects
None yet
Development

No branches or pull requests

4 participants