This is an archive of the discontinued LLVM Phabricator instance.

Differential D53200

[OpenCL] Fix serialization of OpenCLExtensionDecls
ClosedPublic

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

Details

Reviewers
Anastasia
yaxunl
JDevlieghere
Commits
rG3ccd033c4e4e: Merging r345497:
rG52d674d97499: [OpenCL] Fix serialization of OpenCLExtensionDecls
rL347834: Merging r345497:
rL345497: [OpenCL] Fix serialization of OpenCLExtensionDecls
rC345497: [OpenCL] Fix serialization of OpenCLExtensionDecls
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
asavonic added a reviewer: JDevlieghere.Oct 12 2018, 9:12 AM
sidorovd added a subscriber: sidorovd.Oct 15 2018, 8:09 AM
JDevlieghere added a comment.Oct 15 2018, 10:28 AM

Can you add a test case please?

AlexeySachkov updated this revision to Diff 169851.Oct 16 2018, 10:23 AM

Added test

JDevlieghere added a comment.Oct 16 2018, 2:54 PM

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?

AlexeySachkov updated this revision to Diff 169978.Oct 17 2018, 3:11 AM

Removed unnecessary empty line from test

AlexeySachkov added a comment.Oct 17 2018, 3:37 AM

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

AlexeySachkov updated this revision to Diff 170601.Oct 23 2018, 4:01 AM

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!

Closed by commit rC345497: [OpenCL] Fix serialization of OpenCLExtensionDecls (authored by asavonic). · Explain WhyOct 29 2018, 4:16 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
lib/
Serialization/
5 lines
test/
SemaOpenCL/
26 lines
58 lines

Diff 171481

lib/Serialization/ASTWriter.cpp

Show First 20 Lines • Show All 5,008 Lines • ▼ Show 20 LinesASTFileSignature ASTWriter::WriteASTCore(Sema &SemaRef, StringRef isysroot,
WriteHeaderSearch(PP.getHeaderSearchInfo()); WriteHeaderSearch(PP.getHeaderSearchInfo());
WriteSelectors(SemaRef); WriteSelectors(SemaRef);
WriteReferencedSelectorsPool(SemaRef); WriteReferencedSelectorsPool(SemaRef);
WriteLateParsedTemplates(SemaRef); WriteLateParsedTemplates(SemaRef);
WriteIdentifierTable(PP, SemaRef.IdResolver, isModule); WriteIdentifierTable(PP, SemaRef.IdResolver, isModule);
WriteFPPragmaOptions(SemaRef.getFPOptions()); WriteFPPragmaOptions(SemaRef.getFPOptions());
WriteOpenCLExtensions(SemaRef); WriteOpenCLExtensions(SemaRef);
WriteOpenCLExtensionTypes(SemaRef); WriteOpenCLExtensionTypes(SemaRef);
WriteOpenCLExtensionDecls(SemaRef);
WriteCUDAPragmas(SemaRef); WriteCUDAPragmas(SemaRef);
// If we're emitting a module, write out the submodule information. // If we're emitting a module, write out the submodule information.
if (WritingModule) if (WritingModule)
WriteSubmodules(WritingModule); WriteSubmodules(WritingModule);
// We need to have information about submodules to correctly deserialize
// decls from OpenCLExtensionDecls block
WriteOpenCLExtensionDecls(SemaRef);
Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes); Stream.EmitRecord(SPECIAL_TYPES, SpecialTypes);
// Write the record containing external, unnamed definitions. // Write the record containing external, unnamed definitions.
if (!EagerlyDeserializedDecls.empty()) if (!EagerlyDeserializedDecls.empty())
Stream.EmitRecord(EAGERLY_DESERIALIZED_DECLS, EagerlyDeserializedDecls); Stream.EmitRecord(EAGERLY_DESERIALIZED_DECLS, EagerlyDeserializedDecls);
if (!ModularCodegenDecls.empty()) if (!ModularCodegenDecls.empty())
Stream.EmitRecord(MODULAR_CODEGEN_DECLS, ModularCodegenDecls); Stream.EmitRecord(MODULAR_CODEGEN_DECLS, ModularCodegenDecls);
▲ Show 20 Lines • Show All 1,912 Lines • Show Last 20 Lines

test/SemaOpenCL/extension-begin.h

#ifndef INCLUDED
#define INCLUDED
#pragma OPENCL EXTENSION all : begin
#pragma OPENCL EXTENSION all : end
#pragma OPENCL EXTENSION my_ext : begin
struct A {
int a;
};
typedef struct A TypedefOfA;
typedef const __private TypedefOfA* PointerOfA;
void f(void);
__attribute__((overloadable)) void g(long x);
#pragma OPENCL EXTENSION my_ext : end
#pragma OPENCL EXTENSION my_ext : end
__attribute__((overloadable)) void g(void);
#endif // INCLUDED

test/SemaOpenCL/extension-begin.cl

