rts: Add Stack.c and accompanying rtsflags
AbandonedPublic

Authored by Tarrasch on Feb 15 2015, 4:59 PM.

Details

Reviewers
scpmw
simonmar
tibbe
austin
Trac Issues
#3693
Summary

This new module is meant to contain C-functions that operate on the
execution stack and its members (the stack frames). My intent at the
moment is to come one step closer to implementing stack traces.

Test Plan

I wait with testing this module and the rtsflags. I thought of doing
full-stack (pun intended) tests by testing that stack-traces are
available from Haskell land in pure Haskell tests. In those tests, we'll
be using the rtsflags as well.

Also, the changes here does not affect existing code paths that much. It
mainly just adds new functions.

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
add-stack-c-in-rts
Lint
Lint WarningsExcuse: The neigboring implementations are oneliners as well
SeverityLocationCodeMessage
Warningincludes/rts/storage/ClosureMacros.h:70TXT3Line Too Long
Warningincludes/rts/storage/ClosureMacros.h:83TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 3248
Build 3275: GHC Patch Validation (amd64/Linux)
Tarrasch updated this revision to Diff 2239.Feb 15 2015, 4:59 PM
Tarrasch retitled this revision from to rts: Add Stack.c and accompanying rtsflags.
Tarrasch updated this object.
Tarrasch edited the test plan for this revision. (Show Details)
Tarrasch added reviewers: scpmw, simonmar, austin, tibbe.
Tarrasch updated the Trac tickets for this revision.

Hi guys, I started with a small patch towards stack traces. I plan to very soon upload a bigger diff which shows the direction as a whole I'm thinking about.

I kept this diff very small and a bit extra clean, maybe it can be considered for merging after only a few iterations of review?

