delcypher added inline comments.
================
Comment at: clang/test/Sema/warn-calls-without-prototype.c:39
+ return a + b +c;
+}
+
----------------
NoQ wrote:
> delcypher wrote:
> > delcypher wrote:
> > > delcypher wrote:
> > > > @NoQ Any ideas about this? It seems kind of weird that when merging
> > > > `not_a_prototype3` prototype with the K&R style definition of
> > > > `not_a_prototype3` that the resulting FunctionDecl we see at the call
> > > > site in `call_to_function_without_prototype3` is marked as not having a
> > > > prototype.
> > > >
> > > > If I flip the order (see `not_a_prototype6`) then the merged
> > > > declaration is marked as having a prototype.
> > > >
> > > > I'm not sure if this is a bug in `Sema::MergeFunctionDecl` or if this
> > > > just a peculiarity of K&R style function definitions.
> > > I suspect the problem might be here in `Sema::MergeFunctionDecl`.
> > >
> > > ```lang=c++
> > > // C: Function types need to be compatible, not identical. This handles
> > > // duplicate function decls like "void f(int); void f(enum X);"
> > > properly.
> > > if (!getLangOpts().CPlusPlus &&
> > > Context.typesAreCompatible(OldQType, NewQType)) {
> > > const FunctionType *OldFuncType = OldQType->getAs<FunctionType>();
> > > const FunctionType *NewFuncType = NewQType->getAs<FunctionType>();
> > > const FunctionProtoType *OldProto = nullptr;
> > > if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
> > > (OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
> > > // The old declaration provided a function prototype, but the
> > > // new declaration does not. Merge in the prototype.
> > > ```
> > >
> > > ` isa<FunctionNoProtoType>(NewFuncType)` is false in this particular
> > > case, however `New` doesn't have a prototype (i.e. `New->hasPrototype()`
> > > is false). One fix might be to replace
> > > `isa<FunctionNoProtoType>(NewFuncType)` with
> > >
> > > ```
> > > (isa<FunctionNoProtoType>(NewFuncType) || !New->hasPrototype())
> > > ```
> > >
> > > However, I don't really know this code well enough to know if that's the
> > > right fix.
> > Okay. I think the above would actually be the wrong location for a fix
> > because in this case we don't need to go down the path that synthesizes the
> > parameters because we already know them for both `old` and `new` in this
> > situation.
> >
> > Instead I think the change would have to be in
> > `Sema::MergeCompatibleFunctionDecls` to do something like.
> >
> > ```lang=c++
> > // If New is a K&R function definition it will be marked
> > // as not having a prototype. If `Old` has a prototype
> > // then to "merge" we should mark the K&R function as having a prototype.
> > if (!getLangOpts().CPlusPlus && Old->hasPrototype() &&
> > !New->hasPrototype())
> > New->setHasInheritedPrototype();
> > ```
> >
> > What I'm not sure about is if this is semantically the right thing to do.
> > Thoughts?
> Ok dunno but I definitely find this whole thing surprising. I'd expect this
> example to be the entirely normal situation for this code, where it sees that
> the new declaration has no prototype so it "inherits" it from the old
> declaration. But you're saying that
>
> > `isa<FunctionNoProtoType>(NewFuncType)` is false in this particular case
>
> Where does that proto-type come from then? I only see this code affecting the
> type
> ```lang=c++
> 3523 if (RequiresAdjustment) {
> 3524 const FunctionType *AdjustedType =
> New->getType()->getAs<FunctionType>();
> 3525 AdjustedType = Context.adjustFunctionType(AdjustedType,
> NewTypeInfo);
> 3526 New->setType(QualType(AdjustedType, 0));
> 3527 NewQType = Context.getCanonicalType(New->getType());
> 3528 }
> ```
> which doesn't seem to touch no-proto-types:
> ```lang=c++
> 3094 const FunctionType *ASTContext::adjustFunctionType(const FunctionType
> *T,
> 3095
> FunctionType::ExtInfo Info) {
> 3096 if (T->getExtInfo() == Info)
> 3097 return T;
> 3098
> 3099 QualType Result;
> 3100 if (const auto *FNPT = dyn_cast<FunctionNoProtoType>(T)) {
> 3101 Result = getFunctionNoProtoType(FNPT->getReturnType(), Info);
> ...
> 3107 }
> 3108
> 3109 return cast<FunctionType>(Result.getTypePtr());
> 3110 }
> ```
> So it sounds like `New->getType()` was already with prototype from the start?
> Maybe whatever code has set that type should also have set the
> `HasInheritedPrototype` flag?
> Ok dunno but I definitely find this whole thing surprising. I'd expect this
> example to be the entirely normal situation for this code, where it sees that
> the new declaration has no prototype so it "inherits" it from the old
> declaration. But you're saying that
>
> `isa<FunctionNoProtoType>(NewFuncType) `is false in this particular case
Yep.
```
(lldb) p NewFuncType->dump()
FunctionProtoType 0x128829430 'int (int, int)' cdecl
|-BuiltinType 0x12782bf10 'int'
|-BuiltinType 0x12782bf10 'int'
`-BuiltinType 0x12782bf10 'int'
(lldb) p OldFuncType->dump()
FunctionProtoType 0x128829430 'int (int, int)' cdecl
|-BuiltinType 0x12782bf10 'int'
|-BuiltinType 0x12782bf10 'int'
`-BuiltinType 0x12782bf10 'int'
```
I think we might be confusing the `FunctionNoProtoType` type and what the
`FunctionDecl::hasPrototype()` method does.
## `FunctionNoProtoType`
I'm not 100% sure about this but I suspect `FunctionNoProtoType` exists only
for functions without any prototype that are written like this.
```
int function();
```
which would make some sense given what this code tries to do.
```lang=c++
if (MergeTypeWithOld && isa<FunctionNoProtoType>(NewFuncType) &&
(OldProto = dyn_cast<FunctionProtoType>(OldFuncType))) {
// The old declaration provided a function prototype, but the
// new declaration does not. Merge in the prototype.
...
```
i.e. I think it's supposed to handle code like this
```
int foo(int, int, int); // prototype
int foo(); // has no prototype, but when we merge decls it inherits the
prototype from the previous decl.
```
## `FunctionDecl::hasPrototype()`
This method is implemented as
```
bool hasPrototype() const {
return hasWrittenPrototype() || hasInheritedPrototype();
}
```
and those implementations get the data from data in the class without examining
its `FunctionType`. So it seems a `FunctionDecl`'s property of having a
prototype is independent of that FunctionDecl's `FunctionType`.
## K&R function declarations
> So it sounds like New->getType() was already with prototype from the start?
> Maybe whatever code has set that type should also have set the
> HasInheritedPrototype flag?
The nasty thing about K&R function declarations is they are treated as not
having a prototype according to the C11 standard (6.9.1.13) .
```lang=c
extern int max(int a, int b)
{
return a > b ? a : b;
}
```
```lang=c
extern int max(a, b)
int a, b;
{
return a > b ? a : b;
}
```
//Here int a, b; is the declaration list for the parameters. The difference
between these two definitions is that the first form acts as a prototype
declaration that forces conversion of the arguments of subsequent calls to the
function, whereas the second form does not.//
As much as I hate this, given the above it makes to make that in
`Sema::MergeFunctionDecl` `New->hasPrototype()` returns false. So in my example
```lang=c
unsigned not_a_prototype3(int a, int b, int c); // "Old" has a prototype
unsigned not_a_prototype3(a, b, c ) // "New" has no prototype
int a;
int b;
int c;
{
return a + b +c;
}
```
The question is, what should we do when trying to merge these two FunctionDecls?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116635/new/
https://reviews.llvm.org/D116635
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
