Handle platforms with renamed "readelf"
AbandonedPublic

Authored by thomie on Oct 14 2015, 4:49 PM.

Details

Reviewers
kgardas
erikd
austin
bgamari
ony
Trac Issues
#10974
Summary
  • Add -pgmreadelf option
  • Auto configure path and name of "readelf"
  • Add settings readelf command
  • During build of GHC name of tool can be changed with ./configure READELF=myreadelf
Test Plan
make -C testsuite/tests/driver/recomp011
There are a very large number of changes, so older changes are hidden. Show Older Changes
ony edited edge metadata.Oct 15 2015, 4:11 PM
ony updated the Trac tickets for this revision.
ony edited the test plan for this revision. (Show Details)Oct 15 2015, 4:22 PM
ony updated this object.Oct 15 2015, 11:56 PM
thomie requested changes to this revision.Oct 16 2015, 3:16 AM
thomie edited edge metadata.

I don't know why, but recomp011 is now failing. See https://phabricator.haskell.org/harbormaster/build/6753/?l=50:

Stderr:
993	: createProcess: runInteractiveProcess: exec: does not exist (No such file or directory)
994	make[2]: *** [recomp011] Error 1
995	
996	*** unexpected failure for recomp011(normal)
This revision now requires changes to proceed.Oct 16 2015, 3:16 AM
ony added a comment.Oct 16 2015, 3:25 AM
In D1326#37930, @thomie wrote:

I don't know why, but recomp011 is now failing. See https://phabricator.haskell.org/harbormaster/build/6753/?l=50:

Stderr:
993	: createProcess: runInteractiveProcess: exec: does not exist (No such file or directory)
994	make[2]: *** [recomp011] Error 1
995	
996	*** unexpected failure for recomp011(normal)

That's a good question. Since there is no access to config.log I can't figure this out.
I tried to check how can it happen, but have no idea. One of the way is that --target were provided on platform where readelf have no target-prefix. But it should fail during configuration.
It looks like after running configure in settings there is:

("readelf command", ""),

Instead of

("readelf command", "/usr/host/bin/x86_64-pc-linux-gnu-readelf"),

I wish I were able to re-produce similar environment either in virtual machine or chroot or whatever.

In case it helps, on my system it finds: SettingsReadElfCommand='/usr/bin/readelf' (Ubuntu 14.04).

ony added a comment.EditedOct 16 2015, 10:45 AM
In D1326#37935, @thomie wrote:

In case it helps, on my system it finds: SettingsReadElfCommand='/usr/bin/readelf' (Ubuntu 14.04).

On my system it finds target prefixed /use/host/bin/x86_64-pc-linux-gnu-readelf .
Without config.log we can only guess.
Maybe issue in that m4 macro

ony updated this revision to Diff 4518.Oct 16 2015, 12:52 PM
ony edited edge metadata.

gather some info from test machine were unit-test driver/recomp011 fails

ony planned changes to this revision.Oct 16 2015, 12:52 PM
ony updated this revision to Diff 4519.Oct 16 2015, 1:39 PM
ony edited edge metadata.

yet another try. now fail validate really fast (right after configure)

ony planned changes to this revision.Oct 16 2015, 1:39 PM
ony updated this revision to Diff 4526.Oct 16 2015, 10:06 PM
ony edited edge metadata.

Use AC_CHECK_TARGET_TOOL and keep only path to readelf

Looks like on test machines readProcessEnvWithExitCode "/usr/bin/readelf"
fails by some unknown reason. Let's use only name of tool.

To achive that there is no FP_-prefixed macro yet. But standard
AC_CHECK_TARGET_TOOL fulfils our needs completely.
There is no reason for adding configure argument --with-readelf since such
tools usually configured in a way ./configure READELF=myreadelf.

Note there is no good reason to compile-in full path for tools like
as/nm/ld/objdump/readelf. Moreover this is less flexible since you cannot use
icecc/distcc-like systems which inject their tools by changing PATH
environment variable.

