[SEMA] ignore duplicate declaration specifiers from typeof exprs

[nickdesaulniers]

Summary

Create a third diagnostic type for duplicate declaration specifiers. Previously, we had an ExtWarn and a Warning. This change adds a third, Extension, which only warns when -pedantic is set, staying silent otherwise.

The common cases where each are used are roughly:
Extension: pre-C99 (added by this change)
ExtWarn: C++
Warning: _Noreturn, inline, other C++ keywords

Where the preexisting behavior is mostly kept intact. This change intentionally removes warnings for duplicated _Atomic and restrict, since they were not available until C11 and C99 respectively, and we not longer wish to warn for duplicates for C99 and newer.

Fixes PR32985.

[nickdesaulniers]

git-clang-format HEAD~

[joel]

I built Linux using this patch applied to trunk and it worked for me. Thanks!

[manojgupta]

As per https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80868, I thought GCC also emits this error but only with -pedantic. So probably should keep this error but under -Wextra or another appropriate group?

[nickdesaulniers]

• warn only if -pedantic, add gcc bug test cases

[nickdesaulniers]

• git-clang-format HEAD~

[lebedev.ri]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

This ideally needs positive tests. E.g.:

• -std=c89
• -std=c89 -pedantic
• -std=gnu99
• -std=gnu99 -pedantic
• -std=c99
• -std=c99 -pedantic

16 ↗

