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

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 <macro@wdc.com>
---
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

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

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

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

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

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

On Tue, 21 May 2019, Jacob Bachmeyer wrote:

> >  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.  :-)

 So by now the FSF paperwork has been long completed actually.

> >  You are welcome to go ahead with your effort as far as I am concerned.
> >   
> 
> I am working on it.  :-)

 Hopefully you have made good progress now.  Otherwise this is a ping for:

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

Once complete your change can go on top of mine and meanwhile we'll have 
a working GCC test suite.

  Maciej

Maciej W. Rozycki wrote:
> On Tue, 21 May 2019, Jacob Bachmeyer wrote:
>   
>>>  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.  :-)
>>     
>
>  So by now the FSF paperwork has been long completed actually.
>   

Yes, that turned out to be optimistic, for a few reasons.

>>>  You are welcome to go ahead with your effort as far as I am concerned.
>>>   
>>>       
>> I am working on it.  :-)
>>     
>
>  Hopefully you have made good progress now.

Progress is stalled because the DejaGNU maintainer has not been seen on 
this list since March and I am unsure about working too far ahead of the 
"accepted" line.

> Otherwise this is a ping for:
>
> <https://lists.gnu.org/archive/html/dejagnu/2019-05/msg00000.html>
>
> Once complete your change can go on top of mine and meanwhile we'll have 
> a working GCC test suite.
>   

There might be a merge conflict, but that will be easy to resolve by 
overwriting your patch with mine.  I will make sure to include the 
functionality in the rewrite.

I have just sent a patch to the list that has been waiting in my local 
repository since June.  It adds unit tests for the 
default_target_compile procedure, but currently verifies the broken Ada 
handling.  Would you be willing to supply a patch to update those tests 
to the correct behavior?  If so, I will also merge your code on my local 
branch and we might even avoid the merge conflict down the line.

While you are doing that, could you also explain what the various -?args 
GNU Ada driver options do and if any others are needed or could be 
needed?  I will ensure that the rewrite handles all cases if I can get a 
solid description of what those cases actually are.  The rewrite will 
group complier/linker/etc. options in separate lists internally, so 
using those options will be easy without adding more hacks to a 
procedure that has already become a tissue of hacks.

-- Jacob