ony planned changes to this revision.Oct 16 2015, 10:06 PM
ony updated this object.Oct 16 2015, 10:09 PM
ony edited the test plan for this revision. (Show Details)
ony edited edge metadata.
ony requested a review of this revision.Oct 16 2015, 10:13 PM

Now it shouldn't fail since first argument passed to readProcessEnvWithExitCode for test machine will be equal to old hard-coded value.

ony added a comment.Oct 16 2015, 11:03 PM

(pulls hair out of his head)
Same test driver/recomp011 failed and passed in a same run according to logs:

12	================== Testsuite summary ==================
13	
14	Unexpected results from:
15	TEST="recomp011"
16	
17	OVERALL SUMMARY for test run started at Sat Oct 17 03:29:52 2015 UTC
18	 0:06:09 spent to go through
19	    4747 total tests, which gave rise to
20	   15151 test cases, of which
21	   10355 were skipped
22	
23	      71 had missing libraries
24	    4621 expected passes
25	     103 expected failures
26	
27	       0 caused framework failures
28	       0 unexpected passes
29	       1 unexpected failures
30	       0 unexpected stat failures
31	
32	Unexpected failures:
33	   driver/recomp011  recomp011 [bad exit code] (normal)
51	Beginning test run at Sat Oct 17 03:36:15 2015 UTC
52	=====> recomp011(normal) 1 of 1 [0, 0, 0] 
53	cd ./driver/recomp011 && $MAKE -s --no-print-directory recomp011    </dev/null > recomp011.run.stdout 2> recomp011.run.stderr
54	
55	OVERALL SUMMARY for test run started at Sat Oct 17 03:36:15 2015 UTC
56	 0:00:06 spent to go through
57	       1 total tests, which gave rise to
58	       1 test cases, of which
59	       0 were skipped
60	
61	       0 had missing libraries
62	       1 expected passes
63	       0 expected failures
64	
65	       0 caused framework failures
66	       0 unexpected passes
67	       0 unexpected failures
68	       0 unexpected stat failures
69	
70	make[1]: Leaving directory `/srv/builds/patches/rGHC/B6797-D1326-4526/testsuite/tests'
71	make: Leaving directory `/srv/builds/patches/rGHC/B6797-D1326-4526/testsuite'

It looks like it runs ./validate without --no-clean flag. But from previous run I saw that settings actually had readelf command associated with value /usr/bin/readelf.

ony updated this revision to Diff 4530.Oct 17 2015, 3:01 AM
ony retitled this revision from Handle platforms with renamed "readelf" to Validate: force reconf if outdated and add --build.
ony updated this object.

Just in case ensure that validate does reconfigure

ony updated this revision to Diff 4545.Oct 17 2015, 10:24 AM
ony retitled this revision from Validate: force reconf if outdated and add --build to Handle platforms with renamed "readelf".
ony updated this object.
ony edited edge metadata.

Kick out validate from this diff.

Phabricator treats this as a single change rather than 2 commits.

ony planned changes to this revision.Oct 17 2015, 10:24 AM
ony requested a review of this revision.Oct 18 2015, 1:36 AM

That's a really wrong idea to use master always. Now I can't fix this issue since master now is broken.

thomie requested changes to this revision.Oct 19 2015, 4:30 AM
thomie edited edge metadata.

It still doesn't validate.

master is not broken according to https://phabricator.haskell.org/diffusion/GHC/history/

configure.ac
542

What happens on Windows?

This revision now requires changes to proceed.Oct 19 2015, 4:30 AM
ony added a comment.Oct 19 2015, 7:59 AM
In D1326#38329, @thomie wrote:

It still doesn't validate.

master is not broken according to https://phabricator.haskell.org/diffusion/GHC/history/

I don't think this change introduce:

992	rts/posix/Signals.c:536:1: error:
993	     error: ‘backtrace_handler’ defined but not used [-Werror=unused-function]
994	     backtrace_handler(int sig STG_UNUSED)
995	     ^
996	cc1: all warnings being treated as errors
configure.ac
542

