Page MenuHomePhabricator

[Sema] Comparison of pointers to complete and incomplete types
ClosedPublic

Authored by pestctrl on May 14 2020, 8:18 AM.

Details

Summary

Clang is missing one of the conditions for C99 6.5.9p2, where comparison between pointers must either both point to incomplete types or both point to complete types. This patch adds an extra check to the clause where two pointers are of compatible types.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
pestctrl updated this revision to Diff 264089.May 14 2020, 2:05 PM

Updated test to also expect a warning along with the newly added error.

pestctrl updated this revision to Diff 264697.EditedMay 18 2020, 12:27 PM
pestctrl edited the summary of this revision. (Show Details)

Rebased on top of master using arc

@efriedma I think you were the last person to touch this code regarding cases where pointer comparisons are invalid. Could you have a look at my changes and gimme some feedback?

I'm not sure how you derive this requirement from the standard; the section in question doesn't use the words "complete" or "incomplete" at all. Am I missing something obvious?

Oh, wait, I was looking at the C11 version. This diagnostic needs to be restricted to C99 or earlier, and only to relational comparisons, I think.

And given it isn't really a valuable diagnostic, I think it should be off by default (an "Extension" diagnostic).

Ah, you're right. I don't see the clause in the C11 standard. I'll see what I can do. Thanks!

pestctrl updated this revision to Diff 265807.May 22 2020, 3:10 PM

Changed error to warning, only emit during a relational operation

Hey @efriedma, thanks again for the comments.

Yes, you were correct in that this message should only be emitted for relational comparisons.

Regarding having the warning off by default, I did want to point out that GCC enables this warning by default: https://c.godbolt.org/z/W_NgYA.

Do you still think that the warning be off by default? I am OK with either.

Any thoughts on "this diagnostic needs to be restricted to C99 or earlier"?

pestctrl updated this revision to Diff 265898.May 23 2020, 4:06 PM

Added warning to group c99-extensions, only enable warning when C99 or less

pestctrl updated this revision to Diff 265907.May 23 2020, 8:40 PM

Both extension and extwarn need to be in the c99-extensions group

pestctrl updated this revision to Diff 265908.May 23 2020, 10:08 PM

Rebase on master.

pestctrl updated this revision to Diff 266086.May 25 2020, 4:18 PM

Rebase on master?

pestctrl updated this revision to Diff 266214.May 26 2020, 8:06 AM

Forgot to add a comma >_<

efriedma added inline comments.May 26 2020, 12:11 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
6451

ext_typecheck_compare_complete_incomplete_pointers should be InGroup<C11>

6452

warn_typecheck_compare_complete_incomplete_pointers should be a Warning, not an ExtWarn. And should be DefaultIgnore. And should indicate why we're warning. And should be in the group C99Compat.

pestctrl updated this revision to Diff 266645.May 27 2020, 1:15 PM
pestctrl marked an inline comment as done.

ext_typecheck_compare_complete_incomplete_pointers:

  • Moved to group C11.

ext_typecheck_compare_complete_incomplete_pointers:

  • Changed to Warning from ExtWarn
  • Moved to group C99Compat
  • Added DefaultIgnore
pestctrl marked an inline comment as done.May 27 2020, 1:20 PM
pestctrl added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
6452

I'm confused by what you mean for "indicate why we're warning". Should I change the warning text?

pestctrl updated this revision to Diff 266880.May 28 2020, 7:45 AM

clang-format for the test file

pestctrl marked an inline comment as done.May 29 2020, 1:28 PM
efriedma added inline comments.May 29 2020, 2:26 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
6452

Yes; try taking a look at the text of the other C99Compat warnings.

pestctrl updated this revision to Diff 267876.Jun 2 2020, 7:05 AM

Updated warning message to be more descriptive

pestctrl updated this revision to Diff 267901.Jun 2 2020, 8:48 AM

Updated test with new error message

pestctrl updated this revision to Diff 267920.Jun 2 2020, 10:12 AM

Tests need to contain the full error message

pestctrl updated this revision to Diff 267947.Jun 2 2020, 12:04 PM

Copy pasted error message

pestctrl updated this revision to Diff 267950.Jun 2 2020, 12:07 PM

Copy pasted error messages

pestctrl marked an inline comment as done.Jun 3 2020, 11:06 AM

