Interface hashes include time stamp of dependent files (UsageFile mtime)

mentioned in issue #4900

changed weight to 5

Trac metadata

Trac field	Value
CC	→ tkn.akio@gmail.com

(This happens for me with ghc-7.6.3, ghc-7.4.1 and some version of GHC 7.7.)

That certainly sounds wrong. Would anyone care to investigate?

Simon

http://www.haskell.org/ghc/docs/7.6.3/html/libraries/ghc/HscTypes.html#t:Usage see

UsageFile { usg_file_path :: FilePath
          , usg_mtime     :: UTCTime }

https://github.com/ghc/ghc/blame/9082111dc/compiler/iface/MkIface.lhs#L603

iface_hash <- computeFingerprint ...

where computeFingerprint just uses the instance Binary Usage, so the mtime goes into the fingerprint.

UsageFile was introduced with addDependentFile in https://github.com/ghc/ghc/commit/b994313.

The modification time being only (obtained) in second resolution is what triggers the inconsistent behaviour.

Regarding that commit, I am also not sure how https://github.com/ghc/ghc/commit/b994313#L3R1283 works:

checkModUsage _this_pkg UsageFile{ usg_file_path = file, usg_mtime = old_mtime } = do
  new_mtime <- liftIO $ getModificationTime file
  return $ old_mtime /= new_mtime

I might be wrong, but it looks to me as if this can only ever return False (UpToDate in current GHC head) because the mtime is so coarse - in other words if your computer is too slow, a second will pass and this will never be considered up to date?

Looks like #7473 (closed) is related.

changed title from Interface hashes include time stamp of included .h file when CPP is used to Interface hashes include time stamp of dependent files (UsageFile mtime)

