Message ID | alpine.DEB.2.20.1905142145350.18422@tpp.hgst.com |
---|---|
State | Not Applicable |
Headers | show |
Series | GNAT test suite fixes for build sysroot | expand |
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
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"
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