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.

Sign up for GitHub

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

[clang] move -Wcast-function-type under -Wextra #77178

Merged
merged 10 commits into from
Mar 20, 2024
Merged

[clang] move -Wcast-function-type under -Wextra #77178

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d147292
[clang] move -Wcast-function-type under -Wextra
Jan 6, 2024
386ea16
Added fixme comment. Removed the extra testcase. Added release note
Abhinkop Feb 18, 2024
d14a0dc
Fixed a typo
Abhinkop Feb 18, 2024
daa462a
Removed added notes by merge conflict
Abhinkop Feb 18, 2024
eb8ad4f
[clang] move -Wcast-function-type under -Wextra
Jan 6, 2024
b2c2569
Added fixme comment. Removed the extra testcase. Added release note
Abhinkop Feb 18, 2024
3774838
Fixed a typo
Abhinkop Feb 18, 2024
f07fb78
Removed added notes by merge conflict
Abhinkop Feb 18, 2024
1cccdb8
Merge branch '76872' of github.com:Abhinkop/llvm-project into 76872
Abhinkop Feb 18, 2024
ec4c425
Reword release note slightly; NFC
AaronBallman Mar 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang/include/clang/Basic/DiagnosticGroups.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1026,6 +1026,7 @@ def Extra : DiagGroup<"extra", [
EmptyInitStatement,
StringConcatation,
FUseLdPath,
CastFunctionType,
]>;

def Most : DiagGroup<"most", [
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Sema/warn-cast-function-type-strict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type -verify
// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type-strict -verify

// RUN: %clang_cc1 %s -fsyntax-only -Wextra -Wno-ignored-qualifiers -verify

int t(int array[static 12]);
int u(int i);
Expand Down
1 change: 1 addition & 0 deletions clang/test/Sema/warn-cast-function-type.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 %s -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -verify
// RUN: %clang_cc1 %s -fsyntax-only -Wextra -Wno-cast-function-type-strict -verify

int x(long);

Expand Down
42 changes: 42 additions & 0 deletions clang/test/Sema/warn-extra-cast-function-type-strict.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// RUN: %clang_cc1 %s -fsyntax-only -Wextra -verify
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written, tests checks only default language mode for C (which is gnu99 AFAIK). Maybe we want to check other modes as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add runs for -std=cNN for all remaining 5 of them? and also add extra test cases where conflicts arise?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, more RUN lines with different -std= options, and potentially more expected directives tailored for particular language mode. We have an example of this in C++ defect report tests: https://github.com/llvm/llvm-project/blob/main/clang/test/CXX/drs/dr6xx.cpp

Note that I might be biased here, as someone who've spent a significant time with C++ defect report test suite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this diagnostic is not already well tested, adding all of these additional unit tests makes sense.

But it looks like there's quite a number of existing tests in other files. I don't think we should add a new file just to check this diagnostics behavior via -Wextra via a whole new file.

Copy link
Contributor Author

@Abhinkop Abhinkop Jan 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of adding extra runs for both -Wcast-function-type and -Wextra or should the one for -Wcast-function-type be done in a different issue?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would just remove the newly created clang/test/Sema/warn-extra-cast-function-type-strict.c. If you have cases that expose bugs in the implementation, that is distinctly a separate issue from adding this warning under -Wextra.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. sounds good I will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. File removed, and a FIXME comment added for the existing test.



int t(int array[static 12]);
int u(int i);
const int v(int i); /* expected-warning {{'const' type qualifier on return type has no effec}} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const int v(int i); /* expected-warning {{'const' type qualifier on return type has no effec}} */
const int v(int i); /* expected-warning {{'const' type qualifier on return type has no effect}} */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't have to do it as the file is now not added.

int x(long);

typedef int (f1)(long);
typedef int (f2)(void*);
typedef int (f3)();
typedef void (f4)();
typedef void (f5)(void);
typedef int (f6)(long, int);
typedef int (f7)(long,...);
typedef int (f8)(int *);
typedef int (f9)(const int);
typedef int (f10)(int);

f1 *a;
f2 *b;
f3 *c;
f4 *d;
f5 *e;
f6 *f;
f7 *g;
f8 *h;
f9 *i;
f10 *j;

void foo(void) {
a = (f1 *)x;
b = (f2 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f2 *' (aka 'int (*)(void *)') converts to incompatible function type}} */
c = (f3 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f3 *' (aka 'int (*)()') converts to incompatible function type}} */
d = (f4 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f4 *' (aka 'void (*)()') converts to incompatible function type}} */
e = (f5 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f5 *' (aka 'void (*)(void)') converts to incompatible function type}} */
f = (f6 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f6 *' (aka 'int (*)(long, int)') converts to incompatible function type}} */
g = (f7 *)x; /* expected-warning {{cast from 'int (*)(long)' to 'f7 *' (aka 'int (*)(long, ...)') converts to incompatible function type}} */
h = (f8 *)t;
i = (f9 *)u;
j = (f10 *)v; /* expected-warning {{cast from 'const int (*)(int)' to 'f10 *' (aka 'int (*)(int)') converts to incompatible function type}} */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On line 6 we issue a diagnostic that const qualifier doesn't have effect on return type, yet we issue a diagnostic here telling that this makes function pointer type incompatible. It feels like we fail to make up our minds on the matter, or that we are overly pedantic without pedantic warnings enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, even I found this strange. Is there a place we can verify which is the correct behavior?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope @AaronBallman can clarify.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the diagnostic here is incorrect.

C23 6.7.6.3p4:
If, in the declaration "T D1", D1 has the form
D ( parameter-type-listopt ) attribute-specifier-sequenceopt
and the type specified for ident in the declaration "T D" is "derived-declarator-type-list T", then the
type specified for ident is "derived-declarator-type-list function returning the unqualified, non-atomic
version of T". The optional attribute specifier sequence appertains to the function type.

So the type of the function does not include the qualifiers on the return type, which means that const int f(void) and int f(void) should result in the same type.

We get this wrong: https://godbolt.org/z/xvh449xrd

I think this is a more general problem with how we model the function type: https://godbolt.org/z/G3vxjW9dh

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, create an issue? Comment out this test case and put a comment relating to the new issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to create an issue, we've already got one: #39494

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably worth it to add a FIXME comment above the test case and point to that issue, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So finally the things. I need to do:
Add FIXME comment and leave the test case as is. Remove the clang/test/Sema/warn-extra-cast-function-type-strict.c file . Update release notes. Then update the pull request. Agree? @nickdesaulniers @AaronBallman @Endilll @shafik

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds correct to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
1 change: 1 addition & 0 deletions clang/test/SemaCXX/warn-cast-function-type-strict.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type -verify
// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type-strict -verify
// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wextra -verify

int x(long);

Expand Down
1 change: 1 addition & 0 deletions clang/test/SemaCXX/warn-cast-function-type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wcast-function-type -Wno-cast-function-type-strict -verify
// RUN: %clang_cc1 %s -fblocks -fsyntax-only -Wextra -Wno-cast-function-type-strict -verify

int x(long);

Expand Down