Correctly add unwinding info in manifestSp and makeFixupBlocks

Authored by niteria on Apr 18 2018, 9:04 AM.



In manifestSp the unwind info was before the relevant instruction, not after.
I added some notes to establish semantics.
Also removes redundant annotation in stg_catch_frame.

For makeFixupBlocks it looks like we were off by wORD_SIZE dflags.
I'm not sure why, but it lines up with manifestSp.
In fact it lines up so well so that I can consolidate the Sp unwind logic
in maybeAddUnwind.
I detected the problems with makeFixupBlocks by running T14779b
after patching D4559.

Test Plan

added a new test

Diff Detail

rGHC Glasgow Haskell Compiler
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
niteria created this revision.Apr 18 2018, 9:04 AM
niteria requested review of this revision.Apr 18 2018, 9:05 AM
niteria added inline comments.

These may not be available everywhere... Does anyone know a way to run the test only when certain tools are available?

niteria planned changes to this revision.Apr 19 2018, 3:19 AM

whitespace differences in test output

niteria updated this revision to Diff 16085.Apr 19 2018, 6:50 AM

normalize whitespace in test

niteria updated this revision to Diff 16086.Apr 19 2018, 6:59 AM

only run on 64bit linux

niteria updated this revision to Diff 16090.Apr 19 2018, 8:56 AM
  • Fix cmm lint errors on T14779b that D4559 would detect
  • Consolidate the logic for adding Sp unwind in maybeAddSpAdj
niteria retitled this revision from Correctly add unwinding info in manifestSp to Correctly add unwinding info in manifestSp and makeFixupBlocks.Apr 19 2018, 9:00 AM
niteria edited the summary of this revision. (Show Details)

Looks quite reasonable to me. However, we should fix the test to only run when gdb and readelf are available; I don't think we want to require them to run the testsuite.


This is much nicer!


I'm not sure we have a great way of doing this currently. However, it would be pretty easy to add a test modifier to implementing this check.

bgamari requested changes to this revision.Apr 19 2018, 10:36 AM
This revision now requires changes to proceed.Apr 19 2018, 10:36 AM
simonmar added inline comments.Apr 21 2018, 4:30 AM

This is awesome commentary. Thanks so much for writing it.


I think you mean A ends at Y-1. Correct?

niteria updated this revision to Diff 16158.Apr 23 2018, 9:05 AM
niteria marked 3 inline comments as done.
  • Y => Y-1
  • skip the test when gdb or readelf unavailable
niteria added inline comments.Apr 23 2018, 9:09 AM


You're right. To my defense this was [X, Y) in my head.

niteria marked 3 inline comments as done.Apr 23 2018, 9:09 AM
niteria updated this revision to Diff 16162.Apr 23 2018, 11:06 AM

more debugging tips

Do you consider this to be done, @niteria?

Yes, ready to give it a last review and merge if nothing is off.

LGTM, I'll let @bgamari sign off on it.

bgamari accepted this revision.May 3 2018, 11:13 AM

Looks good to me as well.

This revision is now accepted and ready to land.May 3 2018, 11:13 AM
This revision was automatically updated to reflect the committed changes.
osa1 added a subscriber: osa1.May 4 2018, 3:13 AM

This broke the build for me. It seems the test output needs updating:

Actual stdout output differs from expected:
diff -uw "./codeGen/should_compile/" "./codeGen/should_compile/"
--- ./codeGen/should_compile/        2018-05-04 11:12:40.388983501 +0300
+++ ./codeGen/should_compile/    2018-05-04 11:12:40.388983501 +0300
@@ -4,6 +4,7 @@
 End of assembler dump.
 Contents of the .debug_frame section:

 00000000 0000000000000014 ffffffff CIE "" cf=1 df=-8 ra=16
  LOC CFA rbp rsp ra
 0000000000000000 rbp+0 v+0 s c+0
*** unexpected failure for T14999(normal)

Unexpected results from:

Sigh, looks like your readelf puts an extra newline there. I will try collapsing consecutive newlines to normalize.

osa1 added a comment.May 5 2018, 5:59 AM

@niteria That fixed it for me. Thanks.