This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by Anastasia on Jan 20 2016, 4:31 AM.

Details

Summary

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

Diff Detail

Event Timeline

ichesnokov retitled this revision from to [FIX] Bug 25404 - Crash on typedef in OpenCL 2.0.
ichesnokov updated this object.
ichesnokov added reviewers: ABataev, davidxl, asl.
ichesnokov set the repository for this revision to rL LLVM.
asl edited edge metadata.Jan 20 2016, 6:11 AM

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?

ichesnokov edited edge metadata.
ABataev edited edge metadata.Jan 21 2016, 2:28 AM

Tests?

ichesnokov added inline comments.Jan 21 2016, 3:18 AM
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 edited edge metadata.
ichesnokov removed rL LLVM as the repository for this revision.

New diff without redundant svn:ignore

Please, use the latest Diff (3).

asl added inline comments.Jan 24 2016, 10:29 AM
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 added a subscriber: arsenm.Jan 26 2016, 10:43 AM

Add cfe-commits

ichesnokov marked an inline comment as done.Jan 27 2016, 3:19 AM
ichesnokov added inline comments.
lib/Sema/SemaDecl.cpp
2039

It is not tab, it is triple space

ichesnokov marked 2 inline comments as done.
ichesnokov set the repository for this revision to rL LLVM.

Removed unuseful patch code.

ichesnokov added inline comments.Jan 30 2016, 1:23 AM
test/SemaOpenCL/implicit-typedef.cl
3

Not necessary. Removed.

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.

Please review and commit.

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

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

ichesnokov added a comment.EditedFeb 1 2016, 1:09 AM

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 added inline comments.
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 removed rL LLVM as the repository for this revision.

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 marked 3 inline comments as done.Feb 2 2016, 4:02 AM
ichesnokov added inline comments.
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 added inline comments.Feb 3 2016, 10:37 AM
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 marked 2 inline comments as done.

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

ichesnokov marked 3 inline comments as done.Feb 3 2016, 10:17 PM
ichesnokov added inline comments.
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 marked 3 inline comments as done.Feb 3 2016, 10:25 PM

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

Anastasia added inline comments.Feb 4 2016, 7:36 AM
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 added inline comments.Feb 4 2016, 8:04 AM
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 added inline comments.Feb 4 2016, 10:54 AM
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;

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 added inline comments.Feb 9 2016, 8:47 AM
test/SemaOpenCL/implicit-typedef.cl
1

do you need -pedantic here?

Anastasia resigned from this revision.Jul 12 2016, 10:13 AM
Anastasia removed a reviewer: Anastasia.

Is this still needed? The bug is still open

Anastasia commandeered this revision.Feb 22 2019, 6:05 AM
Anastasia added a reviewer: ichesnokov.

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

Anastasia abandoned this revision.Feb 22 2019, 6:06 AM

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