Page MenuHomePhabricator

[SEMA] ignore duplicate declaration specifiers from typeof exprs
AbandonedPublic

Authored by nickdesaulniers on Sep 18 2018, 2:04 PM.

Details

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.

Diff Detail

Event Timeline

git-clang-format HEAD~

joel added a subscriber: joel.Sep 19 2018, 12:39 AM

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

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?

  • warn only if -pedantic, add gcc bug test cases
  • git-clang-format HEAD~
Harbormaster completed remote builds in B22887: Diff 166352.
lebedev.ri added inline comments.
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.

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

  • move test to new file, use check-prefix for both cases
  • add line numbers to match specific warning lines
srhines added inline comments.Sep 20 2018, 1:48 PM
lib/Sema/SemaType.cpp
1682 ↗(On Diff #166357)

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).

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.

test/Sema/gnu89.c
1–2 ↗(On Diff #166352)
nickdesaulniers marked an inline comment as done.
  • also run test on gnu99+
lib/Sema/SemaType.cpp
1682 ↗(On Diff #166357)

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.

test/Sema/gnu89.c
1–2 ↗(On Diff #166352)

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

  • add ISO C tests, handle typedef case new tests found
  • remove debug statments
Harbormaster completed remote builds in B23000: Diff 166737.
  • condense CHECK-prefixes into CHECK for const const
test/Sema/gnu89-const.c
41–46 ↗(On Diff #166740)

gah, CNU typo!

  • fix typo s/CNU/GNU/g and update NOTEs
nickdesaulniers marked an inline comment as done.Sep 24 2018, 1:46 PM
  • adjust wording in comment

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?

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 added a subscriber: rsmith.Sep 25 2018, 11:44 AM

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").

  • 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.

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.

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 edited the summary of this revision. (Show Details)Sep 28 2018, 4:51 PM
nickdesaulniers marked 2 inline comments as done.
  • add back test for typedef typedef
  • add test for dupl _Atomic and restrict
nickdesaulniers marked an inline comment as done.Oct 1 2018, 3:38 PM
nickdesaulniers added inline comments.
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 marked an inline comment as done.
  • move back untouched test case
rsmith added a comment.Oct 1 2018, 4:03 PM

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

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

lib/Sema/DeclSpec.cpp
442–443

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).

860–861

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

nickdesaulniers added inline comments.Oct 1 2018, 4:13 PM
lib/Sema/DeclSpec.cpp
442–443

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

[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 abandoned this revision.Oct 3 2018, 3:04 PM

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