[FIX] Bug 25404 - Crash on typedef in OpenCL 2.0

[Anastasia]

Summary

Fixed by adding isImplicit() check.
Please make review of the patch.

[asl]

You also need a test

SemaDecl.cpp

2036 ↗

(On Diff #45371)

Do not reference to bugs. Instead, describe the reason.

2037 ↗

(On Diff #45371)

Do not leave commented out code.

2038 ↗

(On Diff #45371)

Why this is generic and not OpenCL-specific?

[ABataev]

Tests?

[• ichesnokov]

SemaDecl.cpp

2038 ↗

(On Diff #45498)

Added isImplicit() check, because implicit TypeDecl::getLocation()
returns 0. The're many implicit typedefs in OpenCL, e.g. atomic_flag.

[• ichesnokov]

Added test to https://llvm.org/bugs/show_bug.cgi?id=25404

[• ichesnokov]

New diff without redundant svn:ignore

[• ichesnokov]

Please, use the latest Diff (3).

[asl]

lib/Sema/SemaDecl.cpp

2039

Please follow the LLVM's coding style guidelines. E.g. *never* use tabs.

test/SemaOpenCL/implicit-typedef.cl

2

Is -main-file-name necessary?

3

This won't work w/o other options, e.g. -verify and so on.

[arsenm]

Add cfe-commits

[• ichesnokov]

lib/Sema/SemaDecl.cpp

2039

It is not tab, it is triple space

[• ichesnokov]

Removed unuseful patch code.

[• ichesnokov]

test/SemaOpenCL/implicit-typedef.cl

3

Not necessary. Removed.

[• ichesnokov]

BugZilla page: https://llvm.org/bugs/show_bug.cgi?id=25404

Please review and submit this bug.

test/SemaOpenCL/implicit-typedef.cl

3

-main-file-name is not necessary. Removed.

[• ichesnokov]

BugZilla page:
https://llvm.org/bugs/show_bug.cgi?id=25404

[• ichesnokov]

Please review and commit.

[majnemer]

Why isn't this logic buried in GetArgumentVector so that other tools may
take advantage of it?

[• ichesnokov]

majnemer,
I'll try to find answer to your question tomorrow.

[• ichesnokov]

BugZilla page: https://llvm.org/bugs/show_bug.cgi?id=25404

[• ichesnokov]

majnemer: Why isn't this logic buried in GetArgumentVector so that other tools may take advantage of it?

Sorry, I do not understand what you say.
I looked into GetArgumentVector and this function is dummy now in both Unix and Windows, and it does not look like correct place to implement this fix.
Could you please clarify what the change do you propose?

This fix is global, not only for OpenCL. The patch is .cl because initial bug report is for OpenCL.
What can I do is generalizing the bug by adding patch in .c.

[Anastasia]

lib/Sema/SemaDecl.cpp

2038

Braces are not needed.

test/SemaOpenCL/implicit-typedef.cl

2

"-cc1" is not needed. I have a feeling most of the things are not needed here.

Generally doesn't seem to follow the style. Could you please check other tests as an example. Let's say test/SemaOpenCL/extern.cl.

3

So now we just silently allow to redefine a builtin OpenCL type. Wondering if giving a warning would be a better option?

[• ichesnokov]

Warnings added to the case. Turned off by default:
Original bahavior:
" If we have a redefinition of a typedef in C, emit a warning. This warning
is normally mapped to an error, but can be controlled with
-Wtypedef-redefinition. If either the original or the redefinition is
in a system header, don't emit this for compatibility with GCC.
"
The similar is applied to implicit/builtin typedefs.
It needs to define -Wsystem-headers to see the warning.

[• ichesnokov]

test/SemaOpenCL/implicit-typedef.cl

3

Test case command line fixed.

3

Added a warning (reused existing C/C++). Warning is checked in test case.

[Anastasia]

lib/Sema/SemaDecl.cpp

2039

redifinition -> redefinition

test/SemaOpenCL/implicit-typedef.cl

2

Could we also add a run line without -Wsystem-headers.

You can pass -D and use #ifdef for checking the error conditionally in this test.

3

atomic_flag is a part of OpenCL not C11 here.

I think it would be nicer to change this error to use select and pass either C11 or OpenCL.

[• ichesnokov]

Test case enriched to check typedef redefinition without warnings.
Minor fix of comment.

[• ichesnokov]

lib/Sema/SemaDecl.cpp

2039

Fixed.

test/SemaOpenCL/implicit-typedef.cl

3

Done.

4

Are you mean another text of warning? Notice that actual patch will work not only in OpenCL, but any language. If builtin functions present, it will prevent the crash. The single message "redefinition of typedef 'atomic_flag' is a C11 feature" is default for all targets.
Do you want to have specific warning text for OpenCL?

[• ichesnokov]

Notice that redefinition of typedef defined at system header will show the same message.

[Anastasia]

test/SemaOpenCL/implicit-typedef.cl

4

No, you can use select{OpenCL|C11} or you can pass 'OpenCL' or 'C11' as a string to the diagnostic as an argument.

You can take a look at err_opencl_unknown_type_specifier as an example, that uses both approaches.

LangOpts.OpenCL will help you to detect the language mode for selecting/passing the right string.

[• ichesnokov]

test/SemaOpenCL/implicit-typedef.cl

4

I can't find anything like select{} in my local copy (there's LLVM and Clang).
I also can't find err_opencl_unknown_type_specifier, such file is not presernt in my WC (updated today).

LangOpts.OpenCL will help you to detect the language mode for selecting/passing the right string.

Currently it checks both Microsoft and Itanium manglers on all platforms, with one test case.
I am not sure how to use LangOpts.OpenCL... We are making .cl test suite and C++ classes will be unavailble here.

Excuse me, could you please explain more?

[Anastasia]

test/SemaOpenCL/implicit-typedef.cl

4

Example of select is in include/clang/Basic/DiagnosticSemaKinds.td line 7707 at revision 259811.

def err_opencl_unknown_type_specifier : Error<
  "OpenCL does not support the '%0' %select{type qualifier|storage class specifier}1">;

Regarding the OpenCL check you can call getLangOpts().OpenCL to check if you compile for OpenCL (see examples in lib/Sema/SemaDecl.cpp).
You can do something like:

bool IsOpenCL  = getLangOpts().OpenCL ? 1: 0;  
Diag(New->getLocation(), diag::ext_redefinition_of_typedef)
            << New->getDeclName() << IsOpenCL;

[• ichesnokov]

Another warning text implemented for OpenCL.
Like: redefinition of OpenCL builtin typedef 'atomic_flag'

test/SemaOpenCL/implicit-typedef.cl

4

Thank you, Anastasia!
I just posted new patch.

[Anastasia]

test/SemaOpenCL/implicit-typedef.cl

1

do you need -pedantic here?

[arsenm]

Is this still needed? The bug is still open

[Anastasia]

I just closed the bug because it's no longer failing on the master branch. So I don't think this is needed.

[Anastasia]

I just took the ownership of this to be able to close it.

include/clang/Basic/DiagnosticSemaKinds.td

   " in the declaration statement in the program scope">;
 def err_opencl_implicit_vector_conversion : Error<
   "implicit conversions between vector types (%0 and %1) are not permitted">;
+def warn_opencl_redefinition_of_typedef : Warning<
+  "redefinition of OpenCL builtin typedef %0">;
 
 // OpenCL v2.0 s6.13.6 -- Builtin Pipe Functions
 def err_opencl_builtin_pipe_first_arg : Error<

lib/Sema/SemaDecl.cpp

   if (getLangOpts().Modules || getLangOpts().C11)
     return;
   
+  // Added isImplicit() check, because implicit TypeDecl::getLocation()
+  // returns 0. The're many implicit typedefs in OpenCL, e.g. atomic_flag.
+  if (Old->isImplicit() || New->isImplicit()) {
+    // Since we don't emit system header warnings for compatibility with GCC,
+    // don't do this for implicit type redefinition warnings the same way.
+    if (!getDiagnostics().getSuppressSystemWarnings()) {
+      if (New->getLocation().isValid()) {
+        if (getLangOpts().OpenCL) {
+          Diag(New->getLocation(), diag::warn_opencl_redefinition_of_typedef)
+            << New->getDeclName();
+        } else {
+          Diag(New->getLocation(), diag::ext_redefinition_of_typedef)
+            << New->getDeclName();
+        }
+        if (Old->getLocation().isValid())
+          Diag(Old->getLocation(), diag::note_previous_definition);
+      }
+    }
+    return;
+  }
+
   // If we have a redefinition of a typedef in C, emit a warning.  This warning
   // is normally mapped to an error, but can be controlled with
   // -Wtypedef-redefinition.  If either the original or the redefinition is

test/SemaOpenCL/implicit-typedef.cl

+// RUN: %clang_cc1 %s -DTEST_WARNINGS -cl-std=CL2.0 -verify -pedantic -fsyntax-only -Wsystem-headers
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -fsyntax-only
+
+#if defined(TEST_WARNINGS)
+typedef atomic_int atomic_flag; // expected-warning {{redefinition of OpenCL builtin typedef 'atomic_flag'}}
+#else
+typedef atomic_int atomic_flag;
+#endif