- User Since
- Jun 6 2014, 1:30 PM (211 w, 1 d)
Feb 2 2017
Wouldn't literal strings always float up to the top level eventually? Always discarding ticks on string literals might be the right thing to do.
Jan 31 2017
Just to clarify my position: No strong opinion on what should or should not be allowed with breakpoints - I was just trying to get as close as possible to the existing semantics, and have as few special cases as possible.
Some comments. Could change things, but feels like a matter of style at this point.
Jan 28 2017
Jan 25 2017
Sorry I didn't react in IRC, was away at the time - Debug.cmmDebugGen simply tries to do the same thing on the Cmm-level that happens in mkTick on Core level: Sometimes Cmm optimisation merges blocks, at which point more source notes enter an existing tick scope, and might become redundant in the process. This does not happen often, yet regularly enough that I noticed it in ThreadScope and added this quick fix.
Dec 10 2016
Played around with it a bit - seems to be doing mostly the right thing. T5654-O1 does not actually test the problematic case, as CorePrep eta-expands f. Also this seems to not work if we "hide" the PAP in a constructor:
Jun 18 2016
Good work. Seems LLVM's DWARF representation has matured a bit since I touched it. Adding debug annotations for the blocks shouldn't be that hard either, right? I added a few more or less obvious remarks.
Dec 10 2015
Not quite sure. Normally the DebugBlock of a procedure refers to the entry block itself, so to me this looks like you are annotating the *second* block. Maybe the first block doesn't contain instructions here and gets optimised away?
Dec 7 2015
Dec 4 2015
Fair enough. The test case might be a bit fragile - maybe just check whether the expected source annotations appear at all?
Nov 26 2015
Hm, interesting idea. The real question here would be whether there is other behaviour that's attached to this flag...
Nov 3 2015
Good point, optimised-out blocks might be a problem here... Especially because in theory they could have ticks (which are now lost). Maybe it would actually be better to let all blocks through, but omit code pointers for "empty" DIEs? I believe that's valid for DWARF.
Oct 31 2015
Good work. Just two small suggestions, see inline comments.
Oct 29 2015
It's not quite as simple as this. It's probably best to look at the output of -ddump-debug to see what's going on, but here's roughly what we start with:
Sep 28 2015
Yes, good direction to take this. (Just realised I didn't answer your comment in D1213 - sorry about that.)
Sep 10 2015
Currently there's no code even using this, so I presume this is just here for completeness' sake?
Sep 4 2015
You are copying the .debug_ghc contents verbatim here? As you are probably aware, my code associated it with DWARF information at this point, also generating event log entries for all other DWARF it could find.
Yeah, the R9 hack is certainly bad. When I originally wrote the code, stg_no_regs was a preprocessor macro, which made it easier in a number of ways.
Sep 3 2015
Also might be a good spot to discuss this - is libdwfl's unwinder segfault-proof? Because especially for Haskell code it will be a bit hit-and-miss whether the backtrace succeeds. The code generator only updates the unwind information at the start of the block, so basically anything after an Sp update until the end of the block would be potential crash territory.
Sep 2 2015
This actually manages to unwind through the signal handler? Wow.
Some small comments...
Sep 1 2015
Well, we would add an additional dependencies for a feature that we know to be incomplete. gdb's backtraces are a lot better and completely portable to boot, so the added value would be marginal. Neither is the fault of the Haskell interface, obviously...
Well, you might not be far off - I am scratching my head a bit too. From looking at that C function it looks like $rsp might get "lost". After all, no unwind rule ever updates it. This doesn't even change when compiling with -fomit-frame-pointers, which definetely should not work. After all, we are computing the CFA for every frame from $rsp, so it cannot stay the same!
@bgamari: I am a bit confused too - normally unwinding should just replicate exactly what the assembly does. I guess your point is that the C code is sloppy with restoring all registers in its unwind rules?
Aug 24 2015
I intend to place both @Tarrasch's CodeMap and my libdw backend (and potentially even libbacktrace- or libunwind-based backends) behind some sort of abstraction, allowing us to use whatever stacktrace mechanism is available at compile time. At the moment I'm leaning towards having all of these use the same StackTrace type.
Aug 21 2015
Perhaps we should expect the s9Q0_info frame should be pushing the underwinder towards the Haskell stack? @scpmw, what did you intend here?
Aug 20 2015
Interesting. A few points:
Aug 19 2015
Jul 23 2015
Looks okay, nothing big stands out. Really disappointing that we need conditional compilation for libelf though... I would really have hoped that we could avoid that.
Jun 28 2015
Jun 24 2015
In my opinion, the CodeMap bits shouldn't become public API in the first place. GHC shouldn't expect the Haskell code to make its memory management decisions. Are we really sure that we have a usage scenario for all that? Can't we get around it using - say - finalisers?
Jun 9 2015
The Haskell module interface needs work in my opinion. Apart from that - what does it look like in the end? Did you try this for anything of non-trivial size?
Apr 21 2015
Sorry, not yet. Do you need it to make progress?
Apr 2 2015
Valid concern, though a bit tricky. Ideas, ordered from "easiest to do" to "catches most mistakes":
Feb 26 2015
@Tarrasch: I won't be able to do anything substantial until after next week. If you want to have a stab at it, go ahead.
Feb 25 2015
@tibbe: For this patch, reifyStack basically walks the stack and copies found return addresses into a buffer. That's obviously a O(n) operation (even if we introduce a bound), which could be undesirable for exceptions.
Feb 22 2015
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.
Feb 9 2015
Feb 4 2015
I agree, Trac #5654 looks like the same issue. My changes probably have just brought it to light.
Jan 30 2015
Okay, here's the updated test case. D636 in concert with unfolding just happens to undo the damage that floating does - if we can get the expression to stay floated (for which we need to prevent eta-expansion completely), the bug is still present.
Jan 29 2015
Okay, here's what I *think* GHC is doing:
Jan 27 2015
Hm, good point - your semantics indeed update CCS on lambda values. So the semantics disagree on expressions that evaluate to lambda values, but are not lambda expressions themselves. Interesting.
Jan 20 2015
(Er, misunderstood what "without requesting a code review" meant. Thought it would prevent unnecessary notifications. Seems it does the opposite...? Sorry about that.)
Added test case. I am not quite sure how to force GHC to float out an expression - using (1+1) as parameter was the easiest way I could find.
Jan 18 2015
Jan 14 2015
This is sort-of what happens. Except that it would tick the parameter (tickHNFArgs) and remove the scoping portion from z, as it's now a plain variable. This means that we end up with something like:
Jan 12 2015
Dec 19 2014
This was already reported in #ghc (possibly same person?) and is actually already corrected in my local branch. So, yeah :)
Dec 17 2014
I was referring to the fact that we can't link against it for most common distributions, as well as that it depends on libelf to the point that it's entirely useless on anything but Linux. The only realistic shipping option would be to ship a patched version, which is probably something we'd like to avoid.
Dec 16 2014
(landed as multiple commits)
@Tarrasch - this doesn't talk about line numbers explicitly at any point, we leave that to the assembler. You are probably referring to the .debug_ghc records, which aren't yet part of this.
Dec 15 2014
Rebased, omit failing tests.
By the way - arc will squash all this into a single commit by default, right? That would be a shame, as I made sure to structure everything into steps, which was really useful when tracking down bugs and performance regressions. For reference, my arc branch is available on GitHub.
Fixed another rare Core change with annotations found by the testsuite
Dec 11 2014
Added debug test
Dec 10 2014
Made consistent with new revision of D169, fixed lint and corrected
yet another performance test.
Dec 8 2014
Does that mean T4801 is failing, or not?
Yeah, I'll have to change a few things because of the changes in D169 anyway. Won't take too long, hopefully.
Dec 7 2014
Okay, this one passes validate for me, and especially all performance tests. T4801 fails depending on build parameters, apparently due to the extra field in MkCgInfoDown.
Fixed lints and test cases, some tick scopes refactoring.
Dec 5 2014
Yes, I'm aware. @austin told be that he'd still merge once it passes the tests. Trickiest part is still to get the performance tests running (still without -g). Current status:
Dec 3 2014
You are referring to your blog post, I presume? Not too keen on leaving the symbols external - are we sure this won't cause problems when names collide? Maybe just do it for LLVM 3.4, if we really must?
Nov 28 2014
Good initiative! This would make our life easier indeed. The real question is probably whether the LLVM change will go through? After all, from their point of view this is probably yet another extension that only GHC will ever use...
Nov 23 2014
Do these tests fail only with -g, or also without -g? (if the latter, I'm more worried)
Nov 22 2014
Uh, sorry, wanted to get a closer look at that build error before I say something. I now had a chance to check the LLVM 3.4 behaviour - apparently what is happening here is (indeed) exactly what I had before. Except that for LLVM 3.4 setting the definition as "used" seemingly doesn't suffice to protect it.
Nov 17 2014
Sure, working away at it... Don't want to break more than strictly necessary. Some minor notes:
- Arc keeps complaining about going over 80 columns, are we taking that literally?
- Also strange to have to give "excuses" for not fixing file endings. After a few revisions I caved now and added the magical newline to UnariseStg.lhs. Not sure this is a good idea - it should probably be a separate patch?
- Not quite sure what Harbourmaster is trying to tell me, it always fails with strange errors - clearly I am declaring an Ord instance for RealSrcSpan?
- The patch has grown quite a few trivial changes now. Had I known that splitting up patches in Arc is actually quite easy, I would probably have done that right away. If anybody wants to see the break-down, here's my branch on GitHub: https://github.com/scpmw/ghc/commits/profiling-arc-169
- Properly creating multiple tick types at the same time needed quite a bit more plumbing.
- Changed diffExpr so it matches by Core and IdInfo instead of just Core. Actually ran into a case where the Core was identical but IdInfos were different...
- Fixed cost centre placement - they shouldn't be reordered relative to each other. Also fixed some extra scc annotatations getting added to bindings.
Nov 2 2014
Hm, interesting - I can reproduce the problem @spacekitteh mentions with and without the patch, as well as with GHC 7.8.3 and 7.6.1. It is gone for GHC 7.4.2, as well as for a random 7.7 version I had lying around (mid 2013). The latter has LLVM streaming (my first suspicion), so the bug must be something different here.
Oct 29 2014
Oct 28 2014
Rebased to HEAD (and a few code style fixes)
Oct 25 2014
Had another look at this - good news is that it seems to be compatible with LLVM versions down to 2.8 (which is as far as we care).
Oct 23 2014
Oct 15 2014
Okay, here's what I currently have. Good news is that I was closer to resolving the remaining lint warnings than I thought - after taking another good look at them I realised I actually already knew how to fix them. So now we have a 100% Core match.
Added a linting step that actually checks annotation transparency for Core passes (not CorePrep, Stg or Cmm yet!). Resolved all warnings from GHC+library compilation.
Oct 9 2014
Current status is that linting works out almost too well: It has managed to spot quite a few instances where intermediate Core is different with annotations. I probably didn't see them earlier because annotated and un-annotated compilation converged to the same code anyway for nofib. Solving these is great for confidence - but on the other hand quite a time sink. After all, every divergence involves quite a bit of cat-and-mouse across the compiler to find the bad code. I must admit I still average around half a day of investigation to track one down.
Oct 2 2014
Just as a quick bump - even though I'm mainly preoccupied with D169, I have not forgotten about this. The new aliases definitely survive optimisations, which makes it slightly dangerous. As it happens, on my computer some regular expression in the Mac Os splitter actually chokes on the new $def labels that now appear in the assembly. I'll have to investigate that a bit more.
Sep 27 2014
Haha - yes, after a lengthy vacation and PhD viva craziness I'm again actively working on it. I agree with most points raised so far, which unfortunately gives me quite a bit to work on. Especially the documentation bits.
Aug 25 2014
So far this diff is only the "core" changes. This doesn't include actual DWARF generation, let alone profiling code (which tibbe's error is from). I will correct it anyway, as rtsBool is clearly the wrong data type there.
Aug 21 2014
No, not yet - and in fact until recently there was a problem with selector thunk detection, which was a performance-criticial Cmm issue. There are also a few cases where I had decided against a 100% Cmm match in the interest of profile accuracy. For example, not using canned closure code means that we have more information about where the code in question was generated from, without actually risking too much runtime overhead (from what I understand).
Fixed some warnings
Removed spurious file and references to CoreNote