(On Diff #166352)

There is no otherwise check prefix in the run lines.

[manojgupta]

lgtm. But someone more familiar with these code paths should approve.

[nickdesaulniers]

• move test to new file, use check-prefix for both cases

[nickdesaulniers]

• add line numbers to match specific warning lines

[srhines]

lib/Sema/SemaType.cpp

1682

This is broken for C11 and C17 (even before you touch anything). As we just talked about, let's have a helper function to detect the oldest version (and maybe even that should get promoted as a LANGOPT).

[nickdesaulniers]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

Since typeof is a gnu extension, its use constitutes an error for all non gnu C standards, so it's moot to check for duplicate const specifiers from typeof exprs.

Since we're trying to match GCC's behavior here, GCC does not warn for -std=gnu99 or -std=gnu99 -pedantic so I will add those test cases.

[nickdesaulniers]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

https://godbolt.org/z/3trZdl

[nickdesaulniers]

• also run test on gnu99+

lib/Sema/SemaType.cpp

1682

From the declarations in include/clang/Frontend/LangStandards.def, newer versions of C bitwise OR together flags of previous versions. So C99 sets C99 flag, C11 sets C99 and C11 flags, and C17 sets C99, C11, and C17 flags.

So this should be correct (though even to me, this looks wrong on first glance). Thanks to @george.burgess.iv and you for help in looking into this.

[nickdesaulniers]

test/Sema/gnu89.c

1–2 ↗

(On Diff #166352)

Ah, I can still put CHECKs for errors. Will add additional tests.

[nickdesaulniers]

• add ISO C tests, handle typedef case new tests found

[nickdesaulniers]

• remove debug statments

[nickdesaulniers]

• condense CHECK-prefixes into CHECK for const const

[nickdesaulniers]

test/Sema/gnu89-const.c

41–46

gah, CNU typo!

[nickdesaulniers]

• fix typo s/CNU/GNU/g and update NOTEs

[nickdesaulniers]

• adjust wording in comment

[nickdesaulniers]

This now matches GCC AFAICT. My only question is:

Should GCC instead warn for the typedef case for -std=c89 (non pedantic), according to C90 6.5.3?

[nickdesaulniers]

Should GCC instead warn for the typedef case for -std=c89 (non pedantic), according to C90 6.5.3?

Seems like yes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80868#c6
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=87435
But let's wait a bit to see what the GCC folks say. If the newer bug doesn't get closed, then I need to rework this patch.

[rsmith]

The right way to produce diagnostics under pedantic mode is to model them as Extension or ExtWarn in the .td file, not by checking the Pedantic diagnostic option directly. If this is an intentional GNU extension too, that makes things a bit more complex (one approach would be to have two diagnostics, one for "this is ill-formed" and another for "this is a GNU extension").

[nickdesaulniers]

• split duplicate_declspec into Extension and Extwarn from Extwarn.
• rename ext_duplicate_declspec to be the Extension, not the Extwarn.
• Fix C++ case.
• Use tablegen'd Extension rather than checking Pedantic diag option.
• Update tests that started failing due to this change.
• Consolidate checks for duplicate declspecs into 1 file.
• Move cases for PR8264 into my added test case, extending them for various language standards.
• Update my tests/rename the new test file name.

[nickdesaulniers]

Ok, I think this is ready for rereview.

test/Parser/atomic.c

5 ↗

(On Diff #167565)

Note to reviewers: this deletion was intentional. It is not copied over to the new test file test/Sema/dupl-declspec.c because we now DONT want to warn for duplicates in C99+, and _Atomic was not available before C11.

test/Sema/declspec.c

43 ↗

(On Diff #167565)

Note to reviewers; this deletion was intentional. It is not copied over to the new test file test/Sema/dupl-declspec.c with the rest of the PR8264 cases because we now DONT want to warn for duplicates in C99+, and restrict was not available before then (ie. C90).

The rest of these cases were moved to the new test file test/Sema/dupl-declspec.c and exhaustively tested against all current C standards.

[nickdesaulniers]

test/FixIt/fixit.c

50 ↗

(On Diff #167565)

Ah, this was a case I should add one last test for. I think reviewers can still start reviewing the rest. This should just add one additional test case.

[nickdesaulniers]

• add back test for typedef typedef
• add test for dupl _Atomic and restrict

[nickdesaulniers]

test/FixIt/fixit.c

50 ↗

(On Diff #167565)

Added test case back.

test/Parser/atomic.c

5 ↗

(On Diff #167565)

Actually, we can still test that it does not appear.

test/Sema/declspec.c

43 ↗

(On Diff #167565)

Actually, we can still test that it does not appear.

[nickdesaulniers]

• move back untouched test case

[rsmith]

Downgrading the C89 warning for duplicate qualifiers from typedefs / __typeof from ExtWarn to Extension seems very reasonable to me. However, I don't see a justification for removing the warning for duplicate explicit (non-typedef) qualifiers in C99, nor for downgrading the corresponding ExtWarn to an Extension in our other language modes; that seems like a regression.

include/clang/Basic/DiagnosticSemaKinds.td

197 ↗

(On Diff #167845)

Please follow our naming convention for diagnostic names: both Extension and ExtWarn diagnostics should start ext_.

lib/Sema/DeclSpec.cpp

441–442 ↗

(On Diff #167845)

This doesn't look like a correct change. When IsExtension is false, the duplicate is not ill-formed, so we shouldn't use an ExtWarn diagnostic (which results in an error under -pedantic-errors).

854–857 ↗

(On Diff #167845)

We still want to warn on duplicate decl specifiers in C99 mode.

[nickdesaulniers]

lib/Sema/DeclSpec.cpp

441–442 ↗

(On Diff #167845)

If the duplicate is not ill-formed, then isn't the original code incorrect, since it produces a warning?

[nickdesaulniers]

[SEMA] split ExtWarn dupl-decl-spec's into Extension and ExtWarn

For types deduced from typedef's and typeof's, don't warn for duplicate
declaration specifiers in C90 unless -pedantic.

Create a third diagnostic type for duplicate declaration specifiers.
Previously, we had an ExtWarn and a Warning. This change adds a third,
Extension, which only warns when -pedantic is set, staying silent
otherwise.

Fixes PR32985.

[nickdesaulniers]

Well that didn't do what I wanted...(sorry, still learning arc). Going to abandon this and push a much simpler fix.

[nickdesaulniers]

Created: https://reviews.llvm.org/D52849

lib/Sema/SemaType.cpp

if (TypeQuals && Result->isReferenceType()) {
           S, DS, TypeQuals, Result,
           DeclSpec::TQ_const | DeclSpec::TQ_volatile | DeclSpec::TQ_atomic,
           diag::warn_typecheck_reference_qualifiers);
     }
 
     // C90 6.5.3 constraints: "The same type qualifier shall not appear more
     // than once in the same specifier-list or qualifier-list, either directly
     // or via one or more typedefs."
-    if (!S.getLangOpts().C99 && !S.getLangOpts().CPlusPlus
-        && TypeQuals & Result.getCVRQualifiers()) {
+    //
+    // C90 and GNU89 do not warn if the TST is from a typedef unless -pedantic
+    // was set. GNU89 additionally does not warn if the TST is from a typeof
+    // expression and -pedantic was not set.
+    if (!S.getLangOpts().C99 && !S.getLangOpts().CPlusPlus &&
+        TypeQuals & Result.getCVRQualifiers() &&
+        (S.Diags.getDiagnosticOptions().Pedantic ||
+         !(DS.getTypeSpecType() == DeclSpec::TST_typename ||
+           (S.getLangOpts().GNUMode &&
+            DS.getTypeSpecType() == DeclSpec::TST_typeofExpr)))) {
       if (TypeQuals & DeclSpec::TQ_const && Result.isConstQualified()) {
         S.Diag(DS.getConstSpecLoc(), diag::ext_duplicate_declspec)
           << "const";
       }
 
       if (TypeQuals & DeclSpec::TQ_volatile && Result.isVolatileQualified()) {
         S.Diag(DS.getVolatileSpecLoc(), diag::ext_duplicate_declspec)
           << "volatile";

test/Sema/gnu89-const.c

+/*
+RUN: not %clang_cc1 %s -std=c89 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C89 %s
+RUN: not %clang_cc1 %s -std=c89 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C89-PEDANTIC %s
+RUN: not %clang_cc1 %s -std=c99 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C99 %s
+RUN: not %clang_cc1 %s -std=c99 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C99-PEDANTIC %s
+RUN: not %clang_cc1 %s -std=c11 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C11 %s
+RUN: not %clang_cc1 %s -std=c11 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C11-PEDANTIC %s
+RUN: not %clang_cc1 %s -std=c17 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C17 %s
+RUN: not %clang_cc1 %s -std=c17 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-C17-PEDANTIC %s
+
+RUN: %clang_cc1 %s -std=gnu89 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU89 %s
+RUN: %clang_cc1 %s -std=gnu89 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU89-PEDANTIC %s
+RUN: %clang_cc1 %s -std=gnu99 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU99 %s
+RUN: %clang_cc1 %s -std=gnu99 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU99-PEDANTIC %s
+RUN: %clang_cc1 %s -std=gnu11 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU11 %s
+RUN: %clang_cc1 %s -std=gnu11 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU11-PEDANTIC %s
+RUN: %clang_cc1 %s -std=gnu17 -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU17 %s
+RUN: %clang_cc1 %s -std=gnu17 -pedantic -fsyntax-only 2>&1 | FileCheck -check-prefix=CHECK-GNU17-PEDANTIC %s
+*/
+
+const const int c_i;
+/*
+CHECK: 21:7: warning: duplicate 'const' declaration specifier
+*/
+
+typedef const int t;
+const t c_i2;
+/*
+CHECK-C89-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-C89-PEDANTIC: 27:1: warning: duplicate 'const' declaration specifier
+          ^ NOTE: special case
+CHECK-C99-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-C99-PEDANTIC-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-C11-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-C11-PEDANTIC-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-C17-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-C17-PEDANTIC-NOT: 27:1: warning: duplicate 'const' declaration specifier
+
+CHECK-GNU89-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU89-PEDANTIC: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU99-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU99-PEDANTIC-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU11-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU11-PEDANTIC-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU17-NOT: 27:1: warning: duplicate 'const' declaration specifier
+CHECK-CNU17-PEDANTIC-NOT: 27:1: warning: duplicate 'const' declaration specifier
+*/
+
+const int c_i3;
+const typeof(c_i) c_i4;
+/*
+CHECK-C89: 50:19: error: expected function body after function declarator
+CHECK-C89-PEDANTIC: 50:19: error: expected function body after function declarator
+CHECK-C99: 50:19: error: expected function body after function declarator
+CHECK-C99-PEDANTIC: 50:19: error: expected function body after function declarator
+CHECK-C11: 50:19: error: expected function body after function declarator
+CHECK-C11-PEDANTIC: 50:19: error: expected function body after function declarator
+CHECK-C17: 50:19: error: expected function body after function declarator
+CHECK-C17-PEDANTIC: 50:19: error: expected function body after function declarator
+
+CHECK-GNU89-NOT: 50:1: warning: duplicate 'const' declaration specifier
+            ^ NOTE: special case
+CHECK-GNU89-PEDANTIC: 50:1: warning: duplicate 'const' declaration specifier
+            ^ NOTE: special case
+CHECK-GNU99-NOT: 50:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU99-PEDANTIC-NOT: 50:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU11-NOT: 50:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU11-PEDANTIC-NOT: 50:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU17-NOT: 50:1: warning: duplicate 'const' declaration specifier
+CHECK-GNU17-PEDANTIC-NOT: 50:1: warning: duplicate 'const' declaration specifier
+*/