Hi!
On 2020-03-26T09:09:01-0600, Sandra Loosemore <san...@codesourcery.com> wrote:
> On 3/26/20 8:27 AM, Thomas Schwinge wrote:
>> Note that as this code is shared between OpenACC/OpenMP, this might
>> affect OpenMP, too, as far as I can tell. (Subject updated.) Jakub, can
>> you please have a look, too?
>>
>> On 2020-03-25T23:02:38-0600, Sandra Loosemore <san...@codesourcery.com>
>> wrote:
>>> The attached patch fixes a bug I found in the C++ front end's handling
>>> of OpenACC data clauses. The problem here is that the C++ front end
>>> wraps the array bounds expressions in NON_LVALUE_EXPR tree nodes, and
>>> the code here (which appears to have been copied from similar code in
>>> the C front end) was failing to strip those before checking to see if
>>> they were INTEGER_CST nodes, etc.
>>
>> So, I had a quick look. I'm confirming the 'NON_LVALUE_EXPR' (C++ only,
>> not seen for C) difference, and that 'STRIP_NOPS' gets rid of these.
>> However, I also in some code paths see, for example, 'integer_nonzerop'
>> calls, which internally do 'STRIP_ANY_LOCATION_WRAPPER'.
>>
>> I don't know if 'STRIP_NOPS' is the correct "hammer" to use here, I don't
>> know what the usual convention is: explicitly remove (via 'STRIP_NOPS' as
>> you suggested, or something else), or have the enquiry functions do it
>> ('STRIP_ANY_LOCATION_WRAPPER' as 'integer_nonzerop' is doing, for
>> example).
>>
>> Empirical data doesn't mean too much here, of course, I'm not seeing a
>> lot of explicit 'STRIP_*' calls in 'gcc/cp/semantics.c'. ;-)
>
> I saw that STRIP_NOPS seem to be the preferred way to do things in e.g.
> fold-const.c. I don't know if there's a reason to use some less general
> form of STRIP_* here?
>
>>> Sadly, I have no test case for this because it was only triggering an
>>> error in conjunction with some other OpenACC patches that are not yet on
>>> trunk
>>
>> So maybe the problem is actually that these "other OpenACC patches" are
>> not sufficiently sanitizing the input data they're doing checks on?
>
> No. In the current code on trunk, both places that are being patched
> continue with checks like
>
> TREE_CODE (low_bound) == INTEGER_CST
>
> etc. So when they're wrapped in NON_LVALUE_EXPRs, it's basically
> failing to detect constants in array dimension specifiers and falling
> through to other code.
Eh, indeed... ;-\ (So we should be able to deduce some misbehavior from
that, to construct a test case. I'll have a look.)
>>> and the tree dumps don't show anything useful.
>>
>> Well, the 'original' tree dumps do show (C++) vs. don't show (C) the
>> 'NON_LVALUE_EXPR's.
While true, that of course doesn't tell us anything what the OMP array
section handling is doing with these.
I guess I was half-asleep when I wrote my email... ;-/
So. I'm not objecting to handling 'NON_LVALUE_EXPR's locally here via
any kind of 'STRIP_*', but it somehow doesn't seem the general solution.
Another option seems to be to teach 'fold_simple' to handle
'NON_LVALUE_EXPR's, so that the existing code:
/* We need to reduce to real constant-values for checks below. */
if (length)
length = fold_simple (length);
if (low_bound)
low_bound = fold_simple (low_bound);
if (low_bound
&& TREE_CODE (low_bound) == INTEGER_CST
&& [...])
low_bound = fold_convert (sizetype, low_bound);
if (length
&& TREE_CODE (length) == INTEGER_CST
&& [...])
length = fold_convert (sizetype, length);
... would then just work. But: I don't know if 'fold_simple' (and
others?) should handle 'NON_LVALUE_EXPR's, or if there's a reason why it
doesn't. (Have not yet tried to figure that out.) What I can tell is
that the attached patch does solve the issue in the C++ OMP array section
handling, and survives a powerpc64le-unknown-linux-gnu
'--enable-checking=yes' (will now re-run with 'fold' checking) bootstrap,
with no testsuite regressions.
Hmm.
Grüße
Thomas
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander
Walter
>From 611fbe24b7e459829c0a304a58963d4987c8de0a Mon Sep 17 00:00:00 2001
From: Thomas Schwinge <tho...@codesourcery.com>
Date: Thu, 26 Mar 2020 21:22:54 +0100
Subject: [PATCH] Fold 'NON_LVALUE_EXPR' some more
---
gcc/cp/constexpr.c | 1 +
gcc/fold-const.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/gcc/cp/constexpr.c b/gcc/cp/constexpr.c
index 192face9a3a..f31d61c1460 100644
--- a/gcc/cp/constexpr.c
+++ b/gcc/cp/constexpr.c
@@ -6650,6 +6650,7 @@ fold_simple_1 (tree t)
case BIT_NOT_EXPR:
case TRUTH_NOT_EXPR:
case NOP_EXPR:
+ case NON_LVALUE_EXPR:
case VIEW_CONVERT_EXPR:
case CONVERT_EXPR:
case FLOAT_EXPR:
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index 71a1d3eb735..b6bc5080ff3 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -1739,6 +1739,7 @@ const_unop (enum tree_code code, tree type, tree arg0)
switch (code)
{
CASE_CONVERT:
+ case NON_LVALUE_EXPR:
case FLOAT_EXPR:
case FIX_TRUNC_EXPR:
case FIXED_CONVERT_EXPR:
--
2.17.1
