Opened 9 years ago

Closed 8 years ago

#2969 closed bug (fixed)

unix package built wrong on Solaris

Reported by: duncan Owned by: igloo
Priority: high Milestone: 6.12.1
Component: Compiler Version: 6.8.3
Keywords: Cc:
Operating System: Solaris Architecture: sparc
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

On Solaris with recent gcc versions the unix package gets built with an inconsistent state of system header files which results in getSymbolicLinkStatus not working. In turn this breaks darcs.

The problem is that the HsUnix.h file contains a bunch of inline functions. These get inlined into the .hc file for the Files.hs module that FFI imports them. The problem is that this .hc file #includes an RTS header files which uses some #define to get certain API spec compatibility and for some reason this causes the lstat() call to be directed to the 32bit file ABI whereas the .hsc file was compiled for the 64bit (large file support) ABI. The outcome is that the .o file for the System.Posix.Files module is actually calling the lstat() function when it expects to be calling the lstat64() function. As a result we get back the wrong struct stat and so of course all the functions like isDirectory etc are then looking at the wrong offsets in the struct stat.

The functions that are being FFI imported are compiled into the cbits/HsUnix.c file anyway (because it #imports HsUnix.h with a #define to turn off the inlining) so there is no need for them to be inlined into the .hc file. The cbits/HsUnix.c gets built correctly (because it does not #include the RTS headers that cause the problem) and so does call lstat64 rather than lstat(). This can be verified with nm.

So the solution is to remove all the inline calls from HsUnix.h, changing them to simple prototypes, and to put the function bodies in HsUnix.c. This solution works fine. Tested on Solaris 10, ghc-6.8.3. The files appear to be unchanged in ghc 6.10 and HEAD so the same solution should work there.

We should add a test case for this. It should test the getSymbolicLinkStatus function with a few things that obviously should work, like isDirectory etc. We might as well test getFileStatus too. The difference is that the FFI wrapper for getFileStatus is defined in the base package rather than the unix package. This is why it was unaffected by the above problem.

Generally all the FFI wrappers defined via inlines in .h files that get #included into .hc files are rather suspect, but especially so for wrappers of things defined in system header files. These should instead be wrapped in very simple .c files that #include a minimum of headers so as to not bump into weird untested combinations of system headers and standards compatibility #defines.

Change History (6)

comment:1 Changed 9 years ago by igloo

difficulty: Unknown
Milestone: 6.10.2
Priority: normalhigh

comment:2 Changed 9 years ago by igloo

Owner: set to duncan

This should be fixed now:

Wed Feb 11 18:29:06 GMT 2009  Ian Lynagh <igloo@earth.li>
  * Don't put inline'd functions in HsUnix.h; fixes trac #2969
  If they are included into a C file which also has certain symbols
  defined, then the behaviour of the HsUnix.h functions can change
  (e.g. lstat can become the 32bit, rather than 64bit, version).

Duncan, as you have a machine it failed on, is there any chance you could add a test for this please?

comment:3 Changed 9 years ago by igloo

Milestone: 6.10.26.12.1

comment:4 Changed 8 years ago by duncan

Seems to be working. I tested in ghc-6.10.4 which also includes the above patch.

To make sure it keeps working on other ports it'd be good to add a trivial test of getFileStatus and getSymbolicLinkStatus.

For example something along the lines of:

import System.Posix.Files
import System.Posix.Directory
import System.Posix.IO
import Control.Monad (when)

main = do
  fs <- testRegular
  ds <- testDir
  testSymlink fs ds
  cleanup

testRegular = do
  createFile "regular" ownerReadMode
  (fs, _) <- getStatus "regular"
  let expected = (False,False,False,True,False,False,False)
      actual   = snd (statusElements fs)
  when (actual /= expected) $
    fail "unexpected file ststus bits for regular file"
  return fs
  
testDir = do
  createDirectory "dir" ownerReadMode
  (ds, _) <- getStatus "dir"
  let expected = (False,False,False,False,True,False,False)
      actual   = snd (statusElements ds)
  when (actual /= expected) $
    fail "unexpected file ststus bits for directory"
  return ds

testSymlink fs ds = do
  createSymbolicLink "regular" "link-regular"
  createSymbolicLink "dir"     "link-dir"
  (fs', ls)  <- getStatus "link-regular"
  (ds', lds) <- getStatus "link-dir"

  let expected = (False,False,False,False,False,True,False)
      actualF  = snd (statusElements ls)
      actualD  = snd (statusElements lds)

  when (actualF /= expected) $
    fail "unexpected file ststus bits for symlink to regular file"

  when (actualD /= expected) $
    fail "unexpected file ststus bits for symlink to directory"
  
  when (statusElements fs /= statusElements fs') $
    fail "status for a file does not match when it's accessed via a symlink"
  
  when (statusElements ds /= statusElements ds') $
    fail "status for a directory does not match when it's accessed via a symlink"
 
cleanup = do
  removeDirectory "dir"
  mapM_ removeLink ["regular", "link-regular", "link-dir"]

getStatus f = do
  fs  <- getFileStatus f
  ls  <- getSymbolicLinkStatus f
  
  fd  <- openFd f ReadOnly Nothing defaultFileFlags
  fs' <- getFdStatus fd
  
  when (statusElements fs /= statusElements fs') $
    fail "getFileStatus and getFdStatus give inconsistent results"
  
  when (not (isSymbolicLink ls) && statusElements fs /= statusElements fs') $
    fail $ "getFileStatus and getSymbolicLinkStatus give inconsistent results "
        ++ "on a file that is not a symbolic link"

  return (fs, ls)

-- Yay for 17-element tuples!
statusElements fs = (,)
  (deviceID fs
  ,fileMode fs
  ,linkCount fs
  ,fileOwner fs
  ,fileGroup fs
  ,specialDeviceID fs
  ,fileSize fs
  ,accessTime fs
  ,modificationTime fs
  ,statusChangeTime fs
  )
  (isBlockDevice fs
  ,isCharacterDevice fs
  ,isNamedPipe fs
  ,isRegularFile fs
  ,isDirectory fs
  ,isSymbolicLink fs
  ,isSocket fs
  )

This test works on Linux and Solaris with ghc-6.10.4.

comment:5 Changed 8 years ago by igloo

Owner: changed from duncan to igloo

comment:6 Changed 8 years ago by igloo

Resolution: fixed
Status: newclosed

Test added to the unix package, thanks!

Note: See TracTickets for help on using tickets.