[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
This revision was automatically updated to reflect the committed changes. Closed by commit rL284110: Fix for PR30639: CGDebugInfo Null dereference with OpenMP array (authored by ABataev). Changed prior to commit: https://reviews.llvm.org/D25373?vs=74386&id=74485#toc Repository: rL LLVM https://reviews.llvm.org/D25373 Files: cfe/trunk/lib/CodeGen/CGStmtOpenMP.cpp cfe/trunk/test/OpenMP/debug-info-openmp-array.cpp cfe/trunk/test/OpenMP/for_reduction_codegen.cpp cfe/trunk/test/OpenMP/for_reduction_codegen_UDR.cpp cfe/trunk/test/OpenMP/target_firstprivate_codegen.cpp cfe/trunk/test/OpenMP/target_map_codegen.cpp Index: cfe/trunk/test/OpenMP/target_map_codegen.cpp === --- cfe/trunk/test/OpenMP/target_map_codegen.cpp +++ cfe/trunk/test/OpenMP/target_map_codegen.cpp @@ -675,7 +675,7 @@ } } -// CK13: define internal void [[KERNEL]](i[[sz]] [[VLA0:%.+]], i[[sz]] [[VLA1:%.+]], double* {{.+}}[[ARG:%.+]]) +// CK13: define internal void [[KERNEL]](i[[sz]] [[VLA0:%.+]], i[[sz]] [[VLA1:%.+]], double* {{.*}}[[ARG:%.+]]) // CK13: [[ADDR0:%.+]] = alloca i[[sz]], // CK13: [[ADDR1:%.+]] = alloca i[[sz]], // CK13: [[ADDR2:%.+]] = alloca double*, Index: cfe/trunk/test/OpenMP/for_reduction_codegen_UDR.cpp === --- cfe/trunk/test/OpenMP/for_reduction_codegen_UDR.cpp +++ cfe/trunk/test/OpenMP/for_reduction_codegen_UDR.cpp @@ -301,7 +301,7 @@ // CHECK: fadd float 5.55e+02, % // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) // Reduction list for runtime. // CHECK: [[RED_LIST:%.+]] = alloca [4 x i8*], @@ -500,7 +500,7 @@ // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) // CHECK: [[ARRS_PRIV:%.+]] = alloca [10 x [4 x [[S_FLOAT_TY, Index: cfe/trunk/test/OpenMP/for_reduction_codegen.cpp === --- cfe/trunk/test/OpenMP/for_reduction_codegen.cpp +++ cfe/trunk/test/OpenMP/for_reduction_codegen.cpp @@ -492,7 +492,7 @@ // CHECK: store float [[UP]], float* [[T_VAR1_LHS]], // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) // Reduction list for runtime. // CHECK: [[RED_LIST:%.+]] = alloca [4 x i8*], @@ -696,7 +696,7 @@ // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) // CHECK: [[ARRS_PRIV:%.+]] = alloca [10 x [4 x [[S_FLOAT_TY, Index: cfe/trunk/test/OpenMP/target_firstprivate_codegen.cpp === --- cfe/trunk/test/OpenMP/target_firstprivate_codegen.cpp +++ cfe/trunk/test/OpenMP/target_firstprivate_codegen.cpp @@ -222,7 +222,7 @@ // make sure that firstprivate variables are generated in all cases and that we use those instances for operations inside the // target region - // TCHECK: define void @__omp_offloading_{{.+}}(i{{[0-9]+}} [[A2_IN:%.+]], [10 x float]* {{.+}} [[B_IN:%.+]], i{{[0-9]+}} [[BN_SZ:%.+]], float* {{.+}} [[BN_IN:%.+]], [5 x [10 x double]]* {{.+}} [[C_IN:%.+]], i{{[0-9]+}} [[CN_SZ1:%.+]], i{{[0-9]+}} [[CN_SZ2:%.+]], double* {{.+}} [[CN_IN:%.+]], [[TT]]* {{.+}} [[D_IN:%.+]]) + // TCHECK: define {{.*
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
ABataev added a comment. I will commit it for you, Erich, no problems https://reviews.llvm.org/D25373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
erichkeane updated this revision to Diff 74386. erichkeane marked an inline comment as done. erichkeane added a comment. Fixed Alexey's nit. Will need someone else to commit this, I don't yet have commit access. https://reviews.llvm.org/D25373 Files: lib/CodeGen/CGStmtOpenMP.cpp test/CodeGenCXX/debug-info-openmp-array.cpp test/OpenMP/for_reduction_codegen.cpp test/OpenMP/for_reduction_codegen_UDR.cpp test/OpenMP/target_firstprivate_codegen.cpp test/OpenMP/target_map_codegen.cpp Index: lib/CodeGen/CGStmtOpenMP.cpp === --- lib/CodeGen/CGStmtOpenMP.cpp +++ lib/CodeGen/CGStmtOpenMP.cpp @@ -232,8 +232,15 @@ assert(I->capturesVariableArrayType()); II = &getContext().Idents.get("vla"); } -if (ArgType->isVariablyModifiedType()) - ArgType = getContext().getVariableArrayDecayedType(ArgType); +if (ArgType->isVariablyModifiedType()) { + bool IsReference = ArgType->isLValueReferenceType(); + ArgType = + getContext().getCanonicalParamType(ArgType.getNonReferenceType()); + if (IsReference && !ArgType->isPointerType()) { +ArgType = getContext().getLValueReferenceType( +ArgType, /*SpelledAsLValue=*/false); + } +} Args.push_back(ImplicitParamDecl::Create(getContext(), nullptr, FD->getLocation(), II, ArgType)); ++I; @@ -297,8 +304,14 @@ QualType VarTy = Var->getType(); Address ArgAddr = ArgLVal.getAddress(); if (!VarTy->isReferenceType()) { -ArgAddr = EmitLoadOfReference( -ArgAddr, ArgLVal.getType()->castAs()); +if (ArgLVal.getType()->isLValueReferenceType()) { + ArgAddr = EmitLoadOfReference( + ArgAddr, ArgLVal.getType()->castAs()); +} else { + assert(ArgLVal.getType()->isPointerType()); + ArgAddr = EmitLoadOfPointer( + ArgAddr, ArgLVal.getType()->castAs()); +} } setAddrOfLocalVar( Var, Address(ArgAddr.getPointer(), getContext().getDeclAlign(Var))); Index: test/OpenMP/for_reduction_codegen_UDR.cpp === --- test/OpenMP/for_reduction_codegen_UDR.cpp +++ test/OpenMP/for_reduction_codegen_UDR.cpp @@ -301,7 +301,7 @@ // CHECK: fadd float 5.55e+02, % // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) // Reduction list for runtime. // CHECK: [[RED_LIST:%.+]] = alloca [4 x i8*], @@ -500,7 +500,7 @@ // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) // CHECK: [[ARRS_PRIV:%.+]] = alloca [10 x [4 x [[S_FLOAT_TY, Index: test/OpenMP/target_map_codegen.cpp === --- test/OpenMP/target_map_codegen.cpp +++ test/OpenMP/target_map_codegen.cpp @@ -675,7 +675,7 @@ } } -// CK13: define internal void [[KERNEL]](i[[sz]] [[VLA0:%.+]], i[[sz]] [[VLA1:%.+]], double* {{.+}}[[ARG:%.+]]) +// CK13: define internal void [[KERNEL]](i[[sz]] [[VLA0:%.+]], i[[sz]] [[VLA1:%.+]], double* {{.*}}[[ARG:%.+]]) // CK13: [[ADDR0:%.+]] = alloca i[[sz]], // CK13: [[ADDR1:%.+]] = alloca i[[sz]], // CK13: [[ADDR2:%.+]] = alloca double*, Index: test/OpenMP/for_reduction_codegen.cpp === --- test/OpenMP/for_reduction_codegen.cpp +++ test/OpenMP/for_reduction_codegen.cpp @@ -492,7 +492,7 @@ // CHECK: store float [[UP]], float* [[T_VAR1_LHS]], // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) // Reduction
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG with a nit Comment at: lib/CodeGen/CGStmtOpenMP.cpp:312 + assert(ArgLVal.getType()->isPointerType()); + ArgAddr = this->EmitLoadOfPointer( + ArgAddr, ArgLVal.getType()->castAs()); Remove `this->` https://reviews.llvm.org/D25373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
erichkeane updated the summary for this revision. erichkeane removed a reviewer: gbenyei. erichkeane removed rL LLVM as the repository for this revision. erichkeane updated this revision to Diff 74310. erichkeane added a comment. Emailed with Alexey who identified the problem in the OpenMP implementation that was resulting in incomplete array types being emitted, which broke an assumption in the new CGDebugInfo code. https://reviews.llvm.org/D25373 Files: lib/CodeGen/CGStmtOpenMP.cpp test/CodeGenCXX/debug-info-openmp-array.cpp test/OpenMP/for_reduction_codegen.cpp test/OpenMP/for_reduction_codegen_UDR.cpp test/OpenMP/target_firstprivate_codegen.cpp test/OpenMP/target_map_codegen.cpp Index: lib/CodeGen/CGStmtOpenMP.cpp === --- lib/CodeGen/CGStmtOpenMP.cpp +++ lib/CodeGen/CGStmtOpenMP.cpp @@ -232,8 +232,15 @@ assert(I->capturesVariableArrayType()); II = &getContext().Idents.get("vla"); } -if (ArgType->isVariablyModifiedType()) - ArgType = getContext().getVariableArrayDecayedType(ArgType); +if (ArgType->isVariablyModifiedType()) { + bool IsReference = ArgType->isLValueReferenceType(); + ArgType = + getContext().getCanonicalParamType(ArgType.getNonReferenceType()); + if (IsReference && !ArgType->isPointerType()) { +ArgType = getContext().getLValueReferenceType( +ArgType, /*SpelledAsLValue=*/false); + } +} Args.push_back(ImplicitParamDecl::Create(getContext(), nullptr, FD->getLocation(), II, ArgType)); ++I; @@ -297,8 +304,14 @@ QualType VarTy = Var->getType(); Address ArgAddr = ArgLVal.getAddress(); if (!VarTy->isReferenceType()) { -ArgAddr = EmitLoadOfReference( -ArgAddr, ArgLVal.getType()->castAs()); +if (ArgLVal.getType()->isLValueReferenceType()) { + ArgAddr = EmitLoadOfReference( + ArgAddr, ArgLVal.getType()->castAs()); +} else { + assert(ArgLVal.getType()->isPointerType()); + ArgAddr = this->EmitLoadOfPointer( + ArgAddr, ArgLVal.getType()->castAs()); +} } setAddrOfLocalVar( Var, Address(ArgAddr.getPointer(), getContext().getDeclAlign(Var))); Index: test/OpenMP/for_reduction_codegen_UDR.cpp === --- test/OpenMP/for_reduction_codegen_UDR.cpp +++ test/OpenMP/for_reduction_codegen_UDR.cpp @@ -301,7 +301,7 @@ // CHECK: fadd float 5.55e+02, % // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) // Reduction list for runtime. // CHECK: [[RED_LIST:%.+]] = alloca [4 x i8*], @@ -500,7 +500,7 @@ // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK2]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(480) %{{.+}}) // CHECK: [[ARRS_PRIV:%.+]] = alloca [10 x [4 x [[S_FLOAT_TY, Index: test/OpenMP/target_map_codegen.cpp === --- test/OpenMP/target_map_codegen.cpp +++ test/OpenMP/target_map_codegen.cpp @@ -675,7 +675,7 @@ } } -// CK13: define internal void [[KERNEL]](i[[sz]] [[VLA0:%.+]], i[[sz]] [[VLA1:%.+]], double* {{.+}}[[ARG:%.+]]) +// CK13: define internal void [[KERNEL]](i[[sz]] [[VLA0:%.+]], i[[sz]] [[VLA1:%.+]], double* {{.*}}[[ARG:%.+]]) // CK13: [[ADDR0:%.+]] = alloca i[[sz]], // CK13: [[ADDR1:%.+]] = alloca i[[sz]], // CK13: [[ADDR2:%.+]] = alloca double*, Index: test/OpenMP/for_reduction_codegen.cpp === --- test/OpenMP/for_reduction_codegen.cpp +++ test/OpenMP/for_reduction_codegen.cpp @@ -492,7 +492,7 @@ // CHECK: store float [[UP]], float* [[T_VAR1_LHS]], // CHECK: ret void -// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]+}}* noalias [[GTID_ADDR:%.+]], i{{[0-9]+}}* noalias %{{.+}}, i64 %{{.+}}, i64 %{{.+}}, i32* nonnull %{{.+}}, [2 x i32]* dereferenceable(8) %{{.+}}, [10 x [4 x [[S_FLOAT_TY* dereferenceable(160) %{{.+}}) +// CHECK: define internal void [[MAIN_MICROTASK1]](i{{[0-9]
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
I don't see any .ll files for the tests. I see the directory build/tools/clang/test/OpenMP/Output has a bunch of .script and .tmp(binary files), but no obvious .ll files. -Original Message- From: Alexey Bataev [mailto:a.bat...@hotmail.com] Sent: Monday, October 10, 2016 1:22 PM To: Keane, Erich Cc: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; guy.ben...@intel.com Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access I mean, could you send new .ll files, which you get after these changes? Best regards, Alexey Bataev > 10 окт. 2016 г., в 22:26, Keane, Erich написал(а): > > Sure, attached. So far, there are a few changes that seem curious and > concerning. A few cases of loads going missing, and a few more of things like > 'nonnull' going missing. > > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 12:18 PM > To: Keane, Erich ; > reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; > cfe-commits@lists.llvm.org; dblai...@gmail.com; > david.majne...@gmail.com; guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null > dereference with OpenMP array access > > Could you send the new results for these tests? > > Best regards, > Alexey Bataev > >> On 10/10/2016 09:22 PM, Keane, Erich wrote: >> This causes a massive number of openmp test failures (obviously), and I'm >> not terribly comfortable with how OpenMP works. I can update the tests (and >> will), but would like it if you could be extra diligent in confirming the >> correct behavior for me. >> >> Failing Tests (6): >> Clang :: OpenMP/for_reduction_codegen.cpp >> Clang :: OpenMP/for_reduction_codegen_UDR.cpp >> Clang :: OpenMP/nvptx_target_codegen.cpp >> Clang :: OpenMP/target_codegen.cpp >> Clang :: OpenMP/target_firstprivate_codegen.cpp >> Clang :: OpenMP/target_map_codegen.cpp -Original Message- >> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >> Sent: Monday, October 10, 2016 10:55 AM >> To: Keane, Erich ; >> reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; >> cfe-commits@lists.llvm.org; dblai...@gmail.com; >> david.majne...@gmail.com; guy.ben...@intel.com >> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null >> dereference with OpenMP array access >> >> Add a check to line 299: >> >> if (!VarTy->isReferenceType() && >> !(FD->getType()->isVariablyModifiedType() && >> ArgLVal.getType()->isPointerType())) >> >> Best regards, >> Alexey Bataev >> >>> On 10/10/2016 08:19 PM, Keane, Erich wrote: >>> That does turn ArgType into a PointerType int*. However, line 301 >>> (GenerateOpenMPCapturedStmtFunction) attempts to cast it to a reference >>> type a bit later, resulting in a SIGABRT there. Do I need to re-add the >>> reference there? Is removing the array type REALLY what we wish to do? >>> >>> -----Original Message- >>> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >>> Sent: Monday, October 10, 2016 10:09 AM >>> To: Keane, Erich ; >>> reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; >>> cfe-commits@lists.llvm.org; dblai...@gmail.com; >>> david.majne...@gmail.com; guy.ben...@intel.com >>> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null >>> dereference with OpenMP array access >>> >>> Aaaahhh, >>> >>> Now I see. We have a parameter with the type that is a reference to VLA. >>> I think in this case we should rework this code to something like this: >>> >>> if (ArgType->isVariablyModifiedType()) >>> ArgType = >>> getContext().getCanonicalParamType(ArgType.getNonReferenceType()); >>> >>> >>> Try this solution. >>> >>> Best regards, >>> Alexey Bataev >>> >>>> On 10/10/2016 07:51 PM, Keane, Erich wrote: >>>> I did. I changed line 236 to "ArgType = >>>> getContext().getCanonicalParamType(ArgType);" (as I believe you were >>>> suggesting), and the output was identical when doing dump on ArgType: >>>> >>>> This: >>>> >>>> LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >>>> `-VariableArrayType 0xa4515e0 'int [*]' variably_modified * >>>
Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
I mean, could you send new .ll files, which you get after these changes? Best regards, Alexey Bataev > 10 окт. 2016 г., в 22:26, Keane, Erich написал(а): > > Sure, attached. So far, there are a few changes that seem curious and > concerning. A few cases of loads going missing, and a few more of things like > 'nonnull' going missing. > > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 12:18 PM > To: Keane, Erich ; > reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; > cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; > guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference > with OpenMP array access > > Could you send the new results for these tests? > > Best regards, > Alexey Bataev > >> On 10/10/2016 09:22 PM, Keane, Erich wrote: >> This causes a massive number of openmp test failures (obviously), and I'm >> not terribly comfortable with how OpenMP works. I can update the tests (and >> will), but would like it if you could be extra diligent in confirming the >> correct behavior for me. >> >> Failing Tests (6): >> Clang :: OpenMP/for_reduction_codegen.cpp >> Clang :: OpenMP/for_reduction_codegen_UDR.cpp >> Clang :: OpenMP/nvptx_target_codegen.cpp >> Clang :: OpenMP/target_codegen.cpp >> Clang :: OpenMP/target_firstprivate_codegen.cpp >> Clang :: OpenMP/target_map_codegen.cpp -Original Message- >> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >> Sent: Monday, October 10, 2016 10:55 AM >> To: Keane, Erich ; >> reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; >> cfe-commits@lists.llvm.org; dblai...@gmail.com; >> david.majne...@gmail.com; guy.ben...@intel.com >> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null >> dereference with OpenMP array access >> >> Add a check to line 299: >> >> if (!VarTy->isReferenceType() && >> !(FD->getType()->isVariablyModifiedType() && >> ArgLVal.getType()->isPointerType())) >> >> Best regards, >> Alexey Bataev >> >>> On 10/10/2016 08:19 PM, Keane, Erich wrote: >>> That does turn ArgType into a PointerType int*. However, line 301 >>> (GenerateOpenMPCapturedStmtFunction) attempts to cast it to a reference >>> type a bit later, resulting in a SIGABRT there. Do I need to re-add the >>> reference there? Is removing the array type REALLY what we wish to do? >>> >>> -----Original Message----- >>> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >>> Sent: Monday, October 10, 2016 10:09 AM >>> To: Keane, Erich ; >>> reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; >>> cfe-commits@lists.llvm.org; dblai...@gmail.com; >>> david.majne...@gmail.com; guy.ben...@intel.com >>> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null >>> dereference with OpenMP array access >>> >>> Aaaahhh, >>> >>> Now I see. We have a parameter with the type that is a reference to VLA. >>> I think in this case we should rework this code to something like this: >>> >>> if (ArgType->isVariablyModifiedType()) >>> ArgType = >>> getContext().getCanonicalParamType(ArgType.getNonReferenceType()); >>> >>> >>> Try this solution. >>> >>> Best regards, >>> Alexey Bataev >>> >>>> On 10/10/2016 07:51 PM, Keane, Erich wrote: >>>> I did. I changed line 236 to "ArgType = >>>> getContext().getCanonicalParamType(ArgType);" (as I believe you were >>>> suggesting), and the output was identical when doing dump on ArgType: >>>> >>>> This: >>>> >>>> LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >>>> `-VariableArrayType 0xa4515e0 'int [*]' variably_modified * >>>> |-BuiltinType 0xa3f4090 'int' >>>> `-<<>> >>>> Vs: >>>> LValueReferenceType 0xa451690 'int (&)[*]' variably_modified >>>> `-VariableArrayType 0xa451650 'int [*]' variably_modified * >>>> |-BuiltinType 0xa3f4090 'int' >>>> `-<<>> >>>> >>>> -Original Message- >>>> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >>>> Sent: Monday, Octob
Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Could you send the new results for these tests? Best regards, Alexey Bataev On 10/10/2016 09:22 PM, Keane, Erich wrote: > This causes a massive number of openmp test failures (obviously), and I'm not > terribly comfortable with how OpenMP works. I can update the tests (and > will), but would like it if you could be extra diligent in confirming the > correct behavior for me. > > Failing Tests (6): > Clang :: OpenMP/for_reduction_codegen.cpp > Clang :: OpenMP/for_reduction_codegen_UDR.cpp > Clang :: OpenMP/nvptx_target_codegen.cpp > Clang :: OpenMP/target_codegen.cpp > Clang :: OpenMP/target_firstprivate_codegen.cpp > Clang :: OpenMP/target_map_codegen.cpp > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 10:55 AM > To: Keane, Erich ; > reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; > cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; > guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference > with OpenMP array access > > Add a check to line 299: > > if (!VarTy->isReferenceType() && > !(FD->getType()->isVariablyModifiedType() && > ArgLVal.getType()->isPointerType())) > > Best regards, > Alexey Bataev > > On 10/10/2016 08:19 PM, Keane, Erich wrote: >> That does turn ArgType into a PointerType int*. However, line 301 >> (GenerateOpenMPCapturedStmtFunction) attempts to cast it to a reference type >> a bit later, resulting in a SIGABRT there. Do I need to re-add the >> reference there? Is removing the array type REALLY what we wish to do? >> >> -Original Message- >> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >> Sent: Monday, October 10, 2016 10:09 AM >> To: Keane, Erich ; >> reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; >> cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; >> guy.ben...@intel.com >> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference >> with OpenMP array access >> >> Aaaahhh, >> >> Now I see. We have a parameter with the type that is a reference to VLA. >> I think in this case we should rework this code to something like this: >> >> if (ArgType->isVariablyModifiedType()) >> ArgType = >> getContext().getCanonicalParamType(ArgType.getNonReferenceType()); >> >> >> Try this solution. >> >> Best regards, >> Alexey Bataev >> >> On 10/10/2016 07:51 PM, Keane, Erich wrote: >>> I did. I changed line 236 to "ArgType = >>> getContext().getCanonicalParamType(ArgType);" (as I believe you were >>> suggesting), and the output was identical when doing dump on ArgType: >>> >>> This: >>> >>> LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >>> `-VariableArrayType 0xa4515e0 'int [*]' variably_modified * >>>|-BuiltinType 0xa3f4090 'int' >>>`-<<>> >>> Vs: >>> LValueReferenceType 0xa451690 'int (&)[*]' variably_modified >>> `-VariableArrayType 0xa451650 'int [*]' variably_modified * >>>|-BuiltinType 0xa3f4090 'int' >>>`-<<>> >>> >>> -Original Message- >>> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >>> Sent: Monday, October 10, 2016 9:47 AM >>> To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich >>> ; cfe-commits@lists.llvm.org; dblai...@gmail.com; >>> david.majne...@gmail.com; guy.ben...@intel.com >>> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null >>> dereference with OpenMP array access >>> >>> Hmm, >>> >>> I thought it must return a pointer to the element type rather than [*] kind >>> type. Did you checked it? >>> >>> Best regards, >>> Alexey Bataev >>> >>> On 10/10/2016 07:09 PM, Erich Keane wrote: >>>> erichkeane added a comment. >>>> >>>> Andrey- >>>> It seems that getVariableArrayDecayedType and getCanonicalParamType return >>>> the same type in this case (at least in the reproduction). I could >>>> definitely change the call, though the changes to CGDebugInfo.cpp are >>>> apparently also necessary even in that case. >>>> >>>> Is there additional logic that should happen in CGStmtOpenMP that I'm >>>> missing? >>>> >>>> >>>> Repository: >>>> rL LLVM >>>> >>>> https://reviews.llvm.org/D25373 >>>> >>>> >>>> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
This causes a massive number of openmp test failures (obviously), and I'm not terribly comfortable with how OpenMP works. I can update the tests (and will), but would like it if you could be extra diligent in confirming the correct behavior for me. Failing Tests (6): Clang :: OpenMP/for_reduction_codegen.cpp Clang :: OpenMP/for_reduction_codegen_UDR.cpp Clang :: OpenMP/nvptx_target_codegen.cpp Clang :: OpenMP/target_codegen.cpp Clang :: OpenMP/target_firstprivate_codegen.cpp Clang :: OpenMP/target_map_codegen.cpp -Original Message- From: Alexey Bataev [mailto:a.bat...@hotmail.com] Sent: Monday, October 10, 2016 10:55 AM To: Keane, Erich ; reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; guy.ben...@intel.com Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Add a check to line 299: if (!VarTy->isReferenceType() && !(FD->getType()->isVariablyModifiedType() && ArgLVal.getType()->isPointerType())) Best regards, Alexey Bataev On 10/10/2016 08:19 PM, Keane, Erich wrote: > That does turn ArgType into a PointerType int*. However, line 301 > (GenerateOpenMPCapturedStmtFunction) attempts to cast it to a reference type > a bit later, resulting in a SIGABRT there. Do I need to re-add the reference > there? Is removing the array type REALLY what we wish to do? > > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 10:09 AM > To: Keane, Erich ; > reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; > cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; > guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference > with OpenMP array access > > Aaaahhh, > > Now I see. We have a parameter with the type that is a reference to VLA. > I think in this case we should rework this code to something like this: > > if (ArgType->isVariablyModifiedType()) > ArgType = > getContext().getCanonicalParamType(ArgType.getNonReferenceType()); > > > Try this solution. > > Best regards, > Alexey Bataev > > On 10/10/2016 07:51 PM, Keane, Erich wrote: >> I did. I changed line 236 to "ArgType = >> getContext().getCanonicalParamType(ArgType);" (as I believe you were >> suggesting), and the output was identical when doing dump on ArgType: >> >> This: >> >> LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >> `-VariableArrayType 0xa4515e0 'int [*]' variably_modified * >> |-BuiltinType 0xa3f4090 'int' >> `-<<>> >> Vs: >> LValueReferenceType 0xa451690 'int (&)[*]' variably_modified >> `-VariableArrayType 0xa451650 'int [*]' variably_modified * >> |-BuiltinType 0xa3f4090 'int' >> `-<<>> >> >> -----Original Message- >> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >> Sent: Monday, October 10, 2016 9:47 AM >> To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich >> ; cfe-commits@lists.llvm.org; dblai...@gmail.com; >> david.majne...@gmail.com; guy.ben...@intel.com >> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference >> with OpenMP array access >> >> Hmm, >> >> I thought it must return a pointer to the element type rather than [*] kind >> type. Did you checked it? >> >> Best regards, >> Alexey Bataev >> >> On 10/10/2016 07:09 PM, Erich Keane wrote: >>> erichkeane added a comment. >>> >>> Andrey- >>> It seems that getVariableArrayDecayedType and getCanonicalParamType return >>> the same type in this case (at least in the reproduction). I could >>> definitely change the call, though the changes to CGDebugInfo.cpp are >>> apparently also necessary even in that case. >>> >>> Is there additional logic that should happen in CGStmtOpenMP that I'm >>> missing? >>> >>> >>> Repository: >>> rL LLVM >>> >>> https://reviews.llvm.org/D25373 >>> >>> >>> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Add a check to line 299: if (!VarTy->isReferenceType() && !(FD->getType()->isVariablyModifiedType() && ArgLVal.getType()->isPointerType())) Best regards, Alexey Bataev On 10/10/2016 08:19 PM, Keane, Erich wrote: > That does turn ArgType into a PointerType int*. However, line 301 > (GenerateOpenMPCapturedStmtFunction) attempts to cast it to a reference type > a bit later, resulting in a SIGABRT there. Do I need to re-add the reference > there? Is removing the array type REALLY what we wish to do? > > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 10:09 AM > To: Keane, Erich ; > reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; > cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; > guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference > with OpenMP array access > > Aaaahhh, > > Now I see. We have a parameter with the type that is a reference to VLA. > I think in this case we should rework this code to something like this: > > if (ArgType->isVariablyModifiedType()) > ArgType = > getContext().getCanonicalParamType(ArgType.getNonReferenceType()); > > > Try this solution. > > Best regards, > Alexey Bataev > > On 10/10/2016 07:51 PM, Keane, Erich wrote: >> I did. I changed line 236 to "ArgType = >> getContext().getCanonicalParamType(ArgType);" (as I believe you were >> suggesting), and the output was identical when doing dump on ArgType: >> >> This: >> >> LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >> `-VariableArrayType 0xa4515e0 'int [*]' variably_modified * >> |-BuiltinType 0xa3f4090 'int' >> `-<<>> >> Vs: >> LValueReferenceType 0xa451690 'int (&)[*]' variably_modified >> `-VariableArrayType 0xa451650 'int [*]' variably_modified * >> |-BuiltinType 0xa3f4090 'int' >> `-<<>> >> >> -Original Message- >> From: Alexey Bataev [mailto:a.bat...@hotmail.com] >> Sent: Monday, October 10, 2016 9:47 AM >> To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich >> ; cfe-commits@lists.llvm.org; dblai...@gmail.com; >> david.majne...@gmail.com; guy.ben...@intel.com >> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference >> with OpenMP array access >> >> Hmm, >> >> I thought it must return a pointer to the element type rather than [*] kind >> type. Did you checked it? >> >> Best regards, >> Alexey Bataev >> >> On 10/10/2016 07:09 PM, Erich Keane wrote: >>> erichkeane added a comment. >>> >>> Andrey- >>> It seems that getVariableArrayDecayedType and getCanonicalParamType return >>> the same type in this case (at least in the reproduction). I could >>> definitely change the call, though the changes to CGDebugInfo.cpp are >>> apparently also necessary even in that case. >>> >>> Is there additional logic that should happen in CGStmtOpenMP that I'm >>> missing? >>> >>> >>> Repository: >>> rL LLVM >>> >>> https://reviews.llvm.org/D25373 >>> >>> >>> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
That does turn ArgType into a PointerType int*. However, line 301 (GenerateOpenMPCapturedStmtFunction) attempts to cast it to a reference type a bit later, resulting in a SIGABRT there. Do I need to re-add the reference there? Is removing the array type REALLY what we wish to do? -Original Message- From: Alexey Bataev [mailto:a.bat...@hotmail.com] Sent: Monday, October 10, 2016 10:09 AM To: Keane, Erich ; reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; guy.ben...@intel.com Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Aaaahhh, Now I see. We have a parameter with the type that is a reference to VLA. I think in this case we should rework this code to something like this: if (ArgType->isVariablyModifiedType()) ArgType = getContext().getCanonicalParamType(ArgType.getNonReferenceType()); Try this solution. Best regards, Alexey Bataev On 10/10/2016 07:51 PM, Keane, Erich wrote: > I did. I changed line 236 to "ArgType = > getContext().getCanonicalParamType(ArgType);" (as I believe you were > suggesting), and the output was identical when doing dump on ArgType: > > This: > >LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >`-VariableArrayType 0xa4515e0 'int [*]' variably_modified * > |-BuiltinType 0xa3f4090 'int' > `-<<>> > Vs: >LValueReferenceType 0xa451690 'int (&)[*]' variably_modified >`-VariableArrayType 0xa451650 'int [*]' variably_modified * > |-BuiltinType 0xa3f4090 'int' > `-<<>> > > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 9:47 AM > To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich > ; cfe-commits@lists.llvm.org; dblai...@gmail.com; > david.majne...@gmail.com; guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference > with OpenMP array access > > Hmm, > > I thought it must return a pointer to the element type rather than [*] kind > type. Did you checked it? > > Best regards, > Alexey Bataev > > On 10/10/2016 07:09 PM, Erich Keane wrote: >> erichkeane added a comment. >> >> Andrey- >> It seems that getVariableArrayDecayedType and getCanonicalParamType return >> the same type in this case (at least in the reproduction). I could >> definitely change the call, though the changes to CGDebugInfo.cpp are >> apparently also necessary even in that case. >> >> Is there additional logic that should happen in CGStmtOpenMP that I'm >> missing? >> >> >> Repository: >> rL LLVM >> >> https://reviews.llvm.org/D25373 >> >> >> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Aaaahhh, Now I see. We have a parameter with the type that is a reference to VLA. I think in this case we should rework this code to something like this: if (ArgType->isVariablyModifiedType()) ArgType = getContext().getCanonicalParamType(ArgType.getNonReferenceType()); Try this solution. Best regards, Alexey Bataev On 10/10/2016 07:51 PM, Keane, Erich wrote: > I did. I changed line 236 to "ArgType = > getContext().getCanonicalParamType(ArgType);" (as I believe you were > suggesting), and the output was identical when doing dump on ArgType: > > This: > >LValueReferenceType 0xa451620 'int (&)[*]' variably_modified >`-VariableArrayType 0xa4515e0 'int [*]' variably_modified * > |-BuiltinType 0xa3f4090 'int' > `-<<>> > Vs: >LValueReferenceType 0xa451690 'int (&)[*]' variably_modified >`-VariableArrayType 0xa451650 'int [*]' variably_modified * > |-BuiltinType 0xa3f4090 'int' > `-<<>> > > -Original Message- > From: Alexey Bataev [mailto:a.bat...@hotmail.com] > Sent: Monday, October 10, 2016 9:47 AM > To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich > ; cfe-commits@lists.llvm.org; dblai...@gmail.com; > david.majne...@gmail.com; guy.ben...@intel.com > Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference > with OpenMP array access > > Hmm, > > I thought it must return a pointer to the element type rather than [*] kind > type. Did you checked it? > > Best regards, > Alexey Bataev > > On 10/10/2016 07:09 PM, Erich Keane wrote: >> erichkeane added a comment. >> >> Andrey- >> It seems that getVariableArrayDecayedType and getCanonicalParamType return >> the same type in this case (at least in the reproduction). I could >> definitely change the call, though the changes to CGDebugInfo.cpp are >> apparently also necessary even in that case. >> >> Is there additional logic that should happen in CGStmtOpenMP that I'm >> missing? >> >> >> Repository: >> rL LLVM >> >> https://reviews.llvm.org/D25373 >> >> >> ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
I did. I changed line 236 to "ArgType = getContext().getCanonicalParamType(ArgType);" (as I believe you were suggesting), and the output was identical when doing dump on ArgType: This: LValueReferenceType 0xa451620 'int (&)[*]' variably_modified `-VariableArrayType 0xa4515e0 'int [*]' variably_modified * |-BuiltinType 0xa3f4090 'int' `-<<>> Vs: LValueReferenceType 0xa451690 'int (&)[*]' variably_modified `-VariableArrayType 0xa451650 'int [*]' variably_modified * |-BuiltinType 0xa3f4090 'int' `-<<>> -Original Message- From: Alexey Bataev [mailto:a.bat...@hotmail.com] Sent: Monday, October 10, 2016 9:47 AM To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich ; cfe-commits@lists.llvm.org; dblai...@gmail.com; david.majne...@gmail.com; guy.ben...@intel.com Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Hmm, I thought it must return a pointer to the element type rather than [*] kind type. Did you checked it? Best regards, Alexey Bataev On 10/10/2016 07:09 PM, Erich Keane wrote: > erichkeane added a comment. > > Andrey- > It seems that getVariableArrayDecayedType and getCanonicalParamType return > the same type in this case (at least in the reproduction). I could > definitely change the call, though the changes to CGDebugInfo.cpp are > apparently also necessary even in that case. > > Is there additional logic that should happen in CGStmtOpenMP that I'm missing? > > > Repository: >rL LLVM > > https://reviews.llvm.org/D25373 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Hmm, I thought it must return a pointer to the element type rather than [*] kind type. Did you checked it? Best regards, Alexey Bataev On 10/10/2016 07:09 PM, Erich Keane wrote: > erichkeane added a comment. > > Andrey- > It seems that getVariableArrayDecayedType and getCanonicalParamType return > the same type in this case (at least in the reproduction). I could > definitely change the call, though the changes to CGDebugInfo.cpp are > apparently also necessary even in that case. > > Is there additional logic that should happen in CGStmtOpenMP that I'm missing? > > > Repository: >rL LLVM > > https://reviews.llvm.org/D25373 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
erichkeane added a comment. Andrey- It seems that getVariableArrayDecayedType and getCanonicalParamType return the same type in this case (at least in the reproduction). I could definitely change the call, though the changes to CGDebugInfo.cpp are apparently also necessary even in that case. Is there additional logic that should happen in CGStmtOpenMP that I'm missing? Repository: rL LLVM https://reviews.llvm.org/D25373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
ABataev added a comment. I think the fix is not quite correct. I believe it's better to replace a call of getVariableArrayDecayedType() in CGStmtOpenMP.cpp by a call to getCanonicalParamType(). Try to replace it and check the result. Repository: rL LLVM https://reviews.llvm.org/D25373 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Ok, I dug into this deeper. ASTContext.cpp:2811 (getVariableArrayDecayedType) intentionaly sets size to nullptr in this case for the purpose of turning it into a [*] type. OpenMP.cpp:236 (CodeGenFunction::GenerateOpenMPCapturedStmtFunction) calls this to replace variably modified type with this one. It definitely looks like this is on purpose as far as I can tell. From: Keane, Erich Sent: Friday, October 7, 2016 11:56 AM To: 'David Blaikie' ; reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; cfe-commits@lists.llvm.org; david.majne...@gmail.com; 'Alexey Bataev' Cc: junb...@codeaurora.org Subject: RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Added Alexey to the list, he’s the OMP Maintainer, so hopefully he knows better ☺ From: David Blaikie [mailto:dblai...@gmail.com] Sent: Friday, October 7, 2016 11:51 AM To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org<mailto:reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org>; Keane, Erich mailto:erich.ke...@intel.com>>; cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>; david.majne...@gmail.com<mailto:david.majne...@gmail.com>; guy.ben...@intel.com<mailto:guy.ben...@intel.com> Cc: junb...@codeaurora.org<mailto:junb...@codeaurora.org> Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Could you explain how/why there's a null size expr? I would've thought it must have /some/ size for code generation purposes... On Fri, Oct 7, 2016 at 11:33 AM Erich Keane mailto:erich.ke...@intel.com>> wrote: erichkeane created this revision. erichkeane added reviewers: cfe-commits, dblaikie, majnemer, gbenyei. erichkeane set the repository for this revision to rL LLVM. OpenMP creates a variable array type with a a null size-expr. The Debug generation failed to properly consider this case. This patch adds a null check to prevent a null dereference seg-fault in this case, plus adds a test. Repository: rL LLVM https://reviews.llvm.org/D25373 Files: lib/CodeGen/CGDebugInfo.cpp test/CodeGenCXX/debug-info-openmp-array.cpp Index: lib/CodeGen/CGDebugInfo.cpp === --- lib/CodeGen/CGDebugInfo.cpp +++ lib/CodeGen/CGDebugInfo.cpp @@ -2181,7 +2181,8 @@ Count = CAT->getSize().getZExtValue(); else if (const auto *VAT = dyn_cast(Ty)) { llvm::APSInt V; - if (VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) + if (VAT->getSizeExpr() && + VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) Count = V.getExtValue(); } Index: test/CodeGenCXX/debug-info-openmp-array.cpp === --- test/CodeGenCXX/debug-info-openmp-array.cpp +++ test/CodeGenCXX/debug-info-openmp-array.cpp @@ -0,0 +1,17 @@ +// RUN: %clang -target x86_64-unknown-unknown -fverbose-asm -fopenmp -g -O0 -S -emit-llvm %s -o - | FileCheck %s + + +void f(int m) { + int i; + int cen[m]; +#pragma omp parallel for + for (i = 0; i < m; ++i) { +cen[i] = i; + } +} + +// CHECK: !DICompositeType(tag: DW_TAG_array_type, +// CHECK-NOT: size: +// CHECK-SAME: align: 32 +// CHECK-SAME: elements: [[ELEM_TYPE:![0-9]+]] +// CHECK: !DISubrange(count: -1) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Added Alexey to the list, he’s the OMP Maintainer, so hopefully he knows better ☺ From: David Blaikie [mailto:dblai...@gmail.com] Sent: Friday, October 7, 2016 11:51 AM To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich ; cfe-commits@lists.llvm.org; david.majne...@gmail.com; guy.ben...@intel.com Cc: junb...@codeaurora.org Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Could you explain how/why there's a null size expr? I would've thought it must have /some/ size for code generation purposes... On Fri, Oct 7, 2016 at 11:33 AM Erich Keane mailto:erich.ke...@intel.com>> wrote: erichkeane created this revision. erichkeane added reviewers: cfe-commits, dblaikie, majnemer, gbenyei. erichkeane set the repository for this revision to rL LLVM. OpenMP creates a variable array type with a a null size-expr. The Debug generation failed to properly consider this case. This patch adds a null check to prevent a null dereference seg-fault in this case, plus adds a test. Repository: rL LLVM https://reviews.llvm.org/D25373 Files: lib/CodeGen/CGDebugInfo.cpp test/CodeGenCXX/debug-info-openmp-array.cpp Index: lib/CodeGen/CGDebugInfo.cpp === --- lib/CodeGen/CGDebugInfo.cpp +++ lib/CodeGen/CGDebugInfo.cpp @@ -2181,7 +2181,8 @@ Count = CAT->getSize().getZExtValue(); else if (const auto *VAT = dyn_cast(Ty)) { llvm::APSInt V; - if (VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) + if (VAT->getSizeExpr() && + VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) Count = V.getExtValue(); } Index: test/CodeGenCXX/debug-info-openmp-array.cpp === --- test/CodeGenCXX/debug-info-openmp-array.cpp +++ test/CodeGenCXX/debug-info-openmp-array.cpp @@ -0,0 +1,17 @@ +// RUN: %clang -target x86_64-unknown-unknown -fverbose-asm -fopenmp -g -O0 -S -emit-llvm %s -o - | FileCheck %s + + +void f(int m) { + int i; + int cen[m]; +#pragma omp parallel for + for (i = 0; i < m; ++i) { +cen[i] = i; + } +} + +// CHECK: !DICompositeType(tag: DW_TAG_array_type, +// CHECK-NOT: size: +// CHECK-SAME: align: 32 +// CHECK-SAME: elements: [[ELEM_TYPE:![0-9]+]] +// CHECK: !DISubrange(count: -1) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
I cannot personally right now, though I will dig into OpenMP implementation to see how intentional that is. The size is seemingly determined by the OMP code generation as far as I can tell. From: David Blaikie [mailto:dblai...@gmail.com] Sent: Friday, October 7, 2016 11:51 AM To: reviews+d25373+public+d8ec2a4bb41b1...@reviews.llvm.org; Keane, Erich ; cfe-commits@lists.llvm.org; david.majne...@gmail.com; guy.ben...@intel.com Cc: junb...@codeaurora.org Subject: Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access Could you explain how/why there's a null size expr? I would've thought it must have /some/ size for code generation purposes... On Fri, Oct 7, 2016 at 11:33 AM Erich Keane mailto:erich.ke...@intel.com>> wrote: erichkeane created this revision. erichkeane added reviewers: cfe-commits, dblaikie, majnemer, gbenyei. erichkeane set the repository for this revision to rL LLVM. OpenMP creates a variable array type with a a null size-expr. The Debug generation failed to properly consider this case. This patch adds a null check to prevent a null dereference seg-fault in this case, plus adds a test. Repository: rL LLVM https://reviews.llvm.org/D25373 Files: lib/CodeGen/CGDebugInfo.cpp test/CodeGenCXX/debug-info-openmp-array.cpp Index: lib/CodeGen/CGDebugInfo.cpp === --- lib/CodeGen/CGDebugInfo.cpp +++ lib/CodeGen/CGDebugInfo.cpp @@ -2181,7 +2181,8 @@ Count = CAT->getSize().getZExtValue(); else if (const auto *VAT = dyn_cast(Ty)) { llvm::APSInt V; - if (VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) + if (VAT->getSizeExpr() && + VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) Count = V.getExtValue(); } Index: test/CodeGenCXX/debug-info-openmp-array.cpp === --- test/CodeGenCXX/debug-info-openmp-array.cpp +++ test/CodeGenCXX/debug-info-openmp-array.cpp @@ -0,0 +1,17 @@ +// RUN: %clang -target x86_64-unknown-unknown -fverbose-asm -fopenmp -g -O0 -S -emit-llvm %s -o - | FileCheck %s + + +void f(int m) { + int i; + int cen[m]; +#pragma omp parallel for + for (i = 0; i < m; ++i) { +cen[i] = i; + } +} + +// CHECK: !DICompositeType(tag: DW_TAG_array_type, +// CHECK-NOT: size: +// CHECK-SAME: align: 32 +// CHECK-SAME: elements: [[ELEM_TYPE:![0-9]+]] +// CHECK: !DISubrange(count: -1) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
Could you explain how/why there's a null size expr? I would've thought it must have /some/ size for code generation purposes... On Fri, Oct 7, 2016 at 11:33 AM Erich Keane wrote: > erichkeane created this revision. > erichkeane added reviewers: cfe-commits, dblaikie, majnemer, gbenyei. > erichkeane set the repository for this revision to rL LLVM. > > OpenMP creates a variable array type with a a null size-expr. The Debug > generation failed to properly consider this case. This patch adds a null > check to prevent a null dereference seg-fault in this case, plus adds a > test. > > > Repository: > rL LLVM > > https://reviews.llvm.org/D25373 > > Files: > lib/CodeGen/CGDebugInfo.cpp > test/CodeGenCXX/debug-info-openmp-array.cpp > > > Index: lib/CodeGen/CGDebugInfo.cpp > === > --- lib/CodeGen/CGDebugInfo.cpp > +++ lib/CodeGen/CGDebugInfo.cpp > @@ -2181,7 +2181,8 @@ >Count = CAT->getSize().getZExtValue(); > else if (const auto *VAT = dyn_cast(Ty)) { >llvm::APSInt V; > - if (VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) > + if (VAT->getSizeExpr() && > + VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) > Count = V.getExtValue(); > } > > Index: test/CodeGenCXX/debug-info-openmp-array.cpp > === > --- test/CodeGenCXX/debug-info-openmp-array.cpp > +++ test/CodeGenCXX/debug-info-openmp-array.cpp > @@ -0,0 +1,17 @@ > +// RUN: %clang -target x86_64-unknown-unknown -fverbose-asm -fopenmp -g > -O0 -S -emit-llvm %s -o - | FileCheck %s > + > + > +void f(int m) { > + int i; > + int cen[m]; > +#pragma omp parallel for > + for (i = 0; i < m; ++i) { > +cen[i] = i; > + } > +} > + > +// CHECK: !DICompositeType(tag: DW_TAG_array_type, > +// CHECK-NOT: size: > +// CHECK-SAME: align: 32 > +// CHECK-SAME: elements: > [[ELEM_TYPE:![0-9]+]] > +// CHECK: !DISubrange(count: -1) > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access
erichkeane created this revision. erichkeane added reviewers: cfe-commits, dblaikie, majnemer, gbenyei. erichkeane set the repository for this revision to rL LLVM. OpenMP creates a variable array type with a a null size-expr. The Debug generation failed to properly consider this case. This patch adds a null check to prevent a null dereference seg-fault in this case, plus adds a test. Repository: rL LLVM https://reviews.llvm.org/D25373 Files: lib/CodeGen/CGDebugInfo.cpp test/CodeGenCXX/debug-info-openmp-array.cpp Index: lib/CodeGen/CGDebugInfo.cpp === --- lib/CodeGen/CGDebugInfo.cpp +++ lib/CodeGen/CGDebugInfo.cpp @@ -2181,7 +2181,8 @@ Count = CAT->getSize().getZExtValue(); else if (const auto *VAT = dyn_cast(Ty)) { llvm::APSInt V; - if (VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) + if (VAT->getSizeExpr() && + VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) Count = V.getExtValue(); } Index: test/CodeGenCXX/debug-info-openmp-array.cpp === --- test/CodeGenCXX/debug-info-openmp-array.cpp +++ test/CodeGenCXX/debug-info-openmp-array.cpp @@ -0,0 +1,17 @@ +// RUN: %clang -target x86_64-unknown-unknown -fverbose-asm -fopenmp -g -O0 -S -emit-llvm %s -o - | FileCheck %s + + +void f(int m) { + int i; + int cen[m]; +#pragma omp parallel for + for (i = 0; i < m; ++i) { +cen[i] = i; + } +} + +// CHECK: !DICompositeType(tag: DW_TAG_array_type, +// CHECK-NOT: size: +// CHECK-SAME: align: 32 +// CHECK-SAME: elements: [[ELEM_TYPE:![0-9]+]] +// CHECK: !DISubrange(count: -1) Index: lib/CodeGen/CGDebugInfo.cpp === --- lib/CodeGen/CGDebugInfo.cpp +++ lib/CodeGen/CGDebugInfo.cpp @@ -2181,7 +2181,8 @@ Count = CAT->getSize().getZExtValue(); else if (const auto *VAT = dyn_cast(Ty)) { llvm::APSInt V; - if (VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) + if (VAT->getSizeExpr() && + VAT->getSizeExpr()->EvaluateAsInt(V, CGM.getContext())) Count = V.getExtValue(); } Index: test/CodeGenCXX/debug-info-openmp-array.cpp === --- test/CodeGenCXX/debug-info-openmp-array.cpp +++ test/CodeGenCXX/debug-info-openmp-array.cpp @@ -0,0 +1,17 @@ +// RUN: %clang -target x86_64-unknown-unknown -fverbose-asm -fopenmp -g -O0 -S -emit-llvm %s -o - | FileCheck %s + + +void f(int m) { + int i; + int cen[m]; +#pragma omp parallel for + for (i = 0; i < m; ++i) { +cen[i] = i; + } +} + +// CHECK: !DICompositeType(tag: DW_TAG_array_type, +// CHECK-NOT: size: +// CHECK-SAME: align: 32 +// CHECK-SAME: elements: [[ELEM_TYPE:![0-9]+]] +// CHECK: !DISubrange(count: -1) ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits