Opened 4 years ago

Last modified 10 months ago

#11526 new bug

unsafeLookupStaticPtr should not live in IO

Reported by: edsko Owned by:
Priority: normal Milestone: 8.10.1
Component: Core Libraries Version: 8.0.1-rc1
Keywords: Cc: facundo.dominguez, mboes, ekmett
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

GHC.StaticPtr provides

unsafeLookupStaticPtr :: StaticKey -> IO (Maybe (StaticPtr a))

I don't think this function should live in IO. It is morally pure (the SPT that it needs to access is static for the duration of the program after all) and when this lives in IO this function cannot be used in Binary instances (without using unsafePerformIO, obviously).

Change History (17)

comment:1 Changed 4 years ago by bgamari

Cc: facundo.dominguez mboes ekmett added
Component: CompilerCore Libraries
Milestone: 8.0.1

If we want to make this change let's try to get it in to 8.0 before static pointers go into wide use.

comment:2 Changed 4 years ago by facundo.dominguez

unsafeLookupStaticPtr is not pure since the RTS can load code dynamically. Calling the function at different times of execution can yield different results.

comment:3 Changed 4 years ago by edsko

That kind of violates the whole idea of a _static_ reference to something surely?

comment:4 Changed 4 years ago by facundo.dominguez

unsafeLookupStaticPtr could be made pure if the program is forbidden from loading code dynamically. That probably will have all code depending indirectly on static pointers to work awkwardly in GHCi.

comment:5 Changed 4 years ago by simonpj

Hmm. I'm deeply suspicious of StaticKey. It plays no role in our plan of record StaticPointers. If we don't have StaticKey the whole issue doesn't arise.

I'd love to have a proper debate about StaticPointers, based on successful execution of #11011. Would anyone like to lead on it?

Meanwhile, I doubt we should mess with unsafeLookupStaticPtr, since other changes are in train.

If we want to make this change let's try to get it in to 8.0 before static pointers go into wide use.

I think StaticPtr is in 7.10, isn't it?

comment:6 Changed 4 years ago by edsko

Hmm. I'm deeply suspicious of StaticKey . It plays no role in our plan of record StaticPointers. If we don't have StaticKey the whole issue doesn't arise.

StaticKey is a low-level implementation detail. I have just added support for static pointers to the existing distributed-static package, based on the support for static pointers that exists in ghc today. The higher level API that distributed-static exposes is simply that Static a has a Binary instance (for Typeable a). But this is precisely where the problem is: if looking up a static pointer is not pure, then we cannot give a Binary instance.

Facundo's objection is that in fact looking up a static pointer isn't pure. If that is true, then unsafeLookupStaticPtr should indeed live in IO, but then not having StaticKey is not going to solve the problem, it's just going to move it: whatever shape the decoder of Static takes (Binary instance, separate function, ...) will have to deal with it.

comment:7 Changed 4 years ago by simonpj

I agree that the problem would just move.

I don't know what to do about the dynamic-linking thing. It looks as though whether you can deserialise a ByteString depends on when you try to deserialise it, and whether some blob of code has been dynamically linked. Ugh! Any suggestions?

comment:8 Changed 4 years ago by mboes

Replicating here for the record my earlier email, which from my mobile phone originally went to ghc-dev@ instead:

IMHO this dynamic linking thing is a red herring. In principle any function call becomes potentially impure given eg the ability to shadow symbols. In practice that's not something we do to programs. We could consider having two tables though: an initial tackle generated at compile time that will never change, and an aux table populated as new objects get linked in. Then programs can "opt in" to taking into account new objects loaded dynamically by reading (in IO) the aux table.

A followup -

I think a reasonable compromise here is simply to have:

-- | Lookup in a static table constructed at compile time.
unsafeLookupStaticPtr :: StaticKey -> Maybe (StaticPtr a)

-- | Lookup in the static table + an aux table that's initially empty but may grow if objects are dynamically loaded.
unsafeLookupStaticPtrDL :: StaticKey -> IO (Maybe (StaticPtr a))

The idea is: bake in a static table at compile time that is immutable for all time; any new static pointers that get loaded at runtime (i.e. are not reachable from main at the start of the program) go into a separate table, which can only be accessed via unsafeLookupStaticPtrDL. Most programs will never use unsafeLookupStaticPtrDL.

That way it becomes possible to define a Binary instance, but also address Facundo's concerns about dynamically loaded objects.

I don't anticipate anyone will have time to implement the *DL stuff ahead of GHC 8.0, but that doesn't prevent us from moving unsafeLookupStaticPtr out of IO now. For the the DL table to be useful we'd have to populate it upon object load somehow, likely via so-called "constructor functions". Maybe someone with that use case wants to give it a crack (in a separate ticket).

comment:9 Changed 4 years ago by facundo.dominguez

Currently all static names in a statically linked module are inserted in the SPT before the program starts executing main. When the module is loaded dynamically, the static names are inserted at load time.

Making unsafeLookupStaticPtr pure will require changing the implementation so linking modules dynamically do not modify the SPT. It will also require doing something so the table is available when the function is used in (code loaded in) GHCi, unless nobody cares of breaking that.

comment:10 in reply to:  8 Changed 4 years ago by ekmett

Replying to mboes:

I think a reasonable compromise here is simply to have:

-- | Lookup in a static table constructed at compile time.
unsafeLookupStaticPtr :: StaticKey -> Maybe (StaticPtr a)

-- | Lookup in the static table + an aux table that's initially empty but may grow if objects are dynamically loaded.
unsafeLookupStaticPtrDL :: StaticKey -> IO (Maybe (StaticPtr a))

This is the approach we take in the OpenGLRaw package for detecting extensions. Yes, technically you might switch OpenGL contexts and change which ones are available over the life of a program, it isn't the common case, while branching on them is very common and is made nigh unusable, and needlessly slow, since they can't memoize this information if they are stuck in IO.

One of hundreds of such examples:

http://hackage.haskell.org/package/OpenGLRaw-3.1.0.0/docs/Graphics-GL-ARB-CullDistance.html

comment:11 in reply to:  5 Changed 4 years ago by mboes

Replying to simonpj:

Hmm. I'm deeply suspicious of StaticKey. It plays no role in our plan of record StaticPointers.

FTR the reason we have StaticKey (i.e. just Fingerprint, nothing new) is to avoid hardcoding encoding/decoding of static pointers in the compiler. Encoding/decoding is binary's business, or any other package. Patrick Mayer in particular was keen to minimize message size in his use cases to the bare minimum, say by maintaining himself a perfect hash for fingerprints and hence sending only a small integer across the wire.

comment:12 Changed 4 years ago by bgamari

I agree that being able to capture a StaticPtr as plain old data that can be easily serialized, like StaticKey, is very useful and a more natural approach than baking serialization logic into the compiler/standard libraries. It would be nice if the motivation and current implementation were clearly laid out in the Wiki page.

Last edited 4 years ago by bgamari (previous) (diff)

comment:13 Changed 4 years ago by bgamari

Milestone: 8.0.18.2.1

In light of comment:9, it seems like purifying unsafeLookupStaticPtr is something worth doing but won't happen for 8.0.

comment:14 Changed 3 years ago by bgamari

Milestone: 8.2.18.4.1

It doesn't look like this will happen for 8.2. At this point it seems less and less likely that this will happen at all given that static pointers have been out in the wild for years now.

comment:15 Changed 21 months ago by bgamari

Milestone: 8.4.18.6.1

This ticket won't be resolved in 8.4; remilestoning for 8.6. Do holler if you are affected by this or would otherwise like to work on it.

comment:16 Changed 16 months ago by bgamari

Milestone: 8.6.18.8.1

These won't be fixed for 8.6, bumping to 8.8.

comment:17 Changed 10 months ago by osa1

Milestone: 8.8.18.10.1

Bumping milestones of low-priority tickets.

Note: See TracTickets for help on using tickets.