[PATCH 0/3] GNAT test suite fixes for build sysroot

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/3] GNAT test suite fixes for build sysroot

Maciej Rozycki
Hi,

 In the course of setting up GCC regression testing for the RISC-V target
I have discovered that the GNAT test suite does not correctly respond to
the test environment settings passed from the test harness in my setup and
consequently no test case works correctly.

 In my particular setup `--with-build-sysroot=' has been used to configure
GCC and also additional linker flags are defined in the target board so
that adjusted paths are used for the dynamic loader and shared libraries
to be found in the build sysroot at the test suite run time.  That plays
well with most of the test suites included with GCC, but not the GNAT one.

 I have tracked down the cause to three issues spread across `gnatmake',
the GNAT test driver and the DejaGNU target driver.  These issues are
independent from each other, however all have to be addressed for the test
suite to run, so I have decided to group them into a series despite that
each of them can be applied separately, and the DejaGNU one goes to a
separate project even.

 With them all in place I get these results with the `riscv64-linux-gnu'
target:

                === gnat Summary ===

# of expected passes 2926
# of unexpected failures 5
# of expected failures 23
# of unsupported tests 26

which I think are pretty good, especially compared to the original results
without these changes:

                === gnat Summary ===

# of expected passes 1580
# of unexpected failures 133
# of unexpected successes 23
# of unresolved testcases 875
# of unsupported tests 26

 See individual change descriptions for details.

  Maciej
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/3][GCC] gnatmake: Accept the `--sysroot=' GCC driver option

Maciej Rozycki
According to `gnatmake' documentation:

"Any uppercase or multi-character switch that is not a 'gnatmake' switch
is passed to 'gcc' (e.g., '-O', '-gnato,' etc.)"

however the `--sysroot=' switch is actually rejected:

gnatmake: invalid switch: --sysroot=...

likely because it is one of the very few GCC driver options that have a
leading double dash and therefore we don't have a blanket fall-through
for such switches that would satisfy what our documentation claims.

The option is actually shared between the compiler and the linker, so
pass the switch to both build stages if requested, removing GNAT
testsuite issues like:

gnatmake: invalid switch: --sysroot=.../sysroot
compiler exited with status 1
Executing on host: .../gcc/gnatclean -c -q -n ./abstract_with_anonymous_result   (timeout = 300)
spawn -ignore SIGHUP .../gcc/gnatclean -c -q -n ./abstract_with_anonymous_result
PASS: gnat.dg/abstract_with_anonymous_result.adb (test for excess errors)
UNRESOLVED: gnat.dg/abstract_with_anonymous_result.adb compilation failed to produce executable

in a test environment where `--with-build-sysroot=.../sysroot' has been
used to build a cross-compiler.  Passing to the compilation stage only
would lead to errors like:

.../bin/riscv64-linux-gnu-ld: cannot find crt1.o: No such file or directory
.../bin/riscv64-linux-gnu-ld: cannot find -lc
collect2: error: ld returned 1 exit status
gnatlink: error when calling .../gcc/xgcc
gnatmake: *** link failed.
compiler exited with status 1
Executing on host: .../gcc/gnatclean -c -q -n ./abstract_with_anonymous_result   (timeout = 300)
spawn -ignore SIGHUP .../gcc/gnatclean -c -q -n ./abstract_with_anonymous_result
./abstract_with_anonymous_result.ali
./abstract_with_anonymous_result.o
FAIL: gnat.dg/abstract_with_anonymous_result.adb (test for excess errors)
Excess errors:
.../bin/riscv64-linux-gnu-ld: cannot find crt1.o: No such file or directory
.../bin/riscv64-linux-gnu-ld: cannot find -lc
gnatlink: error when calling .../gcc/xgcc

UNRESOLVED: gnat.dg/abstract_with_anonymous_result.adb compilation failed to produce executable

instead.

        gcc/ada/
        * make.adb (Scan_Make_Arg): Also accept `--sysroot=' for the
        compiler and the linker.
---
Hi,

 I think treating `--sysroot=' like this makes sense, as otherwise it'd
have to be specified twice, once with `-largs' and the second time
optionally with `-cargs'.  It's consistent with how `--param=' is handled.
I'm not sure if this peculiarity should be mentioned in the manual; it
surely is not for the existing `--param=' switch.

 Unfortunately I have exhausted the limit of changes I can make to GCC
without my WDC copyright paperwork sorted with FSF.  OK to apply once that
has been completed?

  Maciej
---
 gcc/ada/make.adb |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

gcc-gnatmake-sysroot.diff
Index: gcc/gcc/ada/make.adb
===================================================================
--- gcc.orig/gcc/ada/make.adb
+++ gcc/gcc/ada/make.adb
@@ -4516,7 +4516,9 @@ package body Make is
                end;
             end if;
 
-         elsif Argv'Length >= 8 and then Argv (1 .. 8) = "--param=" then
+         elsif (Argv'Length >= 8 and then Argv (1 .. 8) = "--param=")
+           or (Argv'Length >= 10 and then Argv (1 .. 10) = "--sysroot=")
+         then
             Add_Switch (Argv, Compiler);
             Add_Switch (Argv, Linker);
 
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/3][GCC] GNAT/testsuite: Pass the `ada' option to target compilation