@efriedma Any more comments?

efriedma added inline comments.Jun 8 2020, 4:32 PM
clang/lib/Sema/SemaExpr.cpp
11592

I think this condition is backwards? Should be !getLangOpts().C11. You want the warning with -std=c99 -pedantic, you don't want the warning with std=c11 -pedantic.

pestctrl marked an inline comment as done.Jun 9 2020, 6:53 AM
pestctrl added inline comments.
clang/lib/Sema/SemaExpr.cpp
11592

I don't think it's backwards. If getLangOpts().C11, then it is an extension. Otherwise, it is the warning. I can switch the conditions if it is confusing though.

rsmith added inline comments.Jun 9 2020, 10:25 AM
clang/lib/Sema/SemaExpr.cpp
11592

"Extension" means "this is invalid code that we're accepting anyway" -- that's what this is in C99. In C11, I think we shouldn't be diagnosing at all.

Has anyone checked whether WG14 removed this restriction in C11 as a DR resolution? If so, we shouldn't be diagnosing it at all, in any language mode.

efriedma added inline comments.Jun 9 2020, 11:02 AM
clang/lib/Sema/SemaExpr.cpp
11592

I tracked down the proposal for the change; it's http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1439.pdf . Beyond the reference to http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_314.htm , I can't find any relevant defect report.

pestctrl updated this revision to Diff 269824.Jun 10 2020, 6:31 AM

Don't diagnose outside of C99

pestctrl marked 2 inline comments as done.Jun 10 2020, 7:59 AM
pestctrl added inline comments.
clang/lib/Sema/SemaExpr.cpp
11592

I have updated the diff to diagnose only for C99. Does the existence of the proposal mean we shouldn't be diagnosing in any language mode?

Also, how did you track down the proposal that quickly? Even after skimming through it, I still can't find it through searching.

efriedma added inline comments.Jun 10 2020, 12:18 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
6454

InGroup<C11>

clang/lib/Sema/SemaExpr.cpp
11592

Also, how did you track down the proposal that quickly?

It wasn't really that quick, but the thing that eventually worked was that I googled for site:open-std.org "At various points within a translation unit".

rsmith added inline comments.Jun 10 2020, 1:11 PM
clang/lib/Sema/SemaExpr.cpp
11592

I tried to get some clarity from WG14 as to whether they intended N1439 to be interpreted as applying retroactively, but it seems like their stance is that they do not do maintenance work on past standards, and have no mechanism for identifying whether papers should be encouraged for retroactive application or only for implementations intending to conform to later standards.

In the absence of guidance either way from WG14, I think our best bet is to follow GCC and the literal standards text as this patch does.

pestctrl marked 2 inline comments as done.Jun 10 2020, 2:12 PM
pestctrl added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
6454

Sorry, I'm not sure I understand. Isn't this a C99 warning? Why is it being put in the C11 group?

clang/lib/Sema/SemaExpr.cpp
11592

GCC has left this warning on by default in any language mode, FWIW. Should we still restrict this warning to only C99 mode?

rsmith added inline comments.Jun 10 2020, 3:11 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
6454

Because C11 really means C11Extensions, and this is a C11 extension (ie, it's code that's valid in C11 but not valid in C99):

// A warning group for warnings about using C11 features as extensions.
def C11 : DiagGroup<"c11-extensions">;
clang/lib/Sema/SemaExpr.cpp
11592

Yes, I think so. I assume the GCC folks haven't noticed that this rule was relaxed in C11. I've filed a bug against GCC for this: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95630

pestctrl marked an inline comment as done.Jun 11 2020, 7:39 AM
pestctrl added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
6454

Got it, thank you!

pestctrl updated this revision to Diff 270139.Jun 11 2020, 7:41 AM

Moved the extension to C11 group

pestctrl marked 2 inline comments as done.Jun 13 2020, 8:12 AM
pestctrl marked 5 inline comments as done.Tue, Jun 16, 6:33 AM

@rsmith @efriedma Any more comments?

efriedma accepted this revision.Wed, Jun 17, 1:02 PM

LGTM

I'll merge for you; how do you want to be credited on the "Author" line of the commit message?

This revision is now accepted and ready to land.Wed, Jun 17, 1:02 PM

Benson Chu <bensonchu457@gmail.com>

This revision was automatically updated to reflect the committed changes.