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

[Keane, Erich via cfe-commits]

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