[Sema] Comparison of pointers to complete and incomplete types

[pestctrl]

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.

[pestctrl]

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

[pestctrl]

Rebased on top of master using arc

[pestctrl]

@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?

[efriedma]

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?

[efriedma]

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

[pestctrl]

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

[pestctrl]

Changed error to warning, only emit during a relational operation

[pestctrl]

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.

[efriedma]

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

[pestctrl]

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

[pestctrl]

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

[pestctrl]

Rebase on master.

[pestctrl]

Rebase on master?

[pestctrl]

Forgot to add a comma >_<

[efriedma]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6448

ext_typecheck_compare_complete_incomplete_pointers should be InGroup<C11>

6449

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]

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]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6449

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

[pestctrl]

clang-format for the test file

[efriedma]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6449

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

[pestctrl]

Updated warning message to be more descriptive

[pestctrl]

Updated test with new error message

[pestctrl]

Tests need to contain the full error message

[pestctrl]

Copy pasted error message

[pestctrl]

Copy pasted error messages

[pestctrl]

@efriedma Any more comments?

[efriedma]

clang/lib/Sema/SemaExpr.cpp

11571

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]

clang/lib/Sema/SemaExpr.cpp

11571

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]

clang/lib/Sema/SemaExpr.cpp

11571

"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]

clang/lib/Sema/SemaExpr.cpp

11571

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]

Don't diagnose outside of C99

[pestctrl]

clang/lib/Sema/SemaExpr.cpp

11571

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]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6451

InGroup<C11>

clang/lib/Sema/SemaExpr.cpp

11571

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]

clang/lib/Sema/SemaExpr.cpp

11571

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]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6451

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

11571

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

[rsmith]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6451

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

11571

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]

clang/include/clang/Basic/DiagnosticSemaKinds.td

6451

Got it, thank you!

[pestctrl]

Moved the extension to C11 group

[pestctrl]

@rsmith @efriedma Any more comments?

[efriedma]

LGTM

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

[pestctrl]

Benson Chu <[e-mail]>

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def ext_typecheck_ordered_comparison_of_pointer_integer : ExtWarn<
   "ordered comparison between pointer and integer (%0 and %1)">;
 def ext_typecheck_ordered_comparison_of_pointer_and_zero : Extension<
   "ordered comparison between pointer and zero (%0 and %1) is an extension">;
 def err_typecheck_ordered_comparison_of_pointer_and_zero : Error<
   "ordered comparison between pointer and zero (%0 and %1)">;
 def err_typecheck_three_way_comparison_of_pointer_and_zero : Error<
   "three-way comparison between pointer and zero">;
+def ext_typecheck_compare_complete_incomplete_pointers : Extension<
+  "pointer comparisons before C11 "
+  "need to be between two complete or two incomplete types; "
+  "%0 is %select{|in}2complete and "
+  "%1 is %select{|in}3complete">,
+  InGroup<C99Compat>;
 def ext_typecheck_ordered_comparison_of_function_pointers : ExtWarn<
   "ordered comparison of function pointers (%0 and %1)">,
   InGroup<DiagGroup<"ordered-compare-function-pointers">>;
 def ext_typecheck_comparison_of_fptr_to_void : Extension<
   "equality comparison between function pointer and void pointer (%0 and %1)">;
 def err_typecheck_comparison_of_fptr_to_void : Error<
   "equality comparison between function pointer and void pointer (%0 and %1)">;
 def ext_typecheck_comparison_of_pointer_integer : ExtWarn<

clang/lib/Sema/SemaExpr.cpp

if ((LHSType->isIntegerType() && !LHSIsNull) ||
     QualType LCanPointeeTy =
       LHSType->castAs<PointerType>()->getPointeeType().getCanonicalType();
     QualType RCanPointeeTy =
       RHSType->castAs<PointerType>()->getPointeeType().getCanonicalType();
 
     // C99 6.5.9p2 and C99 6.5.8p2
     if (Context.typesAreCompatible(LCanPointeeTy.getUnqualifiedType(),
                                    RCanPointeeTy.getUnqualifiedType())) {
+      if (IsRelational) {
+        // Pointers both need to point to complete or incomplete types
+        if ((LCanPointeeTy->isIncompleteType() !=
+             RCanPointeeTy->isIncompleteType()) &&
+            !getLangOpts().C11) {
+          Diag(Loc, diag::ext_typecheck_compare_complete_incomplete_pointers)
+              << LHS.get()->getSourceRange() << RHS.get()->getSourceRange()
+              << LHSType << RHSType << LCanPointeeTy->isIncompleteType()
+              << RCanPointeeTy->isIncompleteType();
+        }
+        if (LCanPointeeTy->isFunctionType()) {
           // Valid unless a relational comparison of function pointers
-      if (IsRelational && LCanPointeeTy->isFunctionType()) {
           Diag(Loc, diag::ext_typecheck_ordered_comparison_of_function_pointers)
               << LHSType << RHSType << LHS.get()->getSourceRange()
               << RHS.get()->getSourceRange();
         }
+      }
     } else if (!IsRelational &&
                (LCanPointeeTy->isVoidType() || RCanPointeeTy->isVoidType())) {
       // Valid unless comparison between non-null pointer and function pointer
       if ((LCanPointeeTy->isFunctionType() || RCanPointeeTy->isFunctionType())
           && !LHSIsNull && !RHSIsNull)
         diagnoseFunctionPointerToVoidComparison(*this, Loc, LHS, RHS,
                                                 /*isError*/false);
     } else {

clang/test/Sema/complete-incomplete-pointer-relational-c99.c

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c99 -Wc99-compat %s
+// RUN: %clang_cc1 -fsyntax-only -verify -std=c89 -Wc99-compat %s
+
+int incomplete[]; // expected-warning {{tentative array definition assumed to have one element}}
+int complete[6];
+
+int test_comparison_between_incomplete_and_complete_pointer() {
+  return (&incomplete < &complete) &&  // expected-warning {{pointer comparisons before C11 need to be between two complete or two incomplete types; 'int (*)[]' is incomplete and 'int (*)[6]' is complete}}
+         (&incomplete <= &complete) && // expected-warning {{pointer comparisons before C11 need to be between two complete or two incomplete types; 'int (*)[]' is incomplete and 'int (*)[6]' is complete}}
+         (&incomplete > &complete) &&  // expected-warning {{pointer comparisons before C11 need to be between two complete or two incomplete types; 'int (*)[]' is incomplete and 'int (*)[6]' is complete}}
+         (&incomplete >= &complete) && // expected-warning {{pointer comparisons before C11 need to be between two complete or two incomplete types; 'int (*)[]' is incomplete and 'int (*)[6]' is complete}}
+         (&incomplete == &complete) &&
+         (&incomplete != &complete);
+}