Page MenuHomePhabricator

[OpenCL] Fix serialization of OpenCLExtensionDecls
ClosedPublic

Authored by AlexeySachkov on Oct 12 2018, 9:02 AM.

Details

Summary

I recently discovered that adding the following code into opencl-c.h causes
failure of test/Headers/opencl-c-header.cl:

#pragma OPENCL EXTENSION cl_my_ext : begin
void cl_my_ext_foobarbaz();
#pragma OPENCL EXTENSIOn cl_my_ext : end

Clang crashes at the assertion is ASTReader::getGlobalSubmoduleID():

assert(I != M.SubmoduleRemap.end() && "Invalid index into submodule index remap");

The root cause of the problem that to deserialize OPENCL_EXTENSION_DECLS
section ASTReader needs to deserialize a Decl contained in it. In turn,
deserializing a Decl requires information about whether this declaration is
part of a (sub)module, but this information is not read yet because it is
located further in a module file.

Diff Detail

Repository
rC Clang

Event Timeline

AlexeySachkov created this revision.Oct 12 2018, 9:02 AM

Can you add a test case please?

The patch looks fine but since I don't know much about opencl I'll leave the LGTM to someone that actually knows this code.

test/Headers/opencl-pragma-extension-begin.h
1 ↗(On Diff #169851)

Is this newline intentional?

Removed unnecessary empty line from test

@yaxunl could you please review this patch?

It fixes a bug in a feature implemented by you in:

commit c6fb598a301143e9d21156a012cc6ef669ff0188                                                        
Author: Yaxun Liu <Yaxun.Liu@amd.com>                                                                  
Date:   Sun Dec 18 05:18:55 2016 +0000                                                                 
                                                                                                       
    Recommit r289979 [OpenCL] Allow disabling types and declarations associated with extensions        
                                                                                                       
    Fixed undefined behavior due to cast integer to bool in initializer list.                          
                                                                                                       
                                                                                                       
    git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@290056 91177308-0d34-0410-b5e6-96231b3b80d8
Anastasia accepted this revision.Oct 19 2018, 10:45 AM
Anastasia added inline comments.
test/Headers/opencl-pragma-extension-begin.cl
1 ↗(On Diff #169978)

I think the tests in this folder are for standard includes only but you are testing custom include file here.

Could this be integrated into test/SemaOpenCL/extension-begin.cl? Or if else you could just move to that folder (it might be better to append module to the name in that case).

11 ↗(On Diff #169978)

May be it makes sense to test the added extension?

This revision is now accepted and ready to land.Oct 19 2018, 10:45 AM
Anastasia requested changes to this revision.Oct 19 2018, 10:45 AM
This revision now requires changes to proceed.Oct 19 2018, 10:45 AM
AlexeySachkov added inline comments.Oct 20 2018, 12:34 AM
test/Headers/opencl-pragma-extension-begin.cl
1 ↗(On Diff #169978)

I think the tests in this folder are for standard includes only but you are testing custom include file here.

Currently, standard include file doesn't contain begin/end ocl pragma directives and I decided to create a special one header for the test.

I will integrate it into existing test for extension-begin pragma.

Updated tests

Anastasia accepted this revision.Oct 26 2018, 7:20 AM

LGTM! Thanks!

test/SemaOpenCL/extension-begin.cl
43–44

Is this a typo? Should this be 'enabled' instead of 'disabled'?

This revision is now accepted and ready to land.Oct 26 2018, 7:20 AM
AlexeySachkov added inline comments.Oct 26 2018, 9:27 AM
test/SemaOpenCL/extension-begin.cl
43–44

I left the diagnostic message the same as it was. Looks like it is a bug in the diagnostic. I can try to fix it, but I would like to do it in a separate patch

Anastasia added inline comments.Oct 26 2018, 11:11 AM
test/SemaOpenCL/extension-begin.cl
43–44

Sure! Thanks!

This revision was automatically updated to reflect the committed changes.