Opened 15 months ago

Closed 5 months ago

#9122 closed bug (fixed)

Make Lint check for bad uses of `unsafeCoerce`

Reported by: simonpj Owned by: qnikst
Priority: normal Milestone: 7.12.1
Component: Compiler Version: 7.8.2
Keywords: newcomer Cc:
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: Differential Revisions: Phab:D637

Description (last modified by simonpj)

I think it would be a great idea for Core Lint to check for uses of unsafeCoerce that don't obey the rules. It won't catch all cases, of course, but it would have caught #9035. Specficially, look for:

  • Coercions between lifted and unboxed types
  • Coercion between unboxed types of different sizes
  • Coercion between unboxed ints and floats.

Would anyone like to make a patch for this? Anything that can be checked by Core Lint, should be checked!

I'm afraid I don't know where to look for the reason for the int/float difficulty. I'd write a tiny function that exhibits the unsafe conversion and look the code it generates.

Wiki design page BadUnsafeCoercions

Change History (17)

comment:1 Changed 15 months ago by goldfire

What are "the rules"? Where are they written? And, what if someone wants to do one of the things mentioned? I can imagine wanting to coerce between lifted and unboxed types or between ints and floats. (Unboxed types of different sizes, I tend to agree, wouldn't want to get coerced.)

comment:2 Changed 15 months ago by simonpj

The rules are documented with unsafeCoerce, here.

It's pretty much guaranteed that coercing from, say Int# to Int will give rise to serious problems.

Simon

comment:3 Changed 15 months ago by goldfire

I was thinking of someone who wanted the raw pointer associated with a value of a lifted type. Coercing Int to Int# would seemingly get that pointer value. It's conceivable to me to imagine scenarios where this behavior is desired, such as debugging against some sort of memory dump. Going the other way might also be desirable, if Haskell code has to operate against some pre-loaded memory image. Maybe these sorts of things never happen in Haskell, but I've done them (when debugging kernels / device drivers) in other languages. Or, is the idea that all such shenanigans are subsumed by Foreign.Ptr and friends?

comment:4 Changed 15 months ago by simonpj

Right after you get the pointer in your hand, garbage collection may happen, which moves the object. This way lies madness.

Simon

comment:5 Changed 15 months ago by goldfire

Fair enough -- such users can always simply avoid -dcore-lint anyway.

comment:6 Changed 9 months ago by goldfire

  • Keywords newcomer added

I'm happy to advise a newcomer if they want to take this one on.

comment:7 Changed 6 months ago by qnikst

  • Owner set to qnikst

comment:8 Changed 6 months ago by goldfire

I'm helping qnikst out with this, but I have a question.

Say someone writes

unsafeCoerce# (3 :: Int) :: Int#

According to the original feature request, this should be a Core Lint error. However, is it an error earlier in the GHC pipeline? In 7.8.3, this compiles just fine, though it falls over rather completely when run. (No surprise there!) If it's not an error earlier, this would seem to violate the principle that Core Lint should just be checking for GHC bugs, not user silliness.

So, is it necessary to check for such silliness in the typechecker now, too?

comment:9 follow-ups: Changed 6 months ago by simonpj

I don't know how to reason about what might be "an error at some particular stage of the pipeline".

ANY use of unsafeCoerce might make a program seg-fault. So at best these checks are going to smoke out uses that are more than usually likely to cause this behaviour. It's all very squishy. Ask a more specific question and I'll try to help.

S

comment:10 Changed 6 months ago by qnikst

  • Differential Revisions set to Phab:637

comment:11 in reply to: ↑ 9 Changed 6 months ago by qnikst

Replying to simonpj:

I don't know how to reason about what might be "an error at some particular stage of the pipeline".

ANY use of unsafeCoerce might make a program seg-fault. So at best these checks are going to smoke out uses that are more than usually likely to cause this behaviour. It's all very squishy. Ask a more specific question and I'll try to help.

S

I think that Richard means that this check may be useful also in a typechecker, as lint will help from problems introduced by ghc, but doesn't prevent from incorrect programs to be written and accepted by typechecker. So idea was also perform this check in typechecker.

comment:12 Changed 6 months ago by qnikst

  • Differential Revisions changed from Phab:637 to Phab:D637

comment:13 in reply to: ↑ 9 Changed 6 months ago by goldfire

Should the type-checker reject unsafeCoerce# (3 :: Int) :: Int#? If not, the proposed change would mean that -dcore-lint would show an error even when GHC is not at fault.

Or, another (opposite) way to ask the question: Should a program containing that chunk, but compiled without -dcore-lint, be accepted?

comment:14 Changed 6 months ago by qnikst

I want to clarify following questions about required semantics:

  1. Coercion between unboxed types of different sizes Do we want to check _real_ (word aligned size of values) or _active_ (i.e. number of significant bytes)? (On Phab I assumed latter). But what about UnboxedTuples it seems that checking active size is not totally correct there?
  1. Coercion between unboxed ints and floats. I see one usecase for such coercions, for example we have an array of floats, or unboxed tuple, and we want to calculate checksum, then we want to treat that array as an array of other base type. What should we do around unboxed tuples in this check? (On Phab I disabled check for unboxed tuples)

comment:15 Changed 6 months ago by simonpj

  • Description modified (diff)

comment:16 Changed 5 months ago by Austin Seipp <austin@…>

In 76b1e11943d794da61d342c072a783862a9e2a1a/ghc:

Improve core linter so it catches unsafeCoerce problems (T9122)

Summary:
This is a draft of the patch that is sent for review.

In this patch required changes in linter were introduced
and actual check:

 - new helper function: primRepSizeB
 - primRep check for floating
 - Add access to dynamic flags in linter.
 - Implement additional lint rules.

Reviewers: austin, goldfire, simonpj

Reviewed By: simonpj

Subscribers: thomie

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

GHC Trac Issues: #9122

comment:17 Changed 5 months ago by thoughtpolice

  • Milestone set to 7.12.1
  • Resolution set to fixed
  • Status changed from new to closed

Merged, thanks Alexander!

Note: See TracTickets for help on using tickets.