Opened 4 years ago

Closed 3 years ago

#9878 closed bug (fixed)

Static pointers in GHCi cause panic

Reported by: monoidal Owned by: Phyx-
Priority: normal Milestone: 7.10.3
Component: Compiler Version: 7.9
Keywords: Cc: m@…, alexander.vershilov@…
Operating System: Windows Architecture: Unknown/Multiple
Type of failure: GHCi crash Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D586 Phab:D587 Phab:D1244 Phab:D1310
Wiki Page:

Description

I load this module into ghci

{-# LANGUAGE StaticPointers #-}
module Static where

import GHC.StaticPtr
f = deRefStaticPtr (static True)

and get a panic upon evaluating f

> f
ghc: panic! (the 'impossible' happened)
  (GHC version 7.9.20141210 for x86_64-unknown-linux):
	Loading temp shared object failed: /tmp/ghc30486_0/ghc30486_5.so: undefined symbol: Static_sptEntryZC0_closure

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

Am I doing static pointers right?

Attachments (2)

t9887-hash.c.patch (1.4 KB) - added by qnikst 4 years ago.
fixes key lookup function
track-9887.export-io.patch (1.2 KB) - added by qnikst 4 years ago.
Export IO variant of staticPtrKeys

Download all attachments as: .zip

Change History (32)

comment:1 Changed 4 years ago by simonpj

Owner: set to facundo.dominguez

comment:2 Changed 4 years ago by facundo.dominguez

The current StaticPointers implementation is not intended to be supported in GHCi.

I recognize this is not documented anywhere and this should be addressed. Now, is the ability to use StaticPointers in GHCi essential to you?

comment:3 Changed 4 years ago by facundo.dominguez

Cc: m@… added

comment:4 Changed 4 years ago by simonpj

If it doesn't work in GHCi, then yes it should be documented. And what is "it"? I think it's the static construct itself. Calling deRefStaticPtr is fine.

But also it should be checked! The renamer or typechecker should produce a civilised error message when they encounter static in GHCi.

Simon

comment:5 Changed 4 years ago by monoidal

Support for StaticPointers in GHCi is not essential for me, but I agree we should give a better error message.

comment:6 Changed 4 years ago by facundo.dominguez

For the record, this fails for me when GHCi interprets the code. Compiling the code seems to work.

Prelude> :set -fobject-code
Prelude> :l Static
[1 of 1] Compiling Static           ( Static.hs, Static.o )
Ok, modules loaded: Static.
Prelude Static> f
True

Trying to access GHC.StaticPtr.staticPtrKeys segfaults:

Prelude Static> GHC.StaticPtr.staticPtrKeys
Segmentation fault

But entering GHCi again so the preexisting .o is used allows to use staticPtrKeys:

Prelude> :l Static
Ok, modules loaded: Static.
Prelude Static> f
True
Prelude Static> GHC.StaticPtr.staticPtrKeys
[df8d46d69bf517c3ca5f48d44485169a]

... and if the call to f is omitted staticPtrkeys does not show the pointer:

Prelude> :l Static
Ok, modules loaded: Static.
Prelude Static> GHC.StaticPtr.staticPtrKeys
[]

comment:7 Changed 4 years ago by facundo.dominguez

Cc: alexander.vershilov@… added

comment:8 Changed 4 years ago by qnikst

There are 2 issues here:

  1. That we don't support static pointers in ghci without fobject code. This is because module constructor that we are creating is not called, and because speEntry expressions that float out on desugarer phase is not instantiated by ghci.
  1. Also staticPtrKeys works incorrectly:
  2. it segfaults
  3. it memoizes it's result:
Prelude> :l Static
Ok, modules loaded: Static.
Prelude Static> length GHC.StaticPtr.currentStaticPtrKeys
Prelude Static> length GHC.StaticPtr.staticPtrKeys
staticPtrKeys    <-- debug ouput (function was called)
0
Prelude Static> f99  <-- forcing module loading
True
Prelude Static> length GHC.StaticPtr.staticPtrKeys
0                    <-- no debug output (function was not called)
Prelude Static> fmap length GHC.StaticPtr.currentStaticPtrKeys <-- IO variant
staticPtrKeys        <-- debug output
count: 1025
1025

While I don't know an easy way to workaround 1st issue, I'll attach patches for the 2a and 2b.

Changed 4 years ago by qnikst

Attachment: t9887-hash.c.patch added

fixes key lookup function

Changed 4 years ago by qnikst

Attachment: track-9887.export-io.patch added

Export IO variant of staticPtrKeys

comment:9 Changed 4 years ago by carter

the hashtable has a comment remarking that it isn't thread safe in the context of multiple concurrent updates... is that OK?

comment:10 Changed 4 years ago by facundo.dominguez

I have just discovered that hpc suffers of the same illness reported above:

$ cat t.hs
g :: Int
g = 1 + 1
facundo@fd-tweag:~/tweag/tmp$ ghc-stage2 --interactive t.hs -fhpc
GHCi, version 7.9.20141217: http://www.haskell.org/ghc/  :? for help
[1 of 1] Compiling Main             ( t.hs, interpreted )
Ok, modules loaded: Main.
*Main> g
ghc-stage2: panic! (the 'impossible' happened)
  (GHC version 7.9.20141217 for x86_64-unknown-linux):
	Loading temp shared object failed: /tmp/ghc25774_0/ghc25774_5.so: undefined symbol: _hpc_tickboxes_Main_hpc

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

comment:11 Changed 4 years ago by carter

I'm personally very uncomfortable with GHC features not being supported by GHCI. Any such feature winds up being fundamentally second class. Eg many industrial users of GHC test run their large applications via GHCi to side step long compilations for quickly testing out changes.

a) is actually adding support for GHCI and HPC challenging? (if so whats the challenge? Let us help!) b) or was it simply not done yet? If so, how can we help get this fixed before the 7.10 release

