Opened 13 months ago

Closed 9 months ago

#8781 closed feature request (fixed)

check if GNU nm is really needed and if so let configure detect gnm

Reported by: maeder Owned by: kgardas
Priority: normal Milestone: 7.8.3
Component: Build System Version: 7.8.1-rc1
Keywords: Cc: kgardas
Operating System: Solaris Architecture: x86
Type of failure: Building GHC failed Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

building ghc-7.8.20140130 under Solaris fails with a non-GNU nm as follows:

inplace/bin/deriveConstants --gen-header -o includes/dist-derivedconstants/header/DerivedConstants.h --tmpdir includes/dist-derivedconstants/header/ --gcc-program "/opt/csw/bin/gcc" --gcc-flag -U__i686 --gcc-flag -fno-stack-protector --gcc-flag -Iincludes --gcc-flag -Iincludes/dist --gcc-flag -Iincludes/dist-derivedconstants/header --gcc-flag -Iincludes/dist-ghcconstants/header --gcc-flag -Irts --gcc-flag -fcommon --nm-program "/usr/ccs/bin/nm"
Can't find "STD_HDR_SIZE"
gmake[1]: *** [includes/dist-derivedconstants/header/DerivedConstants.h]

Attachments (9)

log.make2.gz (9.6 KB) - added by maeder 13 months ago.
gmake output
0001-enhance-deriveConstants-to-support-Solaris-nm.patch (5.2 KB) - added by kgardas 13 months ago.
nmp.txt (24.8 KB) - added by maeder 12 months ago.
Solaris "nm -P includes/dist-derivedconstants/header/tmp.o" output
gnmp.txt (24.2 KB) - added by maeder 12 months ago.
Solaris "gnm -P includes/dist-derivedconstants/header/tmp.o" output
mavericks-nmp.txt (19.1 KB) - added by maeder 12 months ago.
Mac "nm -P includes/dist-derivedconstants/header/tmp.o" output
DeriveConstants.patch (2.4 KB) - added by maeder 12 months ago.
different patch (reducing code size)
0001-change-deriveConstants-to-use-nm-in-a-POSIX-way-fixe.patch (3.2 KB) - added by kgardas 11 months ago.
0001-change-deriveConstants-to-use-nm-in-a-POSIX-way-fixe.2.patch (3.8 KB) - added by kgardas 11 months ago.
Now with MinGW supprort included
support-non-GNU-solaris-nm.patch (3.7 KB) - added by maeder 11 months ago.
alternative patch

Download all attachments as: .zip

Change History (52)

Changed 13 months ago by maeder

gmake output

comment:1 Changed 13 months ago by maeder

  • Summary changed from check if GNU nm is realy needed and if so let configure detect gnm to check if GNU nm is really needed and if so let configure detect gnm

comment:2 Changed 13 months ago by kgardas

To fix that it's IMHO a matter of extending deriveConstants to also support POSIX nm output. For example with POSIX nm and '-p' option we get very near to what GNU nm provides, but still there are some differences which should be handled by deriveConstants.

comment:3 Changed 13 months ago by kgardas

  • Owner set to kgardas

comment:4 Changed 13 months ago by kgardas

  • Status changed from new to patch

Attached patch fixes that. It basically enhances deriveConstants utility to also support Solaris' nm program.
Christian please test it. I've tested it here on Solaris 11.1 and Solaris 10 update 8. I'm using PATH which is set to Single Unix Specification 3 standard, that means nm is taken from /usr/xpg4/bin directory

comment:5 Changed 13 months ago by kgardas

