On 3/26/20 8:27 AM, Thomas Schwinge wrote:
Hi!
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. (And it's patches to the "other code" that were
diagnosing the bogus error I saw.)
-Sandra
