llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.09k stars 11.6k forks source link

Incorrect implicit int behavior #54065

Open AaronBallman opened 2 years ago

AaronBallman commented 2 years ago

Clang currently accepts the following invalid C code:

void func(*);

It treats it as though the user had written:

void func(int *);

No other C compiler accepts it, and indeed, the implicit int rules in C89 do not allow it (see C89 3.5.2 and 3.5.4.1). Clang should reject this code as being invalid.

https://godbolt.org/z/qWTc73WeG

llvmbot commented 2 years ago

@llvm/issue-subscribers-c

llvmbot commented 2 years ago

@llvm/issue-subscribers-bug

efriedma-quic commented 2 years ago

We report an error with -pedantic-errors, which is our usual benchmark for "are we following the C standard" in similar situations. Maybe this particular case should default to an error, though.

AaronBallman commented 2 years ago

Hmmm, I am not so certain of that. This should not be diagnosed as a warning that defaults to an error; it should be diagnosed as an error (no warning flag). GCC and ICC (and literally every other C compiler I have access to) treats this as an error, not a pedantic warning.

The warning is correct for cases the standard allows implicit int (which was already deprecated in C89): https://godbolt.org/z/WcYccYxor

jakevossen5 commented 2 years ago

If you decide to go through with this change - let me know and I would be willing to help with the implementation :)

AaronBallman commented 2 years ago

I'm convinced this is a bug in Clang and it should be reported as an error. The grammar for a parameter-type-list eventually leads you to parameter-declaration which requires a declaration-specifiers before the declarator or abstract-declarator, and declaration-specifiers requires a type-specifier, so it's not optional in the grammar.

@jakevossen5 -- if you plan to work on the implementation for this, I'd be happy to review it!

junaire commented 2 years ago

Is anyone working on this? If not, can I have a try? ;)

jakevossen5 commented 2 years ago

Hey @junaire, I've been a bit busy but planning on working on it soon. Though if I get too busy again I'll give you a ping 👍

jakevossen5 commented 2 years ago

@AaronBallman I have a basic implementation, but failing attribute tests. What do you think should be the correct behavior here?

https://godbolt.org/z/KfM5bGhhs

AaronBallman commented 2 years ago

@AaronBallman I have a basic implementation, but failing attribute tests. What do you think should be the correct behavior here?

https://godbolt.org/z/KfM5bGhhs

Heh, that's a fun one. Backing up a step:

void func(x); // Not allowed in C89; can't use an identifier list in a function declaration, only a definition. void func(x) {} // Allowed in C89; implicit int for x

Your example is a function declaration without a parameter type list, so I see that as being an invalid function declaration for the same reasons as my first example.

I'm convinced this is a bug in Clang and it should be reported as an error. The grammar for a parameter-type-list eventually leads you to parameter-declaration which requires a declaration-specifiers before the declarator or abstract-declarator, and declaration-specifiers requires a type-specifier, so it's not optional in the grammar.

After looking again because of the above, I noticed there are no optional uses of type-specifier in the C89 standard. :-D

The way this worked was through making declaration-specifiers optional in some places. Specifically:

3.5
declaration-specifiers:
  storage-class-specifier declaration-specifiers<opt>
  type-specifier declaration-specifiers<opt>
  type-qualifier declaration-specifiers<opt>

(The first case allows for: static i = 12;, the third case allows for const i = 12; as in https://godbolt.org/z/vPeo786bq)

and

3.7.1
function-definition:
  declaration-specifiers<opt> declarator declaration-list<opt> compound-statement

(Which allows for func(int i);, func(), and func(a, b) int a, b; {} as in https://godbolt.org/z/6vxWvTszG)

parameter-declaration does not take an optional declaration-specifiers and so, parameters in a type list are required to specify at least a type specifier or a type qualifier. e.g., void f(*); is not valid, but void f(const *); is. This appears to be the way other C compilers are interpreting this: https://godbolt.org/z/WxGh7Wse4

jakevossen5 commented 2 years ago

So messing around and thinking about it some more - regarding this issue, what is the difference between c89 and > c89?

My understanding is that void foo(*); should not be valid in either c89 or >c89, and there is currently no issue with void foo(x); in either c89 or >c89, it is is only when we have *. Do I have that all correct?

I made this spreadsheet to try to understand. Screen Shot 2022-04-11 at 10 31 45 PM

AaronBallman commented 2 years ago

So messing around and thinking about it some more - regarding this issue, what is the difference between c89 and > c89?

Implicit int was a supported feature of C89. It was removed in C99 without a deprecation period. Because of that, implementations (including Clang) supported it as a warning in C99 and later so users had a transition path. (I should note -- I plan to try to strengthen this for Clang 15 so that we warn in C89 and C99, warn-default-to-error in C11 and C17, and then error only in C2x. However, I've not proposed that patch yet and I don't think it should be part of the patch you're working on. Your patch should focus on getting the behavior of the feature correct in C89 mode.)

My understanding is that void foo(); should not be valid in either c89 or >c89, and there is currently no issue with void foo(x); in either c89 or >c89, it is is only when we have . Do I have that all correct?

That's correct.

I made this spreadsheet to try to understand.

Thanks, this is a handy visual! Here's my take:

void func1(x) {}  // Clang should Warn in C89 and up
void func2(x);    // Clang should Error in C89 and up
void func3(*);    // Clang should Error in C89 and up
void func4(*) {}  // Clang should Error in C89 and up

We warn on func1 simply because implicit int is dangerous. func2 is an error because an identifier-list can only be used in a function definition not a declaration. func3 and func4 are errors because * is not a valid declaration-specifier for a parameter-type-list, nor is it an identifier for an identifier-list.

Other good test cases:

// File scope
const i = 12; // Warn
static j = 12; // Warn
const *k = 0; // Warn
static *l = 0;  // Warn
*m = 0; // Error

func(void) { return 12; } // Warn (return type)

struct S {
  x; // Error, type specifier is required
};

int main(void) {
  const n = 12; // Warn
  static o = 12; // Warn
  const *p = 0; // Warn
  static *q = 0; // Warn
  *r = 0; // Error
}
jakevossen5 commented 2 years ago

So I was checking with clang-check -ast-dump and it was func(*) was parsed at func 'void (int *)'. I originally thought this was a semantic analysis failure and was trying to code up a fix in SemaType.cpp, but it seems like it could be a parser error? The comment linked below makes me think there could be some implicit int things going on in the parser that could be going awry.

https://github.com/llvm/llvm-project/blob/7c04454227f5c6bd3f515783950a815970c90558/clang/lib/Sema/SemaType.cpp#L1344

I was poking around and thinking that maybe ParseDeclaratorInternal could be doing something wacky, but haven't narrowed it down yet.

Figured I would check in to see if I am going down the completely wrong path or if it sounds reasonable. Thanks!

AaronBallman commented 2 years ago

I think that comment may be referring to the behavior in Parser::ParseFunctionDefinition() towards they very start of the function where it fudges in an implicit int.

That said, I do expect the issue is a parsing one. One thing I would probably try is setting a breakpoint in Parser::ParseImplicitInt() to see if I could catch that being called in an unexpected manner.

llvmbot commented 8 months ago

@llvm/issue-subscribers-clang-frontend

Author: Aaron Ballman (AaronBallman)

Clang currently accepts the following invalid C code: ``` void func(*); ``` It treats it as though the user had written: ``` void func(int *); ``` No other C compiler accepts it, and indeed, the implicit int rules in C89 do not allow it (see C89 3.5.2 and 3.5.4.1). Clang should reject this code as being invalid. https://godbolt.org/z/qWTc73WeG