Re: [PING^3][PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada
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/msg0.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
Generalizing DejaGnu timeout scaling (was: Re: [PATCH DejaGNU/GCC 0/1] Support per-test execution timeout factor)
Maciej W. Rozycki wrote: On Wed, 3 Jan 2024, Hans-Peter Nilsson wrote: The test execution timeout is different from the tool execution timeout where it is GCC execution that is being guarded against taking excessive amount of time on the test host rather than the resulting test case executable run on the target afterwards, as concerned here. GCC already has a `dg-timeout-factor' setting for the tool execution timeout, but has no means to increase the test execution timeout. The GCC side of these changes adds a corresponding `dg-test-timeout-factor' setting. Hmm. I think it would be more correct to emphasize that the existing dg-timeout-factor affects both the tool execution *and* the test execution, whereas your new dg-test-timeout-factor only affects the test execution. (And still measured on the host.) Not really, `dg-timeout-factor' is only applied to tool execution and it doesn't affect test execution. Timeout value reporting used to be limited in DejaGNU, but you can enable it easily now by adding the DejaGNU patch series referred in the cover letter and see that `dg-timeout-factor' is ignored for test execution. Then we need a better name for this new feature that more clearly indicates that it applies to running executables compiled as part of a test. Also, 'test_timeout' is documented as a knob for site configuration to twiddle, not for testsuites to adjust. I support adding scale factors for testsuites to indicate "this test takes longer than usual" but these will need to be thought through. This quick hack will cause future maintenance problems. Usually the compilation time is close to 0, so is this based on an actual need more than an itchy "wart"? Or did I miss something? Compilation is usually quite fast, but this is not always the case. If you look at the tests that do use `dg-timeout-factor' in GCC, and some commits that added the setting, then you ought to find actual use cases. I saw at least one such a test that takes an awful lot of time here on a reasonably fast host machine and still passes where GCC has been built with optimisation enabled, but does time out in the compilation phase if the compiler has been built at -O0 for debugging purposes. I'd have to chase it though if you couldn't find it as I haven't written the name down. So yes, `dg-timeout-factor' does have its use, but it is different from that of `dg-test-timeout-factor', hence the need for a separate setting. This name has already caused confusion and the patch has not even been accepted yet. The feature is desirable but this implementation is not acceptable. At the moment, there are two blocking issues with this patch: 1. The global variable name 'test_timeout_factor' is not acceptable because it has already caused confusion, apparently among GCC developers who should be familiar with the GCC testsuite. If it already confuses GCC testsuite domain experts, its meaning is too unclear for general use. While looking for alternative names, I found the fundamental problem with this proposed implementation: test phases (such as running a test program versus running the tool itself) are defined by the testsuite, not by the framework. DejaGnu therefore cannot explicitly support this as offered because the proposal violates encapsulation both ways. 2. New code in DejaGnu using expr(n) is to have the expression braced as recommended in the expr(n) manpage, unless it actually uses the semantics provided by unbraced expr expressions, in which case it *needs* a comment explaining and justifying that. The second issue is trivially fixable, but the first appears fatal. There is a new "testcase" mulitplex command in Git master, which will be included in the next release, that is intended for testsuites to express dynamic state. The original planned use was to support hierarchical test groups, for which a "testcase group" command is currently defined. In the future, dg.exp will be extended to use "testcase group" to delimit each testcase that it processes, and the framework will itself explicitly track each test script as a group. (DejaGnu's current semantics implicitly group tests by test scripts, but only by (*.exp) scripts.) Could this multiplex be a suitable place to put this API feature? Using a command also has the advantage that it will cause a hard failure if the framework does not implement it, unlike a variable that a test script can set for the framework to silently ignore, leading to hard-to-reproduce test (timeout) failures if an older framework is used with a testsuite expecting this feature. The semantics of "testcase patience" or similar would be defined to extend to the end of the group (or test script in versions of DejaGnu that do not fully implement groups) in which it is executed. This limited scope is needed because allowing timeout scale factors to "bleed over" to the next test sc
Re: Generalizing DejaGnu timeout scaling
Hans-Peter Nilsson wrote: On Wed, 3 Jan 2024, Jacob Bachmeyer wrote: Comments before I start on an implementation? I'd suggest to await the conclusion of the debate: I *think* I've proved that dg-timeout-factor is already active as intended (all parts of a test), specifically when the compilation result is executed (for the applicable tests). Notably, modulo bugs in the test-suites. The dg-timeout-factor tag is a GCC testsuite feature; the dg-patience tag will be an upstream DejaGnu framework feature using shared infrastructure also available to tests not using dg.exp. Improved timeout handling will also eventually include per-target timeout defaults and scale factors, to allow testing sites to adjust timeouts for slow (or fast) targets. Of course, it may be useful to separate different timeouts of separable parts of a test - compilation and execution being the topic at hand. But IMHO, YAGNI. Having said that, don't let that stand in the way of a fun hack! It will go on the TODO list either way; the only difference is the priority it will have. -- Jacob
Re: [PATCH DejaGNU 1/1] Support per-test execution timeout factor
Maciej W. Rozycki wrote:
Add support for the `test_timeout_factor' global variable letting a test
case scale the wait timeout used for code execution. This is useful for
particularly slow test cases for which increasing the wait timeout
globally would be excessive.
* baseboards/qemu.exp (qemu_load): Handle `test_timeout_factor'.
* config/gdb-comm.exp (gdb_comm_load): Likewise.
* config/gdb_stub.exp (gdb_stub_load): Likewise.
* config/sim.exp (sim_load): Likewise.
* config/unix.exp (unix_load): Likewise.
* doc/dejagnu.texi (Local configuration file): Document
`test_timeout_factor'.
[...snip full diff...]
First, a minor technical issue: brace your expr(n) expressions like this:
set wait_timeout [expr { $wait_timeout * $test_timeout_factor }]
The Tcl expr(n) manpage recommends that style and explains a few
situations where it is actually required for non-surprising results and
that Tcl's optimizations work better if the expression to expr is
braced. All expr calls in new code in DejaGnu should have the braces.
Second, I need some more explanation how this fits together because I
have some concerns about confusion between various timeouts. In your
introduction to this patch pair, you note that the test execution
timeout and tool execution timeout are different. My main concern is
that "test_timeout_factor" (and for that matter, "test_timeout") may be
badly named, or we need a more coherent model of testing with DejaGnu.
(More precisely, we need better documentation...)
The anticipated confusion stems from the question of what exactly is the
interval of a test? In other words, what is the interval limited by
"test_timeout"? When does the clock start ticking and when does it stop
before the alarm goes off? (I have some suspicions that those answers
are annoyingly counter-intuitive, which means I will have to write more
documentation...)
Lastly, I note no objection to the dg-test-timeout-factor extension; as
far as I can tell, dg.exp is designed to be extended in that way, so
this is a supported extension point instead of an unsupportable monkeypatch.
-- Jacob
Re: [PATCH] libstdc++-v3: testsuite: Prune uncapitalized "in function" linker warning
Hans-Peter Nilsson wrote: (CC to the dejagnu project as a heads-up) Regtested cris-elf with a fresh newlib checkout where 2640 libstdc++-v3 tests otherwise fail due to the stubbed newlib _getentropy. Ok to commit? -- >8 -- Newer newlib trigger warnings about certain functions not implemented (_getentropy) when testing libstdc++-v3. Since 2018 (circa binutils-2.10) the "in function" prefix isn't capitalized for those "not implemented" warnings when generated from the linker (a GNU ld feature used by newlib). Dejagnu up to and including at least dejagnu-1.6.3 (and git @ 42979bd3b9) assumes a capital "In function", leaving that part unpruned, and boom we have thousands of "excess errors" from the libstdc++-v3 testsuite. To confirm: newer binutils drops capitalization in a message "in function FOO\nBAR is not implemented and will always fail". DejaGnu expects GNU ld to say "In function FOO..." and fails to recognize the new form. If this is correct, I will fix it in Git master. (The fix is trivial: change the "I" to "[Ii]" to accept both forms.) (You may still want to add the patch to the testsuite, for compatibility with older versions of DejaGnu.) -- Jacob
Re: [PATCH] libstdc++-v3: testsuite: Prune uncapitalized "in function" linker warning
Hans-Peter Nilsson wrote:
Date: Wed, 14 Aug 2024 20:58:04 -0500
From: Jacob Bachmeyer
Reply-To: jcb62...@gmail.com
[...]
If this is correct, I will fix it in Git master. (The fix is trivial:
change the "I" to "[Ii]" to accept both forms.)
Yes, thanks; just don't forget to escape the []. :)
Of course; I followed the escaping elsewhere in those patterns. (I
probably should change them to {} strings at some point, since no
variables are interpolated.)
Done and pushed to Savannah as commit
ed301dbd6a3d769670503ccfda1ea31b58d02547. Please confirm that this
solves the problem.
(Also note that you can now run DejaGnu from a Git checkout, simply use
the "runtest" in the Git working directory. Any problems with this are
bugs and will be fixed.)
-- Jacob
Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada
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
Re: [PATCH 2/3][GCC] GNAT/testsuite: Pass the `ada' option to target compilation
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
Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada
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
Re: [PATCH 2/3][GCC] GNAT/testsuite: Pass the `ada' option to target compilation
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
Re: [PATCH 3/3][DejaGNU] target: Wrap linker flags into `-largs'/`-margs' for Ada
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