rts/RtsFlags.c
281 ↗(On Diff #2239)

This I see as very controversial. As far as I can tell, this is the only parameter with a space in it. Should I just rename this to something like -r<x> and get on with it?

rts/Stack.c
122

Come to think about this. I probably should remove this from this patch right? It won't be used in subsequent patches either. Opinions?

Ok, uploaded D662 now which should show how this patch can be used.

Tarrasch updated this revision to Diff 2242.Feb 15 2015, 6:04 PM

Fix unsigned/signed bug. Thanks Harbormaster and static analysis!

Tarrasch updated this revision to Diff 2244.Feb 16 2015, 2:52 AM

Set default limit 100 on max stack reification size

I also removed the debug function dumpStackStructure and commented code.
Added some comments.

Tarrasch added inline comments.Feb 16 2015, 2:56 AM
rts/Stack.c
23

So this function will iterate "yield" on the UNDERFLOW_FRAMEs but not the STOP_FRAME. I guess it should do neither (preferably) or both. Any opinions on this?

Tarrasch updated this revision to Diff 2251.Feb 17 2015, 4:32 PM

fix validation error in countStackSize

scpmw edited edge metadata.Feb 22 2015, 11:51 AM

We should probably find some sort of proper intermediate goal to work towards... D661 does pretty much nothing, so it's hard to argue about. On the other hand, D669 does a lot that we probably don't want, so it's not very useful as a starting point for a discussion either.

I think the design space here is mainly about how much effort we put into two things:

  1. How do we get our information about object code?
    • From the symbol table, using libelf or libbfd - or other libs depending on plattform. Not really useful, as that will yield us relatively useless back-end names.
    • Use the generated DWARF information, either using libdwarf or something else, possibly custom-built. This should yield us some proper Haskell names (in contrast to GDB & co, we won't simply ignore the DW_name field).
    • Use custom information in .debug_ghc as D662 currently does. Lots of extensibility, but also requires new GHC code. Probably hard to justify until we have a good application for it (profiling, possibly).
  2. How to make them available to the user?
    • Just have a RTS function that produces a stack trace - or have a flag that does it automatically at certain points, like -xc currently.
    • Also have a function, but have it generate something that's usable from Haskell-land.
    • Support automatically attaching reified stack traces to exceptions

My intuition would be to shoot for the simplest point in the design space here. Manually triggered stack traces on the back of symbol names? That could be quite a compact patch. I will be a bit short on time the coming weeks, but could probably contribute a suitably reduced-down Dwarf.c version.

I agree that this patch can be hard to argue about since it doesn't provide any value on it's own.

My intuition would be to shoot for the simplest point in the design space here. Manually triggered stack traces on the back of symbol names? That could be quite a compact patch.

Hmm, So you don't need the .debug_ghc section to get just the symbol names? That would be a given starter I think. I do want to avoid to create one huge patch, but to add things step by step in my differential patches. At the same time, as you mentioned, the patches should provide some value or they will be hard to argue about.

I think I should aim to also expose the stack traces in Haskell land in the same patch. Then I can test the whole chain with test cases in the base library.

simonmar requested changes to this revision.Feb 28 2015, 4:49 AM
simonmar edited edge metadata.

This is not really ready, but I've added a few comments nonetheless.

rts/RtsFlags.c
278 ↗(On Diff #2251)

It's not clear what this actually does from the user's perspective.

279 ↗(On Diff #2251)

I'd like to avoid the word "reify" in what we expose to the user, and go for something more descriptive, such as "capture" or "save".

rts/RtsUtils.c
114–129 ↗(On Diff #2251)

I'd put this in Storage.c, and drop the stg prefix.

rts/Stack.c
67

no magic numbers please!

87–91

This is GET_ENTRY(p)

rts/Stack.h
22

getExecutable (no "e")

Also I don't think you need to expose this, it's not used outside the file. And finally it's not a very descriptive name :)

This revision now requires changes to proceed.Feb 28 2015, 4:49 AM

Thanks for your comments Simon! I just stumbled upon a small road-block on GET_ENTRY. I'm about to submit my patch once it validates. :)

rts/Stack.c
87–91

I first replaced this with simply

return GET_ENTRY(p);

But it seems that ENTRY_CODE() is undefined in c-land. So I had to go on and implement it.

#ifdef TABLES_NEXT_TO_CODE
     INLINE_HEADER StgFunPtr ENTRY_CODE(const StgInfoTable *info) {return (StgFunPtr)info;}
#else
     INLINE_HEADER StgFunPtr ENTRY_CODE(const StgInfoTable *info) {return info.code;}
#endif

(untested). Am I heading in the wrong direction here?

simonmar added inline comments.Mar 2 2015, 7:19 AM
rts/Stack.c
87–91

Ah yes, it looks like this got lost at some point during a past refactoring. You should add ENTRY_CODE to includes/rts/storage/ClosureMacros.h with the other inline functions there.

Tarrasch updated this revision to Diff 2337.Mar 2 2015, 3:41 PM
Tarrasch edited edge metadata.

Fix comments from @simonmar

I fixed most comments by simplifying the diff. In particular I removed the
rts-flags. I don't think they are necessary until we have automatic on-raise
reification. Which is a long way to go.

@simonmar, should I send the ENTRY_CODE implementations in their own diff so the diffs I send are as small as possible (while still being valuable on their own)?

bgamari added a subscriber: bgamari.Mar 2 2015, 4:02 PM

Anyway, starting tomorrow. I'll be on vacation for 3 weeks. I'll continue to iterate on this patch after that. :)

lelf added a subscriber: lelf.Apr 3 2015, 9:00 AM

@scpmw, Did you have time to look anything at a minified version of Dwarf.c?

scpmw added a comment.Apr 21 2015, 3:42 AM

Sorry, not yet. Do you need it to make progress?

@scpmw, So I'm not sure exactly what I will be needing. I noticed that in Dwarf.c, you do a lot of the libelf stuff as well. I figured I should trim down that Dwarf.c to just do the libelf stuff. More precisely, I should use the Elf information in the binary to look up symbol names, using the instruction pointers that reside on the Haskell stack. Does that "plan" make sense @scpmw? It wouldn't use any of the dwarf information, but we can still retrieve names from the symbol table. Is that correct?

I'll be happy to just mess around in Dwarf.c since i think I'll learn a lot by doing it myself too. :)

tibbe edited edge metadata.May 31 2015, 8:36 AM

Is this pull request still in progress?

Tarrasch abandoned this revision.May 31 2015, 10:53 AM

Not really this particular patch. I'm working on a new patch, which tries to provide some actual value like D662 does, but tries to stay "minimal" like this Diff.

Next week I'll spend a lot of time on this, as we're having our annual hack week at Spotify. :)