BTW: Is this issue a regression? (let's say in comparison with 7.6.x?) If it is, then we may attempt to push it to Austin for inclusion in 7.8.1? If not, then let's wait and ask for merge into HEAD?

comment:6 Changed 13 months ago by maeder

Your nm-patch worked for me, too!

/usr/ccs/bin/nm
-bash-3.2$ nm -V
nm: Software Generation Utilities (SGU) Solaris-ELF (4.0)

comment:7 Changed 12 months ago by maeder

I'm still missing a solution in ghc-7.8-rc2, but my preferred way is simply using gnm (via a link called nm). The provided patch for DeriveConstants.hs works but looks a bit scary.

comment:8 Changed 12 months ago by kgardas

Christian, I've not pushed this patch to Austin for inclusion in 7.8.1 as I've though it's already too late and the patches now in RCx times should be quite safe or fix real regression. Let's see after 7.8.1 is out of door if Austin thinks this may be merged into HEAD or not.
What do you find on this patch scary? It simply uses and parses different output from Solaris nm than it's provided from GNU nm and it also automatically test if its using GNU or Solaris' nm and behaves accordingly. So what's problem with it?

comment:9 Changed 12 months ago by maeder

It's sort of duplicate (but different) code that is difficult to maintain (for various nm versions).
How did the code look like for ghc-7.6.3 that used to work?

comment:11 Changed 12 months ago by maeder

The whole utils/deriveConstants did not exist in ghc-7.6.3.

comment:12 Changed 12 months ago by kgardas

Thanks for investigation. In its light, have you changed your attitude to my proposed patch? I'm afraid that if we're going to support several nm output formats, then either the code looks like you criticize or we need to employ some Haskell library for parsing which I'm not sure if it's not over-kill in this simple case. I'm curious how things work on MacOSX or *BSDs. IIRC On Windows we're using GNU tools and no other commercial UNIX besides Solaris looks supported...

comment:13 Changed 12 months ago by maeder

I think, I've a better patch. Always call "nm -P" (for POSIX.2 standard output). The output looks like

derivedConstantMAX_Vanilla_REG C 0000000b 0000000b

for gnm or like

derivedConstantMAX_Vanilla_REG D        1        b

for Solaris nm.

So just take the first and last word (of 4 words) to extract the name and the size.

Changed 12 months ago by maeder

Solaris "nm -P includes/dist-derivedconstants/header/tmp.o" output

Changed 12 months ago by maeder

Solaris "gnm -P includes/dist-derivedconstants/header/tmp.o" output

Changed 12 months ago by maeder

Mac "nm -P includes/dist-derivedconstants/header/tmp.o" output

comment:14 Changed 12 months ago by maeder

Somehow my comment for OS X got lost. The patches are not yet working on OS X. "nm -V" fails on Mac.
The third word is the size and leading underscores must be ignored. I'll provide a patch checking for "C" in the second word and extracting the size from the third word. (For GNU nm the third and last words are identical.)

Changed 12 months ago by maeder

different patch (reducing code size)

comment:15 Changed 12 months ago by maeder

Is there maybe a problem under windows with "nm -P"? I cannot test it.

comment:16 Changed 12 months ago by kgardas

On Windows IIRC mingw tools are used which means IMHO GNU nm but I'm not able to test that neither...
Are you able to change your patch to git patch? If not, I may try to find the time to do that...

comment:17 Changed 12 months ago by maeder

Please make a git patch and maybe extend the comment for the three formats:

"_derivedConstantMAX_Vanilla_REG C b 0" Mac OS X
"derivedConstantMAX_Vanilla_REG C 0000000b 0000000b" GNU
"derivedConstantMAX_Vanilla_REG D        1        b" Solaris

comment:18 Changed 11 months ago by kgardas

Austin, could you be so kind and merge the "change deriveConstants to use nm in a POSIX way" patch to HEAD? This basically allows us to use Solaris' own nm and not need to rely on GNU nm on this OS. Also it preserves compatibility with GNU nm of course! Thanks! Karel

comment:19 Changed 11 months ago by Austin Seipp <austin@…>

In 045b28033a33a48d31951240a8cb35f2b78345dc/ghc:

change deriveConstants to use nm in a POSIX way (fixes #8781)

The patch provided by Christian Maeder <[email protected]>

Signed-off-by: Karel Gardas <[email protected]>
Signed-off-by: Austin Seipp <[email protected]>

comment:20 Changed 11 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from patch to closed

Merged, thanks!

comment:21 Changed 11 months ago by Austin Seipp <austin@…>

In 15b1eb7c67e29c4ad6f6859f89d220b33493fd46/ghc:

Revert "change deriveConstants to use nm in a POSIX way (fixes #8781)"

It causes a failure on Windows right now.

This reverts commit 045b28033a33a48d31951240a8cb35f2b78345dc.

comment:22 Changed 11 months ago by thoughtpolice

  • Owner kgardas deleted
  • Resolution fixed deleted
  • Status changed from closed to new

comment:23 Changed 11 months ago by kgardas

  • Owner set to kgardas

comment:24 Changed 11 months ago by maeder

It would be interesting to see the failure. Is the additional "-P" option the problem or is there yet another output format produced for:

nm -P includes/dist-derivedconstants/header/tmp.o

comment:25 Changed 11 months ago by kgardas

It's all discussed on ghc-devs@, I've also provided a fix for this for windows guys testing. The output on windows from gnu nm is three words list:

_derivedConstantCINT_SIZE C 00000005

Changed 11 months ago by kgardas

Now with MinGW supprort included

comment:26 Changed 11 months ago by kgardas

  • Status changed from new to patch

comment:27 Changed 11 months ago by maeder

Sorry, I don't like this patch as it contains duplicate code. Let's improve it for ghc-7.8.2 (whatever ends up in ghc-7.8.1 now)

comment:28 Changed 11 months ago by maeder

I propose the code as follows:

          parseNmLine xs0 = case words xs0 of                                                                 
                            x0 : x1 : x2 : r -> case stripPrefix prefix $ dropWhile (== '_') x0 of            
                              Just name -> case readHex $ case r of                                           
                                  [x3] | x1 /= "C" -> x3                                                      
                                  _ -> x2 of                                                                  
                                [(size, "")] -> Just (name, size)                                             
                                _ -> Nothing                                                                  
                              _ -> Nothing                                                                    
                            _ -> Nothing

comment:29 Changed 11 months ago by maeder

Well, it could be much less indented, too, after the first line. If you think, that the second is too long break it after "->" or use single letter names for x0, x1 and x2.

comment:30 follow-up: Changed 11 months ago by jberthold

Maybe repetition is not good, but let me suggest to clarify which part is responsible for which variant of nm output. Also, one could use the Maybe monad instead of nested case expressions.
how about this one:

parseNmLine xs0 = case words xs0 of
    -- "_derivedConstantMAX_Vanilla_REG C b 0" Mac OS X
    -- "derivedConstantMAX_Vanilla_REG C 0000000b 0000000b" GNU
    -- "_derivedConstantMAX_Vanilla_REG C 000000b" MinGW 
                    x0 : "C" : x2 : r       -> mkMapping x0 x2
    -- "derivedConstantMAX_Vanilla_REG D        1        b" Solaris
                    x0 :  _  : x2 : x3 : [] -> mkMapping x0 x3
                    other -> Nothing

mkMapping x0 x2 = do name <- stripPrefix $ dropWhile (== '_') x0
                     size <- case readHex x2 of [(n,"")] -> return n
                                                _        -> fail "not hex"
                     return (name, size)
Version 0, edited 11 months ago by jberthold (next)

comment:31 Changed 11 months ago by maeder

Yes, also not bad. The unused "r" in the first case should be a "_". The pattern
"x0 : _ : x2 : x3 : []" should be "[x0, _, x2, x3]".

Following stripPrefix the actual "prefix" is missing.

The actual cases for "case ... of" should be (less indented) on separate lines in order to avoid re-intenting lines when changing (ie. names) in "size <- case readHex x2 of".

Also after "do" a line break should follow (for the same reason as after "of")
(If the do-notation based on the monad instance is really an improvement is a matter of taste.)

The comments clutter the code too much (in my eyes, but that's again a matter of taste)

One advantage over my code is, that "C" is matched by a pattern (rather than using the Eq instance).

After this style discussion I suggest, that Karel should create an improved patch and Austin decides to apply it.

comment:32 in reply to: ↑ 30 Changed 11 months ago by kgardas

Replying to jberthold:

[...]

-- "derivedConstantMAX_Vanilla_REG D 1 b" Solaris

x0 : _ : x2 : x3 : [] -> mkMapping x0 x3

[...]

}}}