Same that happens to ar/nm/ld/ranlib. They aren't used.
This condition is taken from code of FP_ARG_WITH_PATH_GNU_PROG. I do think it looks weird to check "host" to decide if we need "target" tool, but I decided that I just miss whole picture in my mind.
If you think it is wrong then you probably want to create a ticket and/or patch to change FP_ARG_WITH_PATH_GNU_PROG or migrate ar/nm/etc to some different macro.

bgamari edited edge metadata.Oct 19 2015, 10:43 AM

In principle it should be possible to scrape the build artifacts off of the buildbot. @austin, perhaps you could advise?

ony added a comment.EditedOct 19 2015, 11:48 PM

In principle it should be possible to scrape the build artifacts off of the buildbot. @austin, perhaps you could advise?

Were able to reproduce on my machine.

By some reason I have:

--- settings	2015-10-20 06:45:38.248921502 +0300
+++ "bindisttest/install   dir/lib/ghc-7.11.20151019/settings"	2015-10-20 07:38:58.943009601 +0300
@@ -17,7 +17,7 @@
  ("dllwrap command", "/bin/false"),
  ("windres command", "/bin/false"),
  ("libtool command", "libtool"),
- ("readelf command", "readelf"),
+ ("readelf command", ""),
  ("perl command", "/usr/host/bin/perl"),
  ("cross compiling", "NO"),
  ("target os", "OSLinux"),

Have to figure out what exactly that bindist means and why it generated with empty value for "readelf command".

Update: there is second configure script (distrib/configure.ac.in)

ony updated this revision to Diff 4569.Oct 20 2015, 12:15 AM
ony edited edge metadata.

Add missing change to distrib/configure.ac.in for BINDIST=YES

ony planned changes to this revision.Oct 20 2015, 12:15 AM
ony requested a review of this revision.Oct 20 2015, 12:57 AM

Great that you found the reason it wasn't validating before. Someone else ran into this problem some time ago, but I didn't think of it this time, sorry.

aclocal.m4
2042

Why does this now call AC_CHECK_TARGET_TOOL instead of FP_ARG_WITH_PATH_GNU_PROG (as it did in your initial version). Is that important? If so, please add a comment. Someone might be tempted to refactor this later, and perhaps introduce a regression. If it isn't important, please make the code as similar as possible to the way the other tools are found (ar/nm/ld/ranlib). Then future readers don't have to go and lookup the documentation for AC_CHECK_TARGET_TOOL to figure out why this one might be different.

thomie requested changes to this revision.Oct 20 2015, 9:31 AM
thomie edited edge metadata.
This revision now requires changes to proceed.Oct 20 2015, 9:31 AM
ony requested a review of this revision.Oct 20 2015, 12:25 PM
ony edited edge metadata.
In D1326#38486, @thomie wrote:

Great that you found the reason it wasn't validating before. Someone else ran into this problem some time ago, but I didn't think of it this time, sorry.

No problem. Kinda quest.

aclocal.m4
2042

Why does this now call AC_CHECK_TARGET_TOOL instead of FP_ARG_WITH_PATH_GNU_PROG (as it did in your initial version). Is that important?

  • It results in a same value for prog in readElfSection as it were before (for platforms supported before this change). So it shouldn't break anything.
  • There is no need to add dedicated argument for overriding autodetected value since it already handled by adding READELF=readelf-wrapper as parameter to ./configure. And there is no --without-readelf support for *nix platforms.
  • In case if we have platform with readelf supporting wide range of target platforms and thus having no target prefix we'll successfully fallback to readelf only name even that --target=x86_64-pc-linux-gnu were specified.
  • Storing name instead of path gives a bit more flexibility. I can change PATH to point to a different place with some wrappers that tweaks behavior of system tools. As an example - icecc and distcc use that technique.
  • AC_CHECK_TARGET_TOOL is a standard way to find name for tool that works with target platform.
  • If *nix system changes layout during update which results in changing location of tool readelf GHC wouldn't be broken.

That change (to autoconf alternative) was as an attempt to minimize effect of this change for already supported platforms.

Someone might be tempted to refactor this later, and perhaps introduce a regression. If it isn't important, please make the code as similar as possible to the way the other tools are found (ar/nm/ld/ranlib).