// Test this without pch. // Test this without pch.
// RUN: %clang_cc1 %s -DHEADER -DHEADER_USER -triple spir-unknown-unknown -verify -pedantic -fsyntax-only // RUN: %clang_cc1 %s -triple spir-unknown-unknown -verify -pedantic -fsyntax-only
// Test with pch. // Test with pch.
// RUN: %clang_cc1 %s -DHEADER -triple spir-unknown-unknown -emit-pch -o %t -verify -pedantic // RUN: %clang_cc1 -x cl %S/extension-begin.h -triple spir-unknown-unknown -emit-pch -o %t.pch -pedantic
// RUN: %clang_cc1 %s -DHEADER_USER -triple spir-unknown-unknown -include-pch %t -fsyntax-only -verify -pedantic // RUN: %clang_cc1 %s -triple spir-unknown-unknown -include-pch %t.pch -DIMPLICIT_INCLUDE -DUSE_PCH -fsyntax-only -verify -pedantic
#if defined(HEADER) && !defined(INCLUDED) // Test with modules
#define INCLUDED // RUN: rm -rf %t.modules
// RUN: mkdir -p %t.modules
#pragma OPENCL EXTENSION all : begin // expected-warning {{expected 'disable' - ignoring}} //
#pragma OPENCL EXTENSION all : end // expected-warning {{expected 'disable' - ignoring}} // RUN: %clang_cc1 -cl-std=CL1.2 -DIMPLICIT_INCLUDE -include %S/extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.modules %s -verify -pedantic
//
#pragma OPENCL EXTENSION my_ext : begin // RUN: rm -rf %t.modules
// RUN: mkdir -p %t.modules
struct A { //
int a; // RUN: %clang_cc1 -cl-std=CL2.0 -DIMPLICIT_INCLUDE -include %S/extension-begin.h -triple spir-unknown-unknown -O0 -emit-llvm -o - -fmodules -fimplicit-module-maps -fmodules-cache-path=%t.modules %s -verify -pedantic
};
#ifndef IMPLICIT_INCLUDE
typedef struct A TypedefOfA; #include "extension-begin.h"
typedef const TypedefOfA* PointerOfA; #endif // IMPLICIT_INCLUDE
#ifndef USE_PCH
void f(void); // expected-warning@extension-begin.h:4 {{expected 'disable' - ignoring}}
// expected-warning@extension-begin.h:5 {{expected 'disable' - ignoring}}
__attribute__((overloadable)) void g(long x); // expected-warning@extension-begin.h:21 {{OpenCL extension end directive mismatches begin directive - ignoring}}
#endif // USE_PCH
#pragma OPENCL EXTENSION my_ext : end
#pragma OPENCL EXTENSION my_ext : end // expected-warning {{OpenCL extension end directive mismatches begin directive - ignoring}}
__attribute__((overloadable)) void g(void);
#endif // defined(HEADER) && !defined(INCLUDED)
#ifdef HEADER_USER
#pragma OPENCL EXTENSION my_ext : enable #pragma OPENCL EXTENSION my_ext : enable
void test_f1(void) { void test_f1(void) {
struct A test_A1; struct A test_A1;
f(); f();
g(0); g(0);
} }
#pragma OPENCL EXTENSION my_ext : disable #pragma OPENCL EXTENSION my_ext : disable
void test_f2(void) { void test_f2(void) {
struct A test_A2; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}} struct A test_A2; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}}
const struct A test_A_local; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}} const struct A test_A_local; // expected-error {{use of type 'struct A' requires my_ext extension to be enabled}}
TypedefOfA test_typedef_A; // expected-error {{use of type 'TypedefOfA' (aka 'struct A') requires my_ext extension to be enabled}} TypedefOfA test_typedef_A; // expected-error {{use of type 'TypedefOfA' (aka 'struct A') requires my_ext extension to be enabled}}
PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 'const struct A *') requires my_ext extension to be enabled}} PointerOfA test_A_pointer; // expected-error {{use of type 'PointerOfA' (aka 'const struct A *') requires my_ext extension to be enabled}}
f(); // expected-error {{use of declaration 'f' requires my_ext extension to be enabled}} f(); // expected-error {{use of declaration 'f' requires my_ext extension to be enabled}}
g(0); // expected-error {{no matching function for call to 'g'}} g(0); // expected-error {{no matching function for call to 'g'}}
// expected-note@-26 {{candidate unavailable as it requires OpenCL extension 'my_ext' to be disabled}} // expected-note@extension-begin.h:18 {{candidate unavailable as it requires OpenCL extension 'my_ext' to be disabled}}
// expected-note@-22 {{candidate function not viable: requires 0 arguments, but 1 was provided}} // expected-note@extension-begin.h:23 {{candidate function not viable: requires 0 arguments, but 1 was provided}}
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

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

Anastasia: Is this a typo? Should this be 'enabled' instead of 'disabled'?
AlexeySachkovAuthorUnsubmitted
Not Done
ReplyInline Actions

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

AlexeySachkov: I left the diagnostic message the same as it was. Looks like it is a bug in the diagnostic. I…
AnastasiaUnsubmitted
Not Done
ReplyInline Actions

Sure! Thanks!

Anastasia: Sure! Thanks!
} }
#endif // HEADER_USER