IMHO there should be "D" used in the code, since otherwise I'm not sure this will work correctly on Solaris and not match anything else.

x0 :  'D'  : x2 : x3 : [] -> mkMapping x0 x3

otherwise also nice!

comment:33 Changed 11 months ago by maeder

No, checking if it is not a 'C' is enough and ensure by the first case.
(Surely, one could apply stricter tests, but I think that's unnecessary as no error handling is done anyway.)

comment:34 Changed 11 months ago by kgardas

No, it's not! Especially if we're doing all those exercises to make this piece of code better. The problem with

x0 : _ : x2 : x3 : [] -> mkMapping x0 x3

is that it matches

tmp.c      f        0        0

line which is the last line in Solaris's nm -P tmp.o output. So you match Solaris case and pass it down to mkMapping where stripPrefix will produce Nothing and you ends with (Nothing, 0) pair being returned. IMHO not good. So if you'd like to make this piece of code nice, also please let's make it correct.

comment:35 follow-up: Changed 11 months ago by maeder

No, no pair is returned by simply Nothing that is ignored later.

comment:36 in reply to: ↑ 35 Changed 11 months ago by kgardas

Replying to maeder:

No, no pair is returned by simply Nothing that is ignored later.

Sorry, I don't understand. I still see that four members list is matched and mkMapping is invoked with x0 and x3 as its params. Isn't that true? So if I'm not mistaken then mkMapping is invoked for:

tmp.c      f        0        0

which is IMHO not good. Or is it? Please correct me if I'm wrong. Thanks.

comment:37 Changed 11 months ago by maeder

Yes, mkMapping is invoked and returns Nothing, that is ignored by catMaybes in line 635.

comment:38 Changed 11 months ago by maeder

However, matching for the 'D' is no bad idea, as it yields Nothing earlier.

Changed 11 months ago by maeder

alternative patch

comment:39 Changed 11 months ago by maeder

My patch should be merged to the ghc-7.8 branch, too. (The file DeriveConstants.hs has a further non-conflicting change in HEAD.) I cannot test it under windows.

comment:40 Changed 11 months ago by maeder

Just for the record. Under 64bit windows (as described wiki:Building/Preparation/Windows/MSYS2) I get the "nm -p" output:

derivedConstantMAX_Vanilla_REG C 000000000000000b

(without leading underscore)

comment:41 Changed 11 months ago by maeder

  • Milestone set to 7.8.3

comment:42 Changed 10 months ago by thoughtpolice

  • Status changed from patch to merge

comment:43 Changed 9 months ago by thoughtpolice

  • Resolution set to fixed
  • Status changed from merge to closed

This should all be merged, thanks!

Note: See TracTickets for help on using tickets.