Fix nptr field alignment in RtClosureInspect
ClosedPublic

Authored by osa1 on Fri, Jun 29, 1:44 AM.

Details

Summary

extractSubTerms (which is extracting pointer and non-pointer fields of a
closure) was computing the alignment incorrectly when aligning a 64-bit value
(e.g. a Double) on i386 by aligning it to 64-bits instead of to word size
(32-bits). This is documented in mkVirtHeapOffsetsWithPadding:

Align the start offset (eg, 2-byte value should be 2-byte aligned).
But not more than to a word.

Fixes Trac #15061

Test Plan

Validated on both 32-bit and 64-bit. 32-bit fails with various unrelated stat
failures, but no actual test failures.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
osa1 created this revision.Fri, Jun 29, 1:44 AM
osa1 updated the Trac tickets for this revision.Fri, Jun 29, 1:44 AM
simonpj added inline comments.
compiler/ghci/RtClosureInspect.hs
746

Blimey. That sounds tricky.

Are you sure that this two-line comment is enough to fully explain to an uninformed reader what is happening here?

Since you (presumably) now understand quite a bit of what this code is doing, could you write a Note to explain it for the next person? It seems a shame to waste all that learning. Thanks!

osa1 edited the summary of this revision. (Show Details)Fri, Jun 29, 4:32 AM
osa1 added inline comments.Fri, Jun 29, 4:39 AM
compiler/ghci/RtClosureInspect.hs
746

This is exactly the same logic that mkVirtHeapOffsetsWithPadding (which is referenced in the commit above, in line 780) uses to place fields in a closure payload.

The code in this module is written in a terrible style and I'm sure it's full of bugs still, but if you look at the whole function I think what this function does is clear. Is there anything in particular that you think could use some documentation? I'm happy to document but I'm not sure which parts to document.

I think one improvement over this code would be to reuse mkVirtHeapOffsets instead of duplicating the logic here. It'd require some refactoring but I think it shouldn't be too hard.

osa1 edited the summary of this revision. (Show Details)Fri, Jun 29, 4:39 AM

This is exactly the same logic that mkVirtHeapOffsetsWithPadding (

Well the comment says "see also". It does not say "The function extractSubTerms implements precisely the same logic as mkVirtHeapOffsetsWithPadding ", perhaps with some more supplementary commentary to explain.

I'm happy to document but I'm not sure which parts to document.

Just about everything! (Not your fault of course. But *any* improvement would help.) I don't even understand the *signature* of extractSubTerms let alone hos its implementation work. E.g. what is the [Type] passed into it? What are the types of go, go_unary_types, go_rep and what do they do? I can see you get an array of words out of the closure dataArgs (I guess?), but then how do we know how to decompose them. etc.

bgamari accepted this revision.Fri, Jun 29, 9:24 AM
bgamari added inline comments.
compiler/ghci/RtClosureInspect.hs
746

Yes, code reuse would be great here; this is all very subtle so elimilnating the code entirely would be

Indeed this is all quite subtle and quite a bug-nest; it would be great to eliminating any duplication we can. @osa1, if you plan on continuing refactoring in this area then I would say that we can accept this patch as is and document in later work. However, I agree with Simon that more documentation is needed here. This function, for instance, doesn't even have a comment explaining what it does. The same holds for most of the functions in this module.

This revision is now accepted and ready to land.Fri, Jun 29, 9:24 AM
osa1 added a comment.Sat, Jun 30, 1:30 AM

@simonpj, I started documenting this module in D4911.

I wonder if we could reuse ghc-heap (the library) in the debugger and get rid of RtClosureInspect. Main differences betweent these two are:

  • RtClosureInspect has a much more simplistic view of closures.
  • RtClosureInspect does type refinement as it builds Term for an object, based on constructors it sees on the way. So if I build a Term for something with type a, I get a back a Int if I see a I# constructor during reconstruction.

The first point is easy to handle; just deal with a few more cases or implement a simple mapping from ghc-heap closure type to RtClosureInspects Term.

I can't comment much on the second point. Type reconstruction business seems tricky, and I don't know if it makes sense to do it in ghc-heap (I don't know how it's used currently).

Just an idea ..

In D4906#135301, @osa1 wrote:

@simonpj, I started documenting this module in D4911.

I wonder if we could reuse ghc-heap (the library) in the debugger and get rid of RtClosureInspect. Main differences betweent these two are:

  • RtClosureInspect has a much more simplistic view of closures.
  • RtClosureInspect does type refinement as it builds Term for an object, based on constructors it sees on the way. So if I build a Term for something with type a, I get a back a Int if I see a I# constructor during reconstruction.

Indeed RtClosureInspect already uses ghc-heap's getClosureData. What places are you thinking of replacing? Type reconstruction is indeed tricky; I don't think we can move it in to ghc-heap as it requires large swaths of GHC functionality.

osa1 updated this revision to Diff 17180.Wed, Jul 4, 1:11 AM
  • Say more about offset calculation
This revision was automatically updated to reflect the committed changes.