comment:12 Changed 4 years ago by mboes

a) is actually adding support for GHCi and HPC challenging? (if so whats the challenge? Let us help!)

This tells me there might be a slight misinterpretation of Facundo's last comment. The point is, -XStaticPointers uses the exact same strategy as hpc for initializing the static pointer table. Insofar as there -XStaticPointers doesn't work in GHCi, it seems to not work for the same reason that hpc does not work.

I very much suspect that whatever fix to this issue we find will equally apply to hpc. Should we nonetheless still file a separate bug report for the fact that HPC fails in the same way -XStaticPointers does at the GHCi prompt?

comment:13 Changed 4 years ago by facundo.dominguez

My plan to address this issue would be as follows:

  • In the renamer, try to infer from dflags whether ghci is compiling bytecode and reject the static form.
  • Integrate Alexander's patch to fix hs_spt_keys.
  • Document the unavailability of static forms for bytecode.

The API in GHC.StaticPtr is not designed to support dynamic loading. We would need to make lookups and staticPtrKeys monadic. It's doable but it needs green lights from SPJ and Mathieu.

Then we need to decide what to do with the SPT implementation. To make it thread-safe we can use a readers-writer lock implemented with mutexes, or maybe someone can suggest a better alternative.

-- end of plan --

In addition, if you really want to support static pointers when compiling bytecode, then the question is how to link the _stubs.c files produced by StaticPointers which contain external symbols expected to be defined as top-level values in the corresponding modules.

One possibility is to discard the stub file when in GHCi, and have GHCi call hs_spt_insert directly after linking a module.

Another alternative is to modify the stub to get the pointers to the top-level functions of the module in some other way.

I know nothing of how GHCi links the bytecode. Someone else would have to comment if these ideas have any legs.

-- end of comments for StaticPointers --

HPC may need a different solution. The external symbol which appears in stub.c is produced at the C-- level, so it would not have a byte-code incarnation as top-level values do for StaticPointers.

comment:14 Changed 4 years ago by mboes

Then we need to decide what to do with the SPT implementation. To make it thread-safe we can use a readers-writer lock implemented with mutexes, or maybe someone can suggest a better alternative.

-XStaticPointers is limited to the following use case: the universe of static forms that exist in the program is fixed at compile time and constant throughout the lifetime of the program. Using the static keyword inside GHCi does not fit this assumption, nor does dynamic (runtime) code linking. So the more pressing question is, in the original intended use case, are there ever concurrent updates to the SPT? That is, does module initialization happen concurrently at runtime? If not, then let's keep it simple...

comment:15 Changed 4 years ago by mboes

the universe of static forms that exist in the program is fixed at compile time and constant throughout the lifetime of the program.

BTW, the current type for deRefStaticPtr is borne out of this assumption. If this assumption must be dropped, then deRefStaticPtr must be put in the IO monad. That is, its type should become:

deRefStaticPtr :: ... -> IO (StaticPtr a)

I'm not sure this is desirable. As I recall Simon PJ was also keen to keep deRefStaticPtr pure. So this design decision could be revised, but I for one would prefer to see how people really use the extension first.

comment:16 Changed 4 years ago by simonpj

Let's follow Facundo's plan.

Simon

comment:17 Changed 4 years ago by facundo.dominguez

Differential Rev(s): D586 D587
Status: newpatch

comment:18 Changed 4 years ago by thoughtpolice

Differential Rev(s): D586 D587Phab:D586 Phab:D587
Milestone: 7.10.1

comment:19 Changed 4 years ago by Austin Seipp <austin@…>

In 7637810a93441d29bc84bbeeeced0615bbb9d9e4/ghc:

Trac #9878: Have StaticPointers support dynamic loading.

Summary:
A mutex is used to protect the SPT.

