Hi Richard,
bootstrap finally passed and the fix is now merged as
gcc-15-1971-gb9513c6746b
Thanks for your patience.
- Andre
On Thu, 11 Jul 2024 14:01:02 +0200
Richard Biener <richard.guent...@gmail.com> wrote:
> On Thu, Jul 11, 2024 at 11:24 AM Andre Vehreschild <ve...@gmx.de>
> wrote:
> >
> > Hi Richard,
> >
> > would that be sufficient? Bootstrap is still running for me...
>
> Yes.
>
> Richard.
>
> > From c30c2cf829a094ba5e4c2c31333bed6e8c0d32af Mon Sep 17 00:00:00
> > 2001 From: Andre Vehreschild <ve...@gcc.gnu.org>
> > Date: Thu, 11 Jul 2024 11:21:04 +0200
> > Subject: [PATCH] [Fortran] Fix bootstrap broken by
> > gcc-15-1965-ge4f2f46e015
> >
> > gcc/fortran/ChangeLog:
> >
> > * trans-array.cc (gfc_conv_array_parameter): Init variable
> > to NULL_TREE to fix bootstrap.
> > ---
> > gcc/fortran/trans-array.cc | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/gcc/fortran/trans-array.cc b/gcc/fortran/trans-array.cc
> > index 0fffa07495c..5558ab69969 100644
> > --- a/gcc/fortran/trans-array.cc
> > +++ b/gcc/fortran/trans-array.cc
> > @@ -8750,7 +8750,7 @@ gfc_conv_array_parameter (gfc_se *se,
> > gfc_expr *expr, bool g77, tree stmt;
> > tree parent = DECL_CONTEXT (current_function_decl);
> > tree ctree;
> > - tree pack_attr;
> > + tree pack_attr = NULL_TREE; /* Set when packing class arrays. */
> > bool full_array_var;
> > bool this_array_result;
> > bool contiguous;
> > --
> > 2.45.2
> >
> > Sorry for the breakage.
> >
> > Regards,
> > Andre
> >
> > On Thu, 11 Jul 2024 11:06:47 +0200
> > Richard Biener <richard.guent...@gmail.com> wrote:
> >
> > > On Thu, Jul 11, 2024 at 11:04 AM Andre Vehreschild <ve...@gmx.de>
> > > wrote:
> > > >
> > > > Hi Richard,
> > > >
> > > > I am sorry to hear that. Shall I revert?
> > >
> > > I would suggest to instead fix by initializing the variable with
> > > NULL (and a comment).
> > >
> > > > - Andre
> > > > On Thu, 11 Jul 2024 10:57:48 +0200
> > > > Richard Biener <richard.guent...@gmail.com> wrote:
> > > >
> > > > > On Thu, Jul 11, 2024 at 10:54 AM Richard Biener
> > > > > <richard.guent...@gmail.com> wrote:
> > > > > >
> > > > > > On Thu, Jul 11, 2024 at 10:04 AM Andre Vehreschild
> > > > > > <ve...@gmx.de> wrote:
> > > > > > >
> > > > > > > Hi Harald,
> > > > > > >
> > > > > > > thank you very much for ok'ing this large patch. Merged as
> > > > > > > gcc-15-1965-ge4f2f46e015
> > > > > > >
> > > > > > > Looking forward to get (no) bug reports ;-)
> > > > > >
> > > > > > This seems to break bootstrap with
> > > > > >
> > > > > > ../../gcc/gcc/fortran/trans-array.cc: In function ‘void
> > > > > > gfc_conv_array_paramete (gfc_se*, gfc_expr*, bool, const
> > > > > > gfc_symbol*, const char*, tree_node**, tree_node**,
> > > > > > tree_node**)’: ../../gcc/gcc/fortran/trans-array.cc:9135:41:
> > > > > > error: ‘pack_attr’ may be used uninitialized
> > > > > > [-Werror=maybe-uninitialized] 9135 | tmp =
> > > > > > build_call_expr_loc (input_location, |
> > > > > > ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ 9136 |
> > > > > > gfor_fndecl_in_unpack_class, 4, tmp,
> > > > > > |
> > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > 9137 | packedptr,
> > > > > > | ~~~~~~~~~~
> > > > > > 9138 |
> > > > > > size_in_bytes (TREE_TYPE (ctree)),
> > > > > > |
> > > > > > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > 9139 | pack_attr);
> > > > > > | ~~~~~~~~~~
> > > > > > ../../gcc/gcc/fortran/trans-array.cc:8665:8: note:
> > > > > > ‘pack_attr’ was declared here 8665 | tree pack_attr;
> > > > > > | ^~~~~~~~~
> > > > > > cc1plus: all warnings being treated as errors
> > > > > > make[3]: *** [Makefile:1198: fortran/trans-array.o] Error 1
> > > > > >
> > > > >
> > > > > It seems to be a false positive but GCCs little mind is too
> > > > > weak to prove that (yes, we error on the side of emitting a
> > > > > diagnostic if we can't prove it's initialized)
> > > > >
> > > > > Richard.
> > > > >
> > > > > >
> > > > > > > Thanks again,
> > > > > > >
> > > > > > > Andre
> > > > > > >
> > > > > > > On Wed, 10 Jul 2024 20:52:37 +0200
> > > > > > > Harald Anlauf <anl...@gmx.de> wrote:
> > > > > > >
> > > > > > > > Hi Andre,
> > > > > > > >
> > > > > > > > Am 10.07.24 um 10:45 schrieb Andre Vehreschild:
> > > > > > > > > Hi Harald,
> > > > > > > > >
> > > > > > > > > thanks for the review. I totally agree, that this
> > > > > > > > > patch has gotten bigger than I expected (and wanted).
> > > > > > > > > But things are as they are.
> > > > > > > > >
> > > > > > > > > About the coding style: I have worked in so many
> > > > > > > > > projects, that I consider a consistent coding style
> > > > > > > > > luxury. I esp. do not have my own one anymore. The
> > > > > > > > > formating you are seeing in my patches is the result
> > > > > > > > > of clang-format with the provided parameter file in
> > > > > > > > > contrib/clang-format. I was happy to have a tool to
> > > > > > > > > do the formatting, that I could integrate into my
> > > > > > > > > IDE, because previously it was hard to mimic the GNU
> > > > > > > > > style. I try to get to the GNU style as good as
> > > > > > > > > possible, where I consider clang-format doing garbage.
> > > > > > > > >
> > > > > > > > > I see that clang-format has a "very specific opinion"
> > > > > > > > > on how to format the lines you mentioned, but it will
> > > > > > > > > "correct" them any time I change them and touch them
> > > > > > > > > later. I now have forbidden clang-format to touch the
> > > > > > > > > code lines, but this means to add formatter specific
> > > > > > > > > comments. Is this ok?
> > > > > > > >
> > > > > > > > yes, this is much better now! Thanks.
> > > > > > > >
> > > > > > > > (I entirely rely on Emacs' formatting when working with
> > > > > > > > C. Sometimes the indentation at first may appear
> > > > > > > > unexpected, but in most of these cases I find that it
> > > > > > > > helps to just use explicit parentheses to convince
> > > > > > > > Emacs. This is documented.)
> > > > > > > > > About the assumed size arrays, that was a small change
> > > > > > > > > and is added now.
> > > > > > > >
> > > > > > > > Great!
> > > > > > > >
> > > > > > > > > Note, the runtime part of the patch
> > > > > > > > > (pr96992_3p1.patch) did not change and is therefore
> > > > > > > > > not updated.
> > > > > > > > >
> > > > > > > > > Regtests ok on x86_64-pc-linux-gnu/Fedora 39. Ok for
> > > > > > > > > mainline?
> > > > > > > >
> > > > > > > > Yes, this is OK now.
> > > > > > > >
> > > > > > > > Thanks for the patch and your patience ;-)
> > > > > > > >
> > > > > > > > Harald
> > > > > > > >
> > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Andre
> > > > > > > > >
> > > > > > > > > On Fri, 5 Jul 2024 22:10:16 +0200
> > > > > > > > > Harald Anlauf <anl...@gmx.de> wrote:
> > > > > > > > >
> > > > > > > > >> Hi Andre,
> > > > > > > > >>
> > > > > > > > >> Am 03.07.24 um 12:58 schrieb Andre Vehreschild:
> > > > > > > > >>> Hi Harald,
> > > > > > > > >>>
> > > > > > > > >>> I am sorry for the long delay, but fixing the
> > > > > > > > >>> negative stride lead from one issue to the next. I
> > > > > > > > >>> finally got a version that does not regress. Please
> > > > > > > > >>> have a look.
> > > > > > > > >>>
> > > > > > > > >>> This patch has two parts:
> > > > > > > > >>> 1. The runtime library part in pr96992_3p1.patch and
> > > > > > > > >>> 2. the compiler changes in pr96992_3p2.patch.
> > > > > > > > >>>
> > > > > > > > >>> In my branch also the two patches from Paul for
> > > > > > > > >>> pr59104 and pr102689 are living, which might lead
> > > > > > > > >>> to small shifts during application of the patches.
> > > > > > > > >>>
> > > > > > > > >>> NOTE, this patch adds internal packing and
> > > > > > > > >>> unpacking of class arrays similar to the regular
> > > > > > > > >>> pack and unpack. I think this is necessary, because
> > > > > > > > >>> the regular un-/pack does not use the vptr's _copy
> > > > > > > > >>> routine for moving data and therefore may produce
> > > > > > > > >>> bugs.
> > > > > > > > >>>
> > > > > > > > >>> The un-/pack_class routines are yet only used for
> > > > > > > > >>> converting a derived type array to a class array.
> > > > > > > > >>> Extending their use when a UN-/PACK() is applied on
> > > > > > > > >>> a class array is still to be done (as part of
> > > > > > > > >>> another PR).
> > > > > > > > >>>
> > > > > > > > >>> Regtests fine on x86_64-pc-linux-gnu/ Fedora 39.
> > > > > > > > >>
> > > > > > > > >> this is a really huge patch to review, and I am not
> > > > > > > > >> sure that I can do this without help from others.
> > > > > > > > >> Paul? Anybody else?
> > > > > > > > >>
> > > > > > > > >> As far as I can tell for now:
> > > > > > > > >>
> > > > > > > > >> - pr96992_3p1.patch (the libgfortran part) looks
> > > > > > > > >> good to me.
> > > > > > > > >>
> > > > > > > > >> - git had some whitespace issues with
> > > > > > > > >> pr96992_3p2.patch as attached, but I could fix that
> > > > > > > > >> locally and do some testing parallel to reading.
> > > > > > > > >>
> > > > > > > > >> A few advance comments on the latter patch:
> > > > > > > > >>
> > > > > > > > >> - my understanding is that the PR at the end of a
> > > > > > > > >> summary line should be like in:
> > > > > > > > >>
> > > > > > > > >> Fortran: Fix rejecting class arrays of different
> > > > > > > > >> ranks as storage association argument [PR96992]
> > > > > > > > >>
> > > > > > > > >> I was told that this helps people explicitly
> > > > > > > > >> scanning for the PR number in that place.
> > > > > > > > >>
> > > > > > > > >> - some rewrites of logical conditions change the
> > > > > > > > >> coding style from what it recommended GNU coding
> > > > > > > > >> style, and I find the more compact way used in some
> > > > > > > > >> places harder to grok (but that may be just me).
> > > > > > > > >> Example:
> > > > > > > > >>
> > > > > > > > >> @@ -8850,20 +8857,24 @@ gfc_conv_array_parameter
> > > > > > > > >> (gfc_se
> > > > > > > > >> * se, gfc_expr
> > > > > > > > >> * expr, bool g77,
> > > > > > > > >> /* There is no need to pack and unpack the
> > > > > > > > >> array, if it is contiguous and not a deferred- or
> > > > > > > > >> assumed-shape array, or if it is simply contiguous.
> > > > > > > > >> */
> > > > > > > > >> - no_pack = ((sym && sym->as
> > > > > > > > >> - && !sym->attr.pointer
> > > > > > > > >> - && sym->as->type != AS_DEFERRED
> > > > > > > > >> - && sym->as->type != AS_ASSUMED_RANK
> > > > > > > > >> - && sym->as->type != AS_ASSUMED_SHAPE)
> > > > > > > > >> - ||
> > > > > > > > >> - (ref && ref->u.ar.as
> > > > > > > > >> - && ref->u.ar.as->type != AS_DEFERRED
> > > > > > > > >> + no_pack = false;
> > > > > > > > >> + gfc_array_spec *as;
> > > > > > > > >> + if (sym)
> > > > > > > > >> + {
> > > > > > > > >> + symbol_attribute *attr
> > > > > > > > >> + = &(IS_CLASS_ARRAY (sym) ? CLASS_DATA (sym)->attr
> > > > > > > > >> : sym->attr);
> > > > > > > > >> + as = IS_CLASS_ARRAY (sym) ? CLASS_DATA
> > > > > > > > >> (sym)->as : sym->as;
> > > > > > > > >> + no_pack
> > > > > > > > >> + = (as && !attr->pointer && as->type != AS_DEFERRED
> > > > > > > > >> + && as->type != AS_ASSUMED_RANK && as->type !=
> > > > > > > > >> AS_ASSUMED_SHAPE);
> > > > > > > > >> + }
> > > > > > > > >> + if (ref && ref->u.ar.as)
> > > > > > > > >> + no_pack = no_pack
> > > > > > > > >> + || (ref->u.ar.as->type != AS_DEFERRED
> > > > > > > > >> && ref->u.ar.as->type !=
> > > > > > > > >> AS_ASSUMED_RANK
> > > > > > > > >> - && ref->u.ar.as->type !=
> > > > > > > > >> AS_ASSUMED_SHAPE)
> > > > > > > > >> - ||
> > > > > > > > >> - gfc_is_simply_contiguous (expr, false,
> > > > > > > > >> true)); -
> > > > > > > > >> - no_pack = contiguous && no_pack;
> > > > > > > > >> + && ref->u.ar.as->type !=
> > > > > > > > >> AS_ASSUMED_SHAPE);
> > > > > > > > >> + no_pack
> > > > > > > > >> + = contiguous && (no_pack ||
> > > > > > > > >> gfc_is_simply_contiguous (expr, false, true));
> > > > > > > > >>
> > > > > > > > >> /* If we have an EXPR_OP or a function
> > > > > > > > >> returning an explicit-shaped or allocatable array,
> > > > > > > > >> an array temporary will be generated which
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> I understand that this may be your personal coding
> > > > > > > > >> style, but you might keep in mind that reviewers
> > > > > > > > >> have to understand the code, too...
> > > > > > > > >>
> > > > > > > > >> I have not fully understood your logic when packing
> > > > > > > > >> is now invoked. We not only need to do it for
> > > > > > > > >> explicit-size arrays, but also for assumed-size.
> > > > > > > > >> This still fails for my slightly extended testcase
> > > > > > > > >> (see attached) where I pass the class array via:
> > > > > > > > >>
> > > > > > > > >> subroutine d4(x,n)
> > > > > > > > >> integer, intent(in) :: n
> > > > > > > > >> ! class (foo), intent(inout) :: x(n) ! OK
> > > > > > > > >> class (foo), intent(inout) :: x(*) ! not OK
> > > > > > > > >> call d3(x,n) ! Simply
> > > > > > > > >> pass assumed-size array end subroutine d4
> > > > > > > > >>
> > > > > > > > >> I am unable to point to the places in your patch
> > > > > > > > >> where you need to handle that in addition.
> > > > > > > > >>
> > > > > > > > >> Otherwise I was unable to see any obvious, major
> > > > > > > > >> problem with the patch, but then I am not fluent
> > > > > > > > >> enough in class handling in the gfortran FE. So if
> > > > > > > > >> e.g. Paul jumps in here within the next 72 hours, it
> > > > > > > > >> would be great.
> > > > > > > > >>
> > > > > > > > >> So here comes the issue with the attached code
> > > > > > > > >> variant. After your patch, this prints as last 4
> > > > > > > > >> relevant lines:
> > > > > > > > >>
> > > > > > > > >> full: -43 44 45
> > > > > > > > >> -46 47 48 -49 50
> > > > > > > > >> d3_1: -43 44 45
> > > > > > > > >> d3_2: 43 -44 -45
> > > > > > > > >> full: 43 -44 -45
> > > > > > > > >> -46 47 48 -49 50
> > > > > > > > >>
> > > > > > > > >> while when switching the declaration of the dummy
> > > > > > > > >> argument of d4:
> > > > > > > > >>
> > > > > > > > >> full: -43 44 45
> > > > > > > > >> -46 47 48 -49 50
> > > > > > > > >> d3_1: -43 -46 -49
> > > > > > > > >> d3_2: 43 46 49
> > > > > > > > >> full: 43 44 45
> > > > > > > > >> 46 47 48 49 50
> > > > > > > > >>
> > > > > > > > >> The latter one is correct, the former one isn't.
> > > > > > > > >>
> > > > > > > > >> Sorry for spoiling the show...
> > > > > > > > >>
> > > > > > > > >> Nevertheless, thanks for your great effort so far!
> > > > > > > > >>
> > > > > > > > >> Harald
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >>> Regards,
> > > > > > > > >>> Andre
> > > > > > > > >>>
> > > > > > > > >>> PS: @Paul I could figure my test failures with -Ox
> > > > > > > > >>> with x e { 2, 3, s } to be caused by initialization
> > > > > > > > >>> order. I.e. a member was set only after it was
> > > > > > > > >>> read.
> > > > > > > > >>
> > > > > > > > >> [remaining part of mail removed]
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > Andre Vehreschild * Email: vehre ad gmx dot de
> > > > > > > >
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Andre Vehreschild * Kreuzherrenstr. 8 * 52062 Aachen
> > > > > > > Tel.: +49 178 3837536 * ve...@gmx.de
> > > >
> > > >
> > > >
> > > > --
> > > > Andre Vehreschild * Email: vehre ad gcc dot gnu dot org
> >
> >
> >
> > --
> > Andre Vehreschild * Email: vehre ad gcc dot gnu dot org
--
Andre Vehreschild * Email: vehre ad gcc dot gnu dot org
