[PATCH] D25373: Fix for Bug 30639: CGDebugInfo Null dereference with OpenMP array access

[Keane, Erich via cfe-commits]

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.bataev at hotmail.com] 
Sent: Monday, October 10, 2016 12:18 PM
To: Keane, Erich <erich.keane at intel.com>; reviews+D25373+public+d8ec2a4bb41b17c6 at reviews.llvm.org; cfe-commits at lists.llvm.org; dblaikie at gmail.com; david.majnemer at gmail.com; guy.benyei at 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.bataev at hotmail.com]
> Sent: Monday, October 10, 2016 10:55 AM
> To: Keane, Erich <erich.keane at intel.com>; 
> reviews+D25373+public+d8ec2a4bb41b17c6 at reviews.llvm.org; 
> cfe-commits at lists.llvm.org; dblaikie at gmail.com; 
> david.majnemer at gmail.com; guy.benyei at 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.bataev at hotmail.com]
>> Sent: Monday, October 10, 2016 10:09 AM
>> To: Keane, Erich <erich.keane at intel.com>; 
>> reviews+D25373+public+d8ec2a4bb41b17c6 at reviews.llvm.org; 
>> cfe-commits at lists.llvm.org; dblaikie at gmail.com; 
>> david.majnemer at gmail.com; guy.benyei at 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'
>>>        `-<<<NULL>>>
>>> Vs:
>>>      LValueReferenceType 0xa451690 'int (&)[*]' variably_modified
>>>      `-VariableArrayType 0xa451650 'int [*]' variably_modified  *
>>>        |-BuiltinType 0xa3f4090 'int'
>>>        `-<<<NULL>>>
>>>
>>> -----Original Message-----
>>> From: Alexey Bataev [mailto:a.bataev at hotmail.com]
>>> Sent: Monday, October 10, 2016 9:47 AM
>>> To: reviews+D25373+public+d8ec2a4bb41b17c6 at reviews.llvm.org; Keane, 
>>> Erich <erich.keane at intel.com>; cfe-commits at lists.llvm.org; 
>>> dblaikie at gmail.com; david.majnemer at gmail.com; guy.benyei at 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
>>>>
>>>>
>>>>

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: build_results.txt
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161010/f76362c9/attachment-0001.txt>