Replying to [ticket:8144#comment:73245 refold]:

Looks like #7473 (closed) is related.

According to discussion in #7473 (closed) getModificationTime was improved on Linux to return time with resolution better than 1 second. nh2, what platform are you on?

Replying to [ticket:8144#comment:73256 jstolarek]:

nh2, what platform are you on?

Linux on all the GHCs I mentioned above.

I wrote a patch:

https://github.com/nh2/ghc/compare/fix-interface-hashes-mtime https://github.com/nh2/packages-base/compare/fix-interface-hashes-mtime

It contains some preparatory cleanup commits, the main change is at https://github.com/nh2/ghc/commit/e368816.

This fixes my test case.

assigned to @nh2

Looks good to me. Making iface hashes more deterministic is definitely the way to go.

mentioned in commit 677820ee

This is now fixed but I have not yet added a test, so I will keep it open. @nh2, can you confirm this fixes your bug? Perhaps include a patch for a simpler test if possible :)

mentioned in commit b6a572b4

mentioned in issue #8159

There is small bug in the replacement code used for the stage 1 compiler: The input file is never read :P

(Doesn't become obvious until you try to backport my change to 7.6.)

Please pull:

https://github.com/nh2/ghc/compare/fix-interface-hashes-mtime

Thanks, I've merged and I'm validating now.

Merged

commit 95ebff9aed3e8570c7ac51148594c48a812c5b19
Author: Austin Seipp <aseipp@pobox.com>
Date:   Fri Aug 23 09:40:59 2013 -0500

    Fix validation failure in Fingerprint.hsc
    
    Signed-off-by: Austin Seipp <aseipp@pobox.com>

commit 41be8d30356d050938f61f06c56928d2c3eb2541
Author: Niklas Hambüchen <mail@nh2.me>
Date:   Fri Aug 23 21:46:14 2013 +0900

    Fingerprint.getFileHash: Fix not reading file at all.
    
    This lead to the stage1 compiler calculating random iface hashes.
    
    Signed-off-by: Austin Seipp <aseipp@pobox.com>

commit 48d7b0e963e88101464136419705e3c9e2e947f9
Author: Niklas Hambüchen <mail@nh2.me>
Date:   Fri Aug 23 21:46:02 2013 +0900

    Fingerprint: Fix comment typo
    
    Signed-off-by: Austin Seipp <aseipp@pobox.com>

A backport to 7.6 can be found at https://github.com/nh2/ghc/releases/tag/fix-interface-hashes-mtime-7.6.3-release-tarball-tag (based on 7.6.3 release tarball, 64 bit Linux ghc binary also provided).

closed

Trac metadata

Trac field	Value
Resolution	Unresolved → ResolvedFixed

reopened

unassigned @nh2

nh2: Would it be possible to get a test case on this one? (Or is there one already?)

Trac metadata

Trac field	Value
Resolution	ResolvedFixed → Unresolved

Replying to [ticket:8144#comment:73842 ezyang]:

nh2: Would it be possible to get a test case on this one? (Or is there one already?)

I'll make one eventually (that means hopefully this week).

Should this be closed or is a test case still pending?

Still pending.

I added a test case at https://github.com/nh2/ghc/compare/testcase-8144. Please pull.

Test LGTM.

changed milestone to %7.10.1

Niklas, can you please rebase this patch? I manually did it, but then the test started failing (the .stdout file had "compilation IS NOT required" once too many times). Can you please take this and verify it should work? Thanks!

Here's a WIP patch: https://github.com/ghc/ghc/commit/a3bf4bfae694572b0bf886fcb1191d066d242aed on wip/8144

removed milestone

Moving to 7.12.1 milestone; if you feel this is an error and should be addressed sooner, please move it back to the 7.10.1 milestone.

What is the status of this? Stable interfaces are important for distributions like Debian!

changed weight to 7

Also, from reading the patch, it is not clear to me if the filename is also part of the ABI hash. It should not be, as build directory names may change, e.g. on automatic build machines.

Also see http://bugs.debian.org/785282

Trac metadata

Trac field	Value
Priority	normal → high

changed weight to 5

Ah, wait: This is only waiting for a test case, right?

So then why do I still see this

./usr/lib/ghc/base-4.7.0.2/System/Info.hi
--- /dev/fd/63  2015-05-14 10:15:20.278404032 +0200
+++ /dev/fd/62  2015-05-14 10:15:20.278404032 +0200
@@ -5,7 +5,7 @@
 Way: Wanted [],
      got    []
 interface base:System.Info 7084
-  interface hash: 038b3a2a32cd43c6a82b096e3ef5c665
+  interface hash: 4ff1db7940716ca0e906d3f6b7af1166
   ABI hash: 67cae6f933216805d975ee28cd5ea72d
   export-list hash: 89e8b7a3bfc33bdb505eb586ca9fe64e
   orphan hash: 693e9af84d3dfcc71e640e005bdc5e2e
@@ -64,7 +64,7 @@
   divMod e11d60a38cd49de2f6058cc39227517a
 import safe Prelude 74043f272d60acec1777d3461cfe5ef4
   exports: be888345b248b32ace8f805dd49f3def
-addDependentFile "/build/ghc-LIQtDg/ghc-7.8.4/includes/ghcplatform.h"
+addDependentFile "/build/ghc-Na5lKv/ghc-7.8.4/includes/ghcplatform.h"
 addDependentFile "libraries/base/dist-install/build/autogen/cabal_macros.h"
 addDependentFile "/usr/include/stdc-predef.h"
 f85496685a82c42827c38c1ecec0b5fd

Looks like the filename is included in the hash then, because the files are identical in both builds...

Trac metadata

Trac field	Value
Priority	high → normal

nomeata, sorry if I am missing something obvious, but this ticket seems to be about the time stamp not appearing in the interface hash, so the fact that this ticket is just waiting for a test is not incompatible with the interface hash including the file name.

Currently we need the .h file name in the interface file so that we can check whether the file fingerprint matches, and we use the interface hash to determine whether to write a new interface file, so it's not immediately obvious that we can avoid putting the file name in the interface hash. Maybe you'd like to create a new ticket about finding some way to avoid this?

and we use the interface hash to determine whether to write a new interface file,

Ah, this is new to me. I assumed that when a module is compiled, its interface is written, and the interface hash is only used to decide whether to rebuild another module or not.

But you are right, this deserves a separate ticket.

changed milestone to %8.0.1

Milestone renamed

added testsuite label

Bug is fixed. Just needs a test.

Trac metadata

Trac field	Value
Component	Compiler → Test Suite

added compiler perf label

added Pnormal label

mentioned in merge request !5150 (closed)

mentioned in commit 20fb11cb

closed with commit 1b9df111

@hdgarrood Thank you, much appreciated!

mentioned in commit 1b9df111

Interface hashes include time stamp of dependent files (UsageFile mtime)

Child items 0

Activity