Maciej Rozycki
In reply to this post by Maciej Rozycki
Pass the `ada' option to DejaGNU's `target_compile' procedure, which by
default calls `default_target_compile', so that it arranges for an Ada
compilation rather the default of C.  We set the compiler to `gnatmake'
manually here, so that part of the logic in `default_target_compile' is
not used, but it affects other settings, such as the use of `adaflags'.

        gcc/testsuite/
        * lib/gnat.exp (gnat_target_compile): Pass the `ada' option to
        `target_compile'.
---
Hi,

 Unfortunately I have exhausted the limit of changes I can make to GCC
without my WDC copyright paperwork sorted with FSF.  OK to apply once that
has been completed?

  Maciej
---
 gcc/testsuite/lib/gnat.exp |    2 ++
 1 file changed, 2 insertions(+)

gcc-test-gnat-options-ada.diff
Index: gcc/gcc/testsuite/lib/gnat.exp
===================================================================
--- gcc.orig/gcc/testsuite/lib/gnat.exp
+++ gcc/gcc/testsuite/lib/gnat.exp
@@ -167,6 +167,8 @@ proc gnat_target_compile { source dest t
  set options [concat "additional_flags=$TOOL_OPTIONS" $options]
     }
 
+    set options [concat "{ada}" $options]
+
     return [target_compile $source $dest $type $options]
 }
 
Reply | Threaded
Open this post in threaded view
|

[PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada

Maciej Rozycki
In reply to this post by Maciej Rozycki
Unrecognized `gnatmake' switches are not implicitly passed on to the
linker, so just pasting board `ldflags' and any other linker flags
verbatim into `add_flags' to use for the invocation line of `gnatmake'
will make them ignored at best.

For example in a GCC test environment that has:

set_board_info ldflags "-Wl,-dynamic-linker,.../sysroot/lib/ld-linux-riscv64-lp64d.so.1 -Wl,-rpath,.../sysroot/lib64/lp64d -Wl,-rpath,.../sysroot/usr/lib64/lp64d"

so that sysroot paths are correctly embedded with the binaries linked
for use with the dynamic loader and shared library dependencies, the
setting will be ignored for the GNAT test suite making all the execution
tests fail, e.g.:

PASS: gnat.dg/abstract_with_anonymous_result.adb (test for excess errors)
spawn qemu-riscv64 ./abstract_with_anonymous_result.exe
/lib/ld-linux-riscv64-lp64d.so.1: No such file or directory
FAIL: gnat.dg/abstract_with_anonymous_result.adb execution test

For `gnatmake' to pass switches on to the linker the `-largs' switch has
to be used, which affects all the switches that follow until a switch is
seen that changes the selection, like `-margs', which resets to the
initial state of the switch interpretation machine.

Wrap linker flags into `-largs'/`-margs' for Ada then, carefully
preserving the place these flags are placed within `add_flags', as
surely someone will have depended on that, correcting test failures like
above:

PASS: gnat.dg/abstract_with_anonymous_result.adb (test for excess errors)
spawn qemu-riscv64 ./abstract_with_anonymous_result.exe
PASS: gnat.dg/abstract_with_anonymous_result.adb execution test

        * lib/target.exp (default_target_compile): Wrap linker flags into
        `-largs'/`-margs' for Ada.

Signed-off-by: Maciej W. Rozycki <[hidden email]>
---
Hi,

 My WDC copyright paperwork has not been sorted with FSF yet, however I
have not contributed to DejaGNU on behalf of WDC so far and I believe this
change falls within the limit of roughly 15 lines to be considered legally
insignificant given that:

"A regular series of repeated changes, such as renaming a symbol, is not
legally significant even if the symbol has to be renamed in many places."

 Please apply then; this does not rely on 2/3 in any way, as we ought to
handle Ada compilations correctly regardless of whether our caller does
the right thing there.

  Maciej
---
 lib/target.exp |   24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

dejagnu-target-ada-ldflags.diff
Index: dejagnu/lib/target.exp
===================================================================
--- dejagnu.orig/lib/target.exp
+++ dejagnu/lib/target.exp
@@ -518,11 +518,12 @@ proc default_target_compile {source dest
     }
 
     if { $type eq "executable" } {
+ set extra_ldflags ""
  if {[board_info $dest exists ldflags]} {
-    append add_flags " [board_info $dest ldflags]"
+    append extra_ldflags " [board_info $dest ldflags]"
  }
  if { $compiler_type eq "c++" } {
-    append add_flags " [g++_link_flags]"
+    append extra_ldflags " [g++_link_flags]"
  }
  if {[isnative]} {
     # This is a lose.
@@ -530,16 +531,23 @@ proc default_target_compile {source dest
     if { $tmp ne "" } {
  if {[regexp ".*solaris2.*" $target_triplet]} {
     # Solaris 2
-    append add_flags " -R$tool_root_dir/libstdc++"
+    append extra_ldflags " -R$tool_root_dir/libstdc++"
  } elseif {[regexp ".*(osf|irix5|linux).*" $target_triplet]} {
     # OSF/1 or IRIX 5
-    append add_flags " -Wl,-rpath,$tool_root_dir/libstdc++"
+    append extra_ldflags " -Wl,-rpath,$tool_root_dir/libstdc++"
  } elseif {[regexp ".*hppa.*" $target_triplet]} {
     # HP-UX
-    append add_flags " -Wl,-a,shared_archive"
+    append extra_ldflags " -Wl,-a,shared_archive"
  }
     }
  }
+ if { $extra_ldflags ne "" } {
+    if { $compiler_type eq "ada" } {
+ append add_flags " -largs $extra_ldflags -margs"
+    } else {
+ append add_flags " $extra_ldflags"
+    }
+ }
     }
 
     if {![info exists ldscript]} {
@@ -561,7 +569,11 @@ proc default_target_compile {source dest
     }
 
     if { $type eq "executable" } {
- append add_flags " $ldflags"
+ if { $compiler_type eq "ada" } {
+    append add_flags " -largs $ldflags -margs"
+ } else {
+    append add_flags " $ldflags"
+ }
  foreach x $libs {
     if {[file exists $x]} {
  append source " $x"
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3][GCC] GNAT/testsuite: Pass the `ada' option to target compilation

Jacob Bachmeyer
In reply to this post by Maciej Rozycki
Maciej W. Rozycki wrote:

> [...]
> ---
>  gcc/testsuite/lib/gnat.exp |    2 ++
>  1 file changed, 2 insertions(+)
>
> gcc-test-gnat-options-ada.diff
> Index: gcc/gcc/testsuite/lib/gnat.exp
> ===================================================================
> --- gcc.orig/gcc/testsuite/lib/gnat.exp
> +++ gcc/gcc/testsuite/lib/gnat.exp
> @@ -167,6 +167,8 @@ proc gnat_target_compile { source dest t
>   set options [concat "additional_flags=$TOOL_OPTIONS" $options]
>      }
>  
> +    set options [concat "{ada}" $options]
> +
>      return [target_compile $source $dest $type $options]
>  }
>  
Your Tcl syntax looks suspicious to me.  Is there a reason for "ada" to
be in both double quotes and braces?

Perhaps {lappend options ada} might be simpler?  Is placing ada at the
beginning of the list important?

-- Jacob

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada

Jacob Bachmeyer
In reply to this post by Maciej Rozycki
Maciej W. Rozycki wrote:

> Unrecognized `gnatmake' switches are not implicitly passed on to the
> linker, so just pasting board `ldflags' and any other linker flags
> verbatim into `add_flags' to use for the invocation line of `gnatmake'
> will make them ignored at best.
>
> [...]
>
> For `gnatmake' to pass switches on to the linker the `-largs' switch has
> to be used, which affects all the switches that follow until a switch is
> seen that changes the selection, like `-margs', which resets to the
> initial state of the switch interpretation machine.
>
> Wrap linker flags into `-largs'/`-margs' for Ada then, carefully
> preserving the place these flags are placed within `add_flags', as
> surely someone will have depended on that, [...]

Fortunately, `add_flags' is a procedure local variable in
default_target_compile, so it is not visible outside of that procedure.

This patch really exposes a significant deficiency in our current
implementation of default_target_compile:  the order of various flags
can be significant, but we only have that order implicitly expressed in
the code, which goes all the way back to (of course) the "Initial
revision" that is probably from a time before Tcl had the features that
will allow significant cleanup in here.

Rather than introducing more variables, I propose changing add_flags to
an array and collecting each category of flags into its own element,
then emitting those elements in a fixed order to build the `opts' list.
This would also allow us to easily support other cases, for example,
prefixing "special" linker flags with "-Wl," or similar handling for
other frontends. I think we only need to support GCC command syntax,
which simplifies the issue a bit, but even GCC frontends are not 100%
consistent, as this issue with gnatmake shows.


What categories do the flags currently accumulated in `add_flags'
cover?  I see at least:
    - compiler flags (adaflags/cxxflags/dflags/f77flags/f90flags)
    - explicit additional flags ("additional_flags=" option)
    - explicit ldflags ("ldflags=" option)
    - libraries ("libs=" option)
    - preprocessor search paths ("incdir=" option)
    - linker search paths ("libdir=" option and [board_info $dest libs])
    - linker script ("ldscript=" option or [board_info $dest ldscript])
    - optimization flags ("optimize=" option)
    - target compiler flags from host ([board_info $host cflags_for_target])
    - type selection flag ("-c"/"-E"/"-S")
    - target compiler flags ([board_info $dest cflags] *regardless* of
the compiler selected)
    - target linker flags ([board_info $dest ldflags])
    - special flags for C++ ([g++_link_flags])
    - an attempt to locate libstdc++, also regardless of compiler
    - debug flags, if the "debug" option is given
    - the math library
    - "-Wl,-r" if board_info knows a "remote_link" key
    - "-Wl,-oformat,[board_info $dest output_format]" if that is defined
    - multilib flags, currently *prepended* if defined
    - a destination file

Some of these could probably be combined and I may have combined
categories that should be separate in the above list.  The GNU toolchain
has always been a kind of "magic box that just works" (until it doesn't
and the manual explains the problem) for me, so I am uncertain what the
ordering rules for combining these categories should be.  Anyone know
the traditional rules and, perhaps more importantly, what systems need
which rules?

-- Jacob
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3][GCC] GNAT/testsuite: Pass the `ada' option to target compilation

Maciej Rozycki
In reply to this post by Jacob Bachmeyer
On Wed, 15 May 2019, Jacob Bachmeyer wrote:

> > Index: gcc/gcc/testsuite/lib/gnat.exp
> > ===================================================================
> > --- gcc.orig/gcc/testsuite/lib/gnat.exp
> > +++ gcc/gcc/testsuite/lib/gnat.exp
> > @@ -167,6 +167,8 @@ proc gnat_target_compile { source dest t
> >   set options [concat "additional_flags=$TOOL_OPTIONS" $options]
> >      }
> >  
> > +    set options [concat "{ada}" $options]
> > +
> >      return [target_compile $source $dest $type $options]
> >  }
> >  
> Your Tcl syntax looks suspicious to me.  Is there a reason for "ada" to
> be in both double quotes and braces?

 Most existing `options' elements are lists, as shown by:

clone_output "options: $options\n"

placed at the top of `default_target_compile' (leading paths stripped
here):

options: {ada} {additional_flags=-fno-diagnostics-show-caret -fno-diagnostics-show-line-numbers -fdiagnostics-color=never --sysroot=.../sysroot} {additional_flags=-gnatJ -c -u} {compiler=.../gcc/gnatmake --GCC=.../gcc/xgcc --GNATBIND=.../gcc/gnatbind --GNATLINK=.../gcc/gnatlink -cargs -B.../gcc -largs --GCC=.../gcc/xgcc\ -B.../gcc\ -march=rv64imafdc\ -mabi=lp64d -margs --RTS=.../riscv64-linux-gnu/lib64/lp64d/libada -q -f} timeout=300

so I did this for consistency, although in reality it doesn't matter, not
at least for `default_target_compile', and either approach would work.

 We are not consistent here in `gnat_target_compile' anyway, as you can
see from the two existing `concat' invocations, and also the `timeout=300'
element.

> Perhaps {lappend options ada} might be simpler?  Is placing ada at the
> beginning of the list important?

 It can't be last because we override the default compiler otherwise
selected by this option in `default_target_compile', and then options
passed in may override it further.  Overall I felt it to be safer if we
placed the compiler type selection first rather than somewhere in the
middle.

 I hope it clears your concerns.

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada

Maciej Rozycki
In reply to this post by Jacob Bachmeyer
On Wed, 15 May 2019, Jacob Bachmeyer wrote:

> This patch really exposes a significant deficiency in our current
> implementation of default_target_compile:  the order of various flags
> can be significant, but we only have that order implicitly expressed in
> the code, which goes all the way back to (of course) the "Initial
> revision" that is probably from a time before Tcl had the features that
> will allow significant cleanup in here.

 I suspect the origins may be different, however as valuable as your
observation is functional problems have precedence over issues with code
structuring, so we need to fix the problem at hand first.  I'm sure
DejaGNU maintainers will be happy to review your implementation of code
restructuring afterwards.

> Some of these could probably be combined and I may have combined
> categories that should be separate in the above list.  The GNU toolchain
> has always been a kind of "magic box that just works" (until it doesn't
> and the manual explains the problem) for me, so I am uncertain what the
> ordering rules for combining these categories should be.  Anyone know
> the traditional rules and, perhaps more importantly, what systems need
> which rules?

 The ordering rules are system-specific I'm afraid and we have to be
careful not to break working systems out there.  People may be forced to a
DejaGNU upgrate, due to a newer version of a program being tested having
such a requirement, and can legitimately expect their system to continue
working.

 NB I have been repeatedly observing cases where a forced upgrade of a
system component I neither care nor I am competent about, triggered by an
upgrade of a component I do care about, caused the system to malfunction
in a way that I find both unacceptable and extremely hard to debug.  It
seems to have become more frequent in the recent years, and I find this
both very frustrating and have wasted lots of time trying to fix the
damage caused.  I would therefore suggest to take all the measures
possible to save people from going through such an experience.

 FWIW,

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/3][GCC] GNAT/testsuite: Pass the `ada' option to target compilation

Jacob Bachmeyer
In reply to this post by Maciej Rozycki
Maciej W. Rozycki wrote:
> On Wed, 15 May 2019, Jacob Bachmeyer wrote:
> [...]
>  We are not consistent here in `gnat_target_compile' anyway, as you can
> see from the two existing `concat' invocations, and also the `timeout=300'
> element.
>  

That is the GCC testsuite rather than DejaGnu itself, so it is less of a
concern to me.

>> Perhaps {lappend options ada} might be simpler?  Is placing ada at the
>> beginning of the list important?
>>    
>
>  It can't be last because we override the default compiler otherwise
> selected by this option in `default_target_compile', and then options
> passed in may override it further.  Overall I felt it to be safer if we
> placed the compiler type selection first rather than somewhere in the
> middle.
>  

This is probably a bug in DejaGnu, (those options should set defaults
rather than override whatever else has been given) but you will still
need to work around it for the installed base.

>  I hope it clears your concerns.
>  

As far as the patch to GCC goes, I am not worried.

-- Jacob

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada

Jacob Bachmeyer
In reply to this post by Maciej Rozycki
Maciej W. Rozycki wrote:

> On Wed, 15 May 2019, Jacob Bachmeyer wrote:
>  
>> This patch really exposes a significant deficiency in our current
>> implementation of default_target_compile:  the order of various flags
>> can be significant, but we only have that order implicitly expressed in
>> the code, which goes all the way back to (of course) the "Initial
>> revision" that is probably from a time before Tcl had the features that
>> will allow significant cleanup in here.
>>    
>
>  I suspect the origins may be different, however as valuable as your
> observation is functional problems have precedence over issues with code
> structuring, so we need to fix the problem at hand first.  I'm sure
> DejaGNU maintainers will be happy to review your implementation of code
> restructuring afterwards.
>  

My concern is that your patch may only solve a small part of the problem
-- enough to make your specific case work, yes, but then someone else
will hit other parts of the problem later and we spiral towards "tissue
of hacks" unmaintainability.

The biggest hint to me that your patch is incomplete is that it only
adds -largs/-margs to wrap LDFLAGS.  I suspect that there are other
-?args options that should be used also with other flag sets, but those
do not appear in this patch.  Do we know what the GNU Ada frontend
actually expects?

>> Some of these could probably be combined and I may have combined
>> categories that should be separate in the above list.  The GNU toolchain
>> has always been a kind of "magic box that just works" (until it doesn't
>> and the manual explains the problem) for me, so I am uncertain what the
>> ordering rules for combining these categories should be.  Anyone know
>> the traditional rules and, perhaps more importantly, what systems need
>> which rules?
>>    
>
>  The ordering rules are system-specific I'm afraid and we have to be
> careful not to break working systems out there.  People may be forced to a
> DejaGNU upgrate, due to a newer version of a program being tested having
> such a requirement, and can legitimately expect their system to continue
> working.
>  

We can start by simply preserving the existing ordering until we know
something should change, but the main goal of my previous message was to
collect the requirements for a specification for default_target_compile
so I can write regression tests (some of which will fail due to known
bugs like broken Ada support in our current implementation) before
embarking on extensive changes to that procedure.  Improving
"target.test" was already on my local TODO list.

>  NB I have been repeatedly observing cases where a forced upgrade of a
> system component I neither care nor I am competent about, triggered by an
> upgrade of a component I do care about, caused the system to malfunction
> in a way that I find both unacceptable and extremely hard to debug.  It
> seems to have become more frequent in the recent years, and I find this
> both very frustrating and have wasted lots of time trying to fix the
> damage caused.  I would therefore suggest to take all the measures
> possible to save people from going through such an experience.
>  

Yes, I have also noticed an attitude that can be summed up as "Who cares
about backwards compatibility?  New!  Shiny!" usually from people who
have no clue and no business being anywhere near a source editor.  
(Surprise!  Their code has lots of bugs, usually severe, too.)  The
problem is not new -- jwz called it out as the "Cascade of
Attention-Deficit Teenagers" model, noting that it seemed to
particularly plague GNOME, long ago.
Unfortunately, people with that particular attitude seem to have
acquired outsize influence over the last few years.  I would suspect an
organized attack if I were more conspiracy-oriented, but Hanlon's razor
strongly suggests that this is simply a consequence of lowering barriers
to entry.

-- Jacob

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada

Maciej Rozycki
On Thu, 16 May 2019, Jacob Bachmeyer wrote:

> >  I suspect the origins may be different, however as valuable as your
> > observation is functional problems have precedence over issues with code
> > structuring, so we need to fix the problem at hand first.  I'm sure
> > DejaGNU maintainers will be happy to review your implementation of code
> > restructuring afterwards.
>
> My concern is that your patch may only solve a small part of the problem
> -- enough to make your specific case work, yes, but then someone else
> will hit other parts of the problem later and we spiral towards "tissue
> of hacks" unmaintainability.

 I think however that fixing problems in small steps as they are
discovered is also a reasonable approach and a way to move forward:
perfect is the enemy of good.

 So I don't think the prospect of making a comprehensive solution should
prevent a simple fix for the problem at hand that has been already
developed from being applied.

 IOW I don't discourage you from developing a comprehensive solution,
however applying my proposal right away will help at least some people and
will not block you in any way.

> The biggest hint to me that your patch is incomplete is that it only
> adds -largs/-margs to wrap LDFLAGS.  I suspect that there are other
> -?args options that should be used also with other flag sets, but those
> do not appear in this patch.  Do we know what the GNU Ada frontend
> actually expects?

 At first glance it looks to me we should be safe overall as compiler
flags are supposed to be passed through by `gnatmake' (barring switch
processing bugs, as observed with 1/3), and IIUC assembler flags are
considered compiler flags for the purpose of this consideration as
`gnatmake' does not make assembly a separate build stage.  So while we
could wrap compiler flags into `-cargs'/`-margs', it would only serve to
avoid possible `gnatmake' switch processing bugs.

 There's also `-bargs' for binder switches, but I can't see any use for it
here.

 Finally boards are offered the choice of `adaflags', `cflags',
`cxxflags', etc. for the individual languages, where the correct syntax
can be used if anything unusual is needed beyond what I have noted above.

 I'll defer any further consideration to the Ada maintainers cc-ed; I do
hope I haven't missed anything here, but then Ada is far from being my
primary area of experience.

> >  The ordering rules are system-specific I'm afraid and we have to be
> > careful not to break working systems out there.  People may be forced to a
> > DejaGNU upgrate, due to a newer version of a program being tested having
> > such a requirement, and can legitimately expect their system to continue
> > working.
>
> We can start by simply preserving the existing ordering until we know
> something should change, but the main goal of my previous message was to
> collect the requirements for a specification for default_target_compile
> so I can write regression tests (some of which will fail due to known
> bugs like broken Ada support in our current implementation) before
> embarking on extensive changes to that procedure.  Improving
> "target.test" was already on my local TODO list.

 You are welcome to go ahead with your effort as far as I am concerned.

> Unfortunately, people with that particular attitude seem to have
> acquired outsize influence over the last few years.  I would suspect an
> organized attack if I were more conspiracy-oriented, but Hanlon's razor
> strongly suggests that this is simply a consequence of lowering barriers
> to entry.

 Nod.

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada

Jacob Bachmeyer
Maciej Rozycki wrote:

> On Thu, 16 May 2019, Jacob Bachmeyer wrote:
>
>  
>>>  I suspect the origins may be different, however as valuable as your
>>> observation is functional problems have precedence over issues with code
>>> structuring, so we need to fix the problem at hand first.  I'm sure
>>> DejaGNU maintainers will be happy to review your implementation of code
>>> restructuring afterwards.
>>>      
>> My concern is that your patch may only solve a small part of the problem
>> -- enough to make your specific case work, yes, but then someone else
>> will hit other parts of the problem later and we spiral towards "tissue
>> of hacks" unmaintainability.
>>    
>
>  I think however that fixing problems in small steps as they are
> discovered is also a reasonable approach and a way to move forward:
> perfect is the enemy of good.
>  

Fair enough; observe the small patches I have recently submitted to DejaGnu.

>  So I don't think the prospect of making a comprehensive solution should
> prevent a simple fix for the problem at hand that has been already
> developed from being applied.
>  

I recognize a difference between "simple but complete" (an ideal
sometimes achieved in practice) and "simple because incomplete" (which
does not actually fix the problem).  My concerns are that your patch may
be the latter.

>  IOW I don't discourage you from developing a comprehensive solution,
> however applying my proposal right away will help at least some people and
> will not block you in any way.
>  

Correct, although, considering how long my FSF paperwork took, I might
be able to finish a comprehensive patch before your paperwork is
completed.  :-)

>> The biggest hint to me that your patch is incomplete is that it only
>> adds -largs/-margs to wrap LDFLAGS.  I suspect that there are other
>> -?args options that should be used also with other flag sets, but those
>> do not appear in this patch.  Do we know what the GNU Ada frontend
>> actually expects?
>>    
>
>  At first glance it looks to me we should be safe overall as compiler
> flags are supposed to be passed through by `gnatmake' (barring switch
> processing bugs, as observed with 1/3), and IIUC assembler flags are
> considered compiler flags for the purpose of this consideration as
> `gnatmake' does not make assembly a separate build stage.  So while we
> could wrap compiler flags into `-cargs'/`-margs', it would only serve to
> avoid possible `gnatmake' switch processing bugs.
>  

I am not sure if those are actually bugs in `gnatmake' or the result of
an incomplete specification for `gnatmake' -- I suspect that --sysroot=
may have been added to the rest of GCC after `gnatmake' was written and
whoever added it did not update the Ada frontend.

>  There's also `-bargs' for binder switches, but I can't see any use for it
> here.
>
>  Finally boards are offered the choice of `adaflags', `cflags',
> `cxxflags', etc. for the individual languages, where the correct syntax
> can be used if anything unusual is needed beyond what I have noted above.
>  

Which also raises the issue of "cflags_for_target" (used regardless of
language and currently always taken from the "unix" board configuration)
and how that is supposed to make sense and whether it should be
similarly split into language-specific values or simply removed.  I have
already submitted a patch to draw that value from the actual host board
configuration.

>  I'll defer any further consideration to the Ada maintainers cc-ed; I do
> hope I haven't missed anything here, but then Ada is far from being my
> primary area of experience.
>  

Likewise, hopefully some of the Ada maintainers will be able to shed
light on this issue.  And I hope Ben (the DejaGnu maintainer) is okay --
I would have expected him to comment by now.

>>>  The ordering rules are system-specific I'm afraid and we have to be
>>> careful not to break working systems out there.  People may be forced to a
>>> DejaGNU upgrate, due to a newer version of a program being tested having
>>> such a requirement, and can legitimately expect their system to continue
>>> working.
>>>      
>> We can start by simply preserving the existing ordering until we know
>> something should change, but the main goal of my previous message was to
>> collect the requirements for a specification for default_target_compile
>> so I can write regression tests (some of which will fail due to known
>> bugs like broken Ada support in our current implementation) before
>> embarking on extensive changes to that procedure.  Improving
>> "target.test" was already on my local TODO list.
>>    
>
>  You are welcome to go ahead with your effort as far as I am concerned.
>  

I am working on it.  :-)

-- Jacob
Reply | Threaded
Open this post in threaded view
|

[PING][PATCH 0/3] GNAT test suite fixes for build sysroot

Maciej Rozycki
In reply to this post by Maciej Rozycki
On Tue, 14 May 2019, Maciej W. Rozycki wrote:

>  In the course of setting up GCC regression testing for the RISC-V target
> I have discovered that the GNAT test suite does not correctly respond to
> the test environment settings passed from the test harness in my setup and
> consequently no test case works correctly.

 Ping for:

<https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00753.html>
<https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00754.html>
<https://lists.gnu.org/archive/html/dejagnu/2019-05/msg00000.html>

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PING][PATCH 0/3] GNAT test suite fixes for build sysroot

Arnaud Charlet
> >  In the course of setting up GCC regression testing for the RISC-V target
> > I have discovered that the GNAT test suite does not correctly respond to
> > the test environment settings passed from the test harness in my setup and
> > consequently no test case works correctly.
>
>  Ping for:
>
> <https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00753.html>

Have you resolved your copyright assignment issues since then?

The above patch needs to use "or else" instead of "or". OK with this change
on the above patch.

Arno
Reply | Threaded
Open this post in threaded view
|

Re: [PING][PATCH 0/3] GNAT test suite fixes for build sysroot

Maciej Rozycki
On Wed, 19 Jun 2019, Arnaud Charlet wrote:

> >  Ping for:
> >
> > <https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00753.html>
>
> Have you resolved your copyright assignment issues since then?

 The ball is now in FSF's court I'm told.

> The above patch needs to use "or else" instead of "or". OK with this change
> on the above patch.

 OK, I have updated that in my patch.

 Technically both variants of the expression achieve the same effect here
as there is no problem with evaluating both sides of the OR operation in
all cases, but your suggestion might help the readers avoid scratching
their heads.

 Thank you for your review.  I will apply the change in due course.

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PING][PATCH 0/3] GNAT test suite fixes for build sysroot

Arnaud Charlet
> > Have you resolved your copyright assignment issues since then?
>
>  The ball is now in FSF's court I'm told.

OK

> > The above patch needs to use "or else" instead of "or". OK with this change
> > on the above patch.
>
>  OK, I have updated that in my patch.
>
>  Technically both variants of the expression achieve the same effect here
> as there is no problem with evaluating both sides of the OR operation in
> all cases, but your suggestion might help the readers avoid scratching
> their heads.

The performance isn't the same, and more importantly, this is the documented
Ada coding style for GNAT: https://gcc.gnu.org/onlinedocs/gnat-style/Statements.html#Statements

Arno
Reply | Threaded
Open this post in threaded view
|

Re: [PING][PATCH 0/3] GNAT test suite fixes for build sysroot

Maciej Rozycki
On Thu, 20 Jun 2019, Arnaud Charlet wrote:

> >  Technically both variants of the expression achieve the same effect here
> > as there is no problem with evaluating both sides of the OR operation in
> > all cases, but your suggestion might help the readers avoid scratching
> > their heads.
>
> The performance isn't the same, and more importantly, this is the documented
> Ada coding style for GNAT: https://gcc.gnu.org/onlinedocs/gnat-style/Statements.html#Statements

 Thanks for the pointer, good to know!

  Maciej
Reply | Threaded
Open this post in threaded view
|

[PING^2][PATCH 0/3] GNAT test suite fixes for build sysroot

Maciej Rozycki
In reply to this post by Maciej Rozycki
On Tue, 14 May 2019, Maciej W. Rozycki wrote:

>  In the course of setting up GCC regression testing for the RISC-V target
> I have discovered that the GNAT test suite does not correctly respond to
> the test environment settings passed from the test harness in my setup and
> consequently no test case works correctly.

 Ping for:

<https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00754.html>
<https://lists.gnu.org/archive/html/dejagnu/2019-05/msg00000.html>

  Maciej
Reply | Threaded
Open this post in threaded view
|

Re: [PING^2][PATCH 0/3] GNAT test suite fixes for build sysroot

Arnaud Charlet
> > In the course of setting up GCC regression testing for the RISC-V target
> > I have discovered that the GNAT test suite does not correctly respond to
> > the test environment settings passed from the test harness in my setup and
> > consequently no test case works correctly.
>
>  Ping for:
>
> <https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00754.html>
> <https://lists.gnu.org/archive/html/dejagnu/2019-05/msg00000.html>

Assuming good Ada test results with these changes for both
RISC-V and native linux x86_64, OK for me.

Arno
Reply | Threaded
Open this post in threaded view
|

Re: [PING^2][PATCH 0/3] GNAT test suite fixes for build sysroot

Maciej Rozycki
On Mon, 16 Sep 2019, Arnaud Charlet wrote:

> > > In the course of setting up GCC regression testing for the RISC-V target
> > > I have discovered that the GNAT test suite does not correctly respond to
> > > the test environment settings passed from the test harness in my setup and
> > > consequently no test case works correctly.
> >
> >  Ping for:
> >
> > <https://gcc.gnu.org/ml/gcc-patches/2019-05/msg00754.html>
> > <https://lists.gnu.org/archive/html/dejagnu/2019-05/msg00000.html>
>
> Assuming good Ada test results with these changes for both
> RISC-V and native linux x86_64, OK for me.

 I have completed regression-testing now and there were no changes at all
for native x86_64/Linux and only progressions for crossed RISC-V/Linux:

$ diff -u old/gnat.sum new/gnat.sum | sed -n '/^\(+FAIL\|-PASS\)/p;/Summary/,${s,^.*/,,;p}'
  === gnat Summary ===
 
-# of expected passes 2137
-# of unexpected failures 710
+# of expected passes 3184
+# of unexpected failures 6
 # of expected failures 23
 # of unsupported tests 28
gnatmake version 10.0.0 20190917 (experimental)

I have therefore committed 2/3 now, however 3/3 requires a DejaGNU
maintainer's attention.

 Thanks for your review.

  Maciej