Hello Christophe,
I wrote to Thiago Bauermann, who was very helpful on the last occasion that you picked up a bug in one of my patches.
As it happens, Harald Anlauf pointed out that the problem occurs with -m32 on x86_64 systems and so I am able to investigate it on my system. Thus far, I have drawn a total blank with finding a fix. If necessary, I will comment out the offending test.
Best regards
Paul
On Wed, 2 Apr 2025 at 10:12, Christophe Lyon christophe.lyon@linaro.org wrote:
Hi!
On Wed, 19 Mar 2025 at 19:22, Paul Richard Thomas paul.richard.thomas@gmail.com wrote:
Hi Andre,
Thanks for the review - I'll act on the points that you raised.
The Linaro people reported a failure in reduce_1.f90 execution, which I
believe is due to incorrect casting of 'dim' and a wrong specification of its kind. I am waiting to hear back from them as to whether or not I have fixed the failure.
Sorry I notice this message just today, so it's a bit outdated... I've looked at bugzilla, so I've noticed that the are proper bug reports about this now (and I've just checked, the problem is still present on arm).
When you say you are "waiting to hear back from them as to whether or not I have fixed the failure", did you contact us directly? (I'm trying to understand if we missed your message, or how we could improve communication).
Thanks,
Christophe
Cheers
Paul
On Wed, 19 Mar 2025 at 12:39, Andre Vehreschild vehre@gmx.de wrote:
Hi Paul,
I took a look at your patch and think I found some improvements needed.
In
+bool +gfc_check_reduce (gfc_expr *array, gfc_expr *operation, gfc_expr *dim,
gfc_expr *mask, gfc_expr *identity, gfc_expr *ordered)+{
...
- if (formal->sym->attr.allocatable || formal->sym->attr.allocatable
|| formal->sym->attr.pointer || formal->sym->attr.pointer|| formal->sym->attr.optional || formal->sym->attr.optional|| formal->sym->ts.type == BT_CLASS || formal->sym->ts.type ==BT_CLASS)
- {
gfc_error ("Each argument of OPERATION at %L shall be a scalar, ""non-allocatable, non-pointer, non-polymorphic and ""nonoptional", &operation->where);return false;- }
The if is only looking at the first formal argument. The right-hand
sides
of the || miss a ->next-> to look at the second formal argument, right?
May be you also want to extend the tests!?
Without having looked at it, but can't you extract the whole block of
- if (array->ts.type == BT_CHARACTER)
- {
unsigned long actual_size, formal_size1, formal_size2,result_size;
...
return false;}- }
and share it with the checks for co_reduce? I figure way to many DRY
principle
violations are in gfortran. So when we can start this, why not do it?
And a
call to a routine, like check_char_arg_conformance() speaks way better,
then
having to read all that code ;-)
In gfc_resolve_reduce() identity and ordered are marked as UNUSED.
Should these
not a least be resolved?
Please run contrib/check_GNU_style on your patch. It reports several
issues
(haven't look into their validity).
In the Changelog:
(gfc_check_rename): Add prototype for intrinsic with 6arguments.
* gfortran.h: Add prototype for intrinsic with 6 arguments.s/discription/description/
I also encountered that nit with the executable stack when working in OpenCoarrays, but haven't had time (or desire) to look into it. I will
put
myself into CC of the pr Jerry mentioned.
Besides the mentions above, this looks good to me.
Thanks for the patch and
Regards, Andre
On Sun, 16 Mar 2025 17:26:55 +0000 Paul Richard Thomas paul.richard.thomas@gmail.com wrote:
Hi All,
This version of the REDUCE intrinsic patch has evolved somewhat since
the
posting on 2nd March. The most important changes are to the wrapper function and the addition of two testsuite entries.
The wrapper function now effects: subroutine wrapper (a, b, c) type_of_ARRAY, intent(inout) :: a, c type_of_ARRAY, intent(inout), optional :: b if (present (b)) then c = OPERATION (a,b ) else c = a end if end subroutine
The reason for wrapping OPERATION in a subroutine is to allow pointer arithmetic to be used throughout in the library function. The only
thing
that needs to be known about the type and kind of ARRAY is the element size. The second branch in the wrapper allows deep copies to be done
in the
library function, such that derived types with allocatable components
do
not leak memory. This is needed at the final step of the algorithm to
copy
the result from each iteration to the result and then nullify it.
This is undoubtedly a bit heavy going for intrinsic types and so, one
day
soon I will possibly do a bit of M4ery. That said, the present version works for all types of ARRAY and I worry a bit about how much this intrinsic will be used. Thoughts?
A slight niggle is the linker error that comes up if compiled without
any
optimization: /usr/bin/ld: warning: /tmp/cc9cx9Rw.o: requires executable stack
(because
the .note.GNU-stack section is executable) I think that this is unlikely to present a security issue, however,
since
it disappears at -O1, I went through each of the options triggered by
-O1
but couldn't make it go away. Does anybody know why this is?
Regtests OK with FC41/x86_64 - OK for mainline?
Regards
Paul
-- Andre Vehreschild * Email: vehre ad gmx dot de