Opened 4 months ago

Last modified 3 months ago

#14090 patch bug

Static pointers are not being registered under certain conditions

Reported by: mnislaih Owned by:
Priority: normal Milestone:
Component: Compiler Version: 8.2.1
Keywords: Cc: facundo.dominguez, mboes
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s): Phab:D3843 Phab:D3933
Wiki Page:

Description (last modified by mnislaih)

It seems that only the static pointers in the module export list survive the Simplifier. This is a regression which doesn't seem to affect ghci/runghc only compiled code.

{-# LANGUAGE StaticPointers #-}

import GHC.StaticPtr

staticHello :: StaticPtr String
staticHello = static "hello"

main = do
  keys <- staticPtrKeys
  if null keys
    then error "static ptrs are not being registered"
    else putStrLn "Everything is fine"
pepe:~/scratch$ stack --resolver ghc-8.2.1 script --optimize bug-spt.hs  
Using resolver: ghc-8.2.1 specified on command line
bug-spt: static ptrs are not being registered
CallStack (from HasCallStack):
  error, called at bug-spt.hs:11:10 in main:Main
pepe:~/scratch$ stack --resolver ghc-8.0.2 script --optimize bug-spt.hs  
Using resolver: ghc-8.0.2 specified on command line
Everything is fine

Change History (16)

comment:1 Changed 4 months ago by mnislaih

Description: modified (diff)

comment:2 Changed 4 months ago by mboes

This is a serious regression, breaking most applications that use -XStaticPointers. Facundo, could you have a look?

comment:3 Changed 4 months ago by facundo.dominguez

The staticHello binding is dropped during desugaring. More specifically, during a call to

        ; (ds_binds, ds_rules_for_imps, ds_vects)
            <- simpleOptPgm dflags mod final_pgm rules_for_imps vects0
                         -- The simpleOptPgm gets rid of type
                         -- bindings plus any stupid dead code

I'm new to this code, so any insights are welcome.

Last edited 4 months ago by facundo.dominguez (previous) (diff)

comment:4 Changed 4 months ago by facundo.dominguez

Surely we could modify the usage analysis so code containing static forms is not considered dead. But I also wonder if deleting dead code this early is a good idea. If the user wants to optimize early compilation, could he remove the code himself?

Last edited 4 months ago by facundo.dominguez (previous) (diff)

comment:5 Changed 4 months ago by facundo.dominguez

Differential Rev(s): Phab:D3843
Status: newpatch

comment:6 Changed 4 months ago by facundo.dominguez

I submitted a patch that makes the bindings containing static forms exported. Please, take a look.

comment:7 Changed 4 months ago by bgamari

But I also wonder if deleting dead code this early is a good idea.

I don't see why not. The less code we hand to the simplifier the better.

I submitted a patch that makes the bindings containing static forms exported. Please, take a look.

I don't think Phab:D3843 is the right solution here. Not only does it require that we sweep the entire Core program looking for statics (and consequently fairly expensive), but it's quite indirect.

Teaching the occurrence analysis about statics sounds somewhat sensible; afterall, static forms essentially allow the user to refer to static subexpression from any context, regardless of scoping. Declaring such a subexpressions as dead is obviously wrong. One way to teach the analyser this would be to add a "contains static form" flag to UsageDetails. Then when analysing a binding check the flag of the analysis result of the RHS; if set, markMany the binding.

comment:8 Changed 4 months ago by facundo.dominguez

Looks fine to me. Would be good to know if anyone objects before going through the trouble of implementing it.

comment:9 Changed 4 months ago by bgamari

In that case it would be good to get Simon's opinion before proceeding.

Simon, what do you think about comment:7?

comment:10 Changed 4 months ago by simonpj

Why do you say

This is a serious regression

After all, if no one mentions a static expression then it's dead isn't it?

I didn't even know about staticPtrKeys! The user manual section does not mention it. Where are all the functions available on static pointers defined? Facundo has put a lot of work into static pointers, and I'm pretty sure that some of it is not reflected in this section. E.g. the user manual doesn't even mention the type StaticKey.

This is worth fixing! It's not a bug if the specification (i.e. the user manual) doesn't claim it is so.

Returning to the current topic, suppose we have

foo = ...lots of code...(static reverse)....lots more code...

do you really want that (static reverse) to be accessible to staticPtrKeys? Why? How would you use it?

I'm a bit stumped.

comment:11 Changed 4 months ago by facundo.dominguez

When we moved inline-java to use a ghc plugin, we stopped using the static pointer table. But it would have broken because of this.

inline-java was using the static pointer table to store java bytecode. At runtime, the SPT was traversed looking for the bytecode to pass it to the JVM. The bytecode was inserted in the SPT by creating some static forms which were never accessed other than through the SPT traversal. A traversal consists in obtaining the keys with staticPtrKeys, then using unsafeLookupStaticPtr to get the value of each, then doing something hacky to determine if the returned thunk is some bytecode. We recognized the bytecode because we stored it together with some magic number that we could recover from the thunk using GHC.Prim.unpackClosure#.

But as I said, inline-java doesn't use the SPT anymore. Maybe mnislaih has another use case.

The documentation for staticPtrKeys is available in the module GHC.StaticPtr. https://www.stackage.org/haddock/lts-9.0/base-4.9.1.0/GHC-StaticPtr.html

We could say more in the user guide, of course.

Last edited 4 months ago by facundo.dominguez (previous) (diff)

comment:12 Changed 4 months ago by mnislaih

I maintain the clr-inline package, which follows in the footsteps of inline-java for the .Net platform and is indeed broken by this change. The static pointer table is used to register bytecode objects, which are then loaded lazily at initialisation time. Since staticPtrKeys was mentioned in the module haddocks, I assumed it was stable api.

comment:13 Changed 4 months ago by simonpj

inline-java was using the static pointer table to store java bytecode.

Crumbs! It was never designed for that purpose! It's like using a hammer to mow the lawn: a tool for one job used for another. The trouble is that, becuase it wasn't designed for the purpose, it keeps failing to serve that purpose. The danger is that, to keep serving the accidental application, the original concept gets muddied.

Let's stand back. If we want to register bytecode objects (I'm terribly hazy about what that even means) what is a general, re-usable mechanism that GHC might provide to make that (and perhaps other things) possible?