Yep. And I'm the one of those who is tempting to do this. I believe we shouldn't compile/configure-in full path to binutils. We should use autoconf as much as possible if we base our build infrastructure on it.
Unfortunately getting rid of associated --with-* arguments requires major version bump and thus cannot be targeted ghc-7.10.

Then future readers don't have to go and lookup the documentation for AC_CHECK_TARGET_TOOL to figure out why this one might be different.

Right now I had to go through code of FP_ARG_WITH_PATH_GNU_PROG to understand what it does before using. And I still don't understand:

  • Why it doesn't use AC_PATH_TARGET_TOOL but forces target prefix if we specified one (--target=...)?
  • Why it checks for $HostOS to decide if we need target-oriented tool? I'm pretty sure that if we'll do cross-compiler host on *nix and targeting windows we do not require readelf. And vice-versa if we do cross on windows for *nix we still need readelf.
bgamari accepted this revision.Oct 22 2015, 8:17 AM
bgamari edited edge metadata.

This looks pretty good to me.

aclocal.m4
2042

We should use autoconf as much as possible if we base our build infrastructure on it.

I must say I'm pretty sympathetic to this view. These macros have certainly added cognitive overhead in my experience.

@austin, is there any reason not to move to AC_CHECK_TARGET_TOOL here? @ony, if not perhaps you'd like to try performing this change on master?

I still think it would be better if readelf were treated in the same way as (ar/nm/ld/ranlib). Then change them all to use AC_CHECK_TARGET_TOOL later if that's better (and create a ticket for it).
If you disagree, then there should be a comment (or Note) explaining the situation.

If this needs to go in quickly, because of the ghc-7.10.3 release, that's also fine. I don't care that much either.

kgardas edited edge metadata.Oct 22 2015, 4:13 PM

Someone adds me as reviewer to this, so I think I will just comment: honestly I do not like the way configuration is handled. I definitely think we should stay with what we have today with --with-<tool>=<tool path>. I see author does not agree with this but such is my feeling that it's better to use kind of common way, otherwise it will be inconvenient for GHC builders/advanced-users just to remember exactly this and this tool needs different way. Anyway, I still think that --with-<tool> also supports authors way of configuration: AR=gar NM=nm ./configure.... -- so I think this more universal...
Otherwise the patch looks OK, at least what I see in it...

erikd edited edge metadata.Oct 24 2015, 5:50 AM

In general, I would advise use of existing autoconf functionality where it fits the bill and where it doesn't implement something specific.

That said, configure.ac/aclocal,m4 hacking is finicky and we should keep changes to small and targeted, even if that means way more commits than one big cleanup.

bgamari requested changes to this revision.Oct 25 2015, 8:02 AM
bgamari edited edge metadata.

@ony, could you move back to FP_ARG_WITH_PATH_GNU_PROG in this patch? @erikd brings up a good point; perhaps we want to move to the autotools macro, but let's keep that change separate from the readelf change.

This revision now requires changes to proceed.Oct 25 2015, 8:02 AM
ony added a comment.Oct 29 2015, 9:35 AM

Please note that there is D1356 to start migrating away from custom cross-compilation toward autoconf.

@ony, could you move back to FP_ARG_WITH_PATH_GNU_PROG in this patch? @erikd brings up a good point; perhaps we want to move to the autotools macro, but let's keep that change separate from the readelf change.

Ok. I'll do this in a few days (currently I'm away from my home computer with Exherbo where this changes are needed).

thomie commandeered this revision.Nov 12 2015, 5:08 AM
thomie edited reviewers, added: ony; removed: thomie.

This patch can be abandoned, because HEAD doesn't use readelf anymore. See 109d7ce85aadbd9fb7a322a0a83548e5d4e83926.

@ony: I realize this whole process, which ultimately didn't result in a commit for GHC HEAD, must have been quite discouraging. So sorry for that. I hope it doesn't stop you from finding another ticket to work on. There are lots of bugs to fix still..

thomie abandoned this revision.Nov 12 2015, 5:08 AM