unsafeLookupStaticPtr and staticPtrKeys in GHC.StaticPtr are made
monadic.

SPT entries are removed in a destructor function of modules.

Authored-by: Facundo Domínguez <facundo.dominguez@tweag.io>
Authored-by: Alexander Vershilov <alexander.vershilov@tweag.io>

Test Plan: ./validate

Reviewers: austin, simonpj, hvr

Subscribers: carter, thomie, qnikst, mboes

Differential Revision: https://phabricator.haskell.org/D587

GHC Trac Issues: #9878

comment:20 Changed 4 years ago by Austin Seipp <austin@…>

In fffbf0627c2c2ee4bc49f9d26a226b39a066945e/ghc:

Trac #9878: Make the static form illegal in interpreted mode.

Summary:
The entries of the static pointers table are expected to exist as
object code. Thus we have ghci complain with an intelligible error
message if the static form is used in interpreted mode.

It also includes a fix to keysHashTable in Hash.c which could cause a
crash. The iteration of the hashtable internals was incorrect. This
patch has the function keysHashTable imitate the iteration in
freeHashTable.

Finally, we submit here some minor edits to comments and
GHC.StaticPtr.StaticPtrInfo field names.

Authored-by: Alexander Vershilov <alexander.vershilov@tweag.
Authored-by: Facundo Domínguez <facundo.dominguez@tweag.io>

Test Plan: ./validate

Reviewers: simonpj, hvr, austin

Reviewed By: austin

Subscribers: carter, thomie, qnikst, mboes

Differential Revision: https://phabricator.haskell.org/D586

GHC Trac Issues: #9878

comment:21 Changed 4 years ago by thoughtpolice

Resolution: fixed
Status: patchclosed

Merged to ghc-7.10.

comment:22 Changed 3 years ago by thomie

Operating System: Unknown/MultipleWindows
Owner: facundo.dominguez deleted
Resolution: fixed
Status: closednew

This is failing in a scary way on Windows 64 bit.

=====> T9878b(ghci) 138 of 147 [0, 0, 0] 
cd . && HC="D:/home/thomie/ghc-master/inplace/bin/ghc-stage2.exe" HC_OPTS="-dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts -fno-warn-tabs -fno-ghci-history " "D:/home/thomie/ghc-master/inplace/bin/ghc-stage2.exe" -dcore-lint -dcmm-lint -dno-debug-output -no-user-package-db -rtsopts -fno-warn-tabs -fno-ghci-history  --interactive -v0 -ignore-dot-ghci +RTS -I0.1 -RTS -fobject-code   <T9878b.script > T9878b.run.stdout 2> T9878b.run.stderr
Wrong exit code (expected 0 , actual 3 )
Stdout:

Stderr:
<interactive>: internal error: checkProddableBlock: invalid fixup in runtime linker: 0000000000270508
    (GHC version 7.11.20150610 for x86_64_unknown_mingw32)
    Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

*** unexpected failure for T9878b(ghci)

comment:23 Changed 3 years ago by Thomas Miedema <thomasmiedema@…>

In a765f72c130111cdbd30f2a3e159186c6e625d2a/ghc:

Testsuite: mark tests as expect_broken on win64

Tickets: #1407, #9381, #9878.

Differential Revision: https://phabricator.haskell.org/D977

comment:24 Changed 3 years ago by thoughtpolice

Milestone: 7.10.17.10.3

Moving to 7.10.3 as it causes a bug on Windows.

comment:25 Changed 3 years ago by Phyx-

Differential Rev(s): Phab:D586 Phab:D587Phab:D586 Phab:D587 Phab:D1244

Fixed by 620fc6f909cd6e51b5613454097ec1c9f323839a, will remove the expect_broken in review soon.

comment:26 Changed 3 years ago by Phyx-

Differential Rev(s): Phab:D586 Phab:D587 Phab:D1244Phab:D586 Phab:D587 Phab:D1244 Phab:D1310
Status: newpatch

comment:27 Changed 3 years ago by Phyx-

Owner: set to Phyx-

comment:28 Changed 3 years ago by thoughtpolice

Status: patchmerge

comment:29 Changed 3 years ago by Thomas Miedema <thomasmiedema@…>

In 5d84110/ghc:

Add short library names support to Windows linker

Make Linker.hs try asking gcc for lib%s.dll as well, also changed tryGcc
to pass -L to all components by using -B instead. These two fix
shortnames linking on windows.

re-enabled tests: ghcilink003, ghcilink006 and T3333
Added two tests: load_short_name and enabled T1407 on windows.

Reviewed By: thomie, bgamari

Differential Revision: https://phabricator.haskell.org/D1310

GHC Trac Issues: #9878, #1407, #1883, #5289

comment:30 Changed 3 years ago by bgamari

Resolution: fixed
Status: mergeclosed
Note: See TracTickets for help on using tickets.