You say you aren't using the SPT for this purpose any more. What are you using? Does it nicely serve the purpose? Could mnislaih use it too?

comment:14 Changed 4 months ago by facundo.dominguez

I know of three ways to accomplish it in ghc-8.2.1 without the SPT.

  1. Use addForeignFile and addTopDecls to make the bytecode available via a foreign import. https://gist.github.com/facundominguez/82f6768e1f4d5fbfecf008115226a484
  1. Use addForeignFile to insert a constructor function in the module to initialize a global bytecode table when the module is loaded. https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-constructor-function-attribute
  1. Like (2), but instead of using addForeignFile to insert the constructor function, use a GHC plugin to inject it. This is the approach that inline-java uses in the latest version in the repo. https://github.com/tweag/inline-java/blob/6c4aa0e94a4952ce91a498d2b64198a5e158ee57/src/Language/Java/Inline/Plugin.hs#L73

(3) works with 8.0.2 as well.

comment:15 Changed 4 months ago by simonpj

OK. But for now I'm arguing not to adopt Phab:D3843 because, as I argue in comment:13, it smells to much like piling a sticking plaster on top of a sticking plaster. If there's a need, let's stand back and address it.

Meanwhile,

  • Documenting what we have would be extremely helpful.
  • Perhaps we should add a note to staticPtrKeys to say that it's an experimental/unstable feature.

comment:16 Changed 3 months ago by facundo.dominguez

Differential Rev(s): Phab:D3843Phab:D3843 Phab:D3933

I documented better staticPtrKeys in Phab:D3933. Let me know if more documentation is needed.

Last edited 3 months ago by facundo.dominguez (previous) (diff)
Note: See TracTickets for help on using tickets.