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

[Keane, Erich via cfe-commits]

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.bataev at hotmail.com] 
Sent: Monday, October 10, 2016 1:22 PM
To: Keane, Erich <erich.keane at intel.com>
Cc: 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

I mean, could you send new  .ll files, which you get after these changes?

Best regards,
Alexey Bataev

> 10 окт. 2016 г., в 22:26, Keane, Erich <erich.keane at intel.com> написал(а):
> 
> 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
>>>>> 
>>>>> 
>>>>> 
> 
> <build_results.txt>