Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #7829: pointer comparisons in assertions #8585

Merged
merged 3 commits into from Apr 8, 2019

Conversation

Projects
None yet
3 participants
@damiendoligez
Copy link
Member

commented Apr 4, 2019

This is a replacement for #1311 and a fix for #7829. When comparing values in debug assertions, cast them to pointers to make sure we don't get into trouble with the sign bit.

Note, this is only relevant on some 32-bit machines, the most important being our Travis CI x86 worker.

@damiendoligez damiendoligez requested a review from dra27 Apr 4, 2019

@damiendoligez damiendoligez added this to the 4.08 milestone Apr 4, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

I remember trying this with #1311 - it doesn't fix it, sadly.

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@dra27 Do you have a machine where you can reproduce this? Debugging through Travis is not the easiest thing...

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@damiendoligez - yes, my Ubuntu 16.04 VM (set up to match Travis) seems to be behaving the same way

@dra27 dra27 referenced this pull request Apr 4, 2019

Merged

Follow-up to #8571 #8580

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@damiendoligez - I think I may have it:

diff --git a/runtime/freelist.c b/runtime/freelist.c
index 4d16b99..fb129bd 100644
--- a/runtime/freelist.c
+++ b/runtime/freelist.c
@@ -551,7 +551,7 @@ void caml_fl_add_blocks (value bp)
     cur = Field(cur, 0);
   } while (cur != Val_NULL);
 
-  if (bp > fl_last){
+  if (Bp_val(bp) > Bp_val(fl_last)){
     Next (fl_last) = bp;
     if (fl_last == caml_fl_merge && (char *) bp < caml_gc_sweep_hp){
       caml_fl_merge = Field (bp, 1);
@@ -564,7 +564,7 @@ void caml_fl_add_blocks (value bp)
 
     prev = Fl_head;
     cur = Next (prev);
-    while (cur != Val_NULL && cur < bp){
+    while (cur != Val_NULL && Bp_val(cur) < Bp_val(bp)){
       CAMLassert (Bp_val (prev) < Bp_val (bp) || prev == Fl_head);
       /* XXX TODO: extend flp on the fly */
       prev = cur;
@dra27
Copy link
Contributor

left a comment

I've run dra27@824ef65 through Travis on top of this PR and that seems finally to have nailed it.

Show resolved Hide resolved runtime/freelist.c
@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I think truncate_flp may need a change too?

@damiendoligez damiendoligez force-pushed the damiendoligez:fix-gc-pointer-assertions branch from 93295e4 to 64174b2 Apr 5, 2019

@damiendoligez

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@dra27 you're perfectly right, thanks for debugging this. I've added a commit with your fixes.

@dra27 dra27 merged commit 772ba89 into ocaml:trunk Apr 8, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

dra27 added a commit that referenced this pull request Apr 8, 2019

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Squashed to 4.08 in 8be0b87

@gasche

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

(My favorite command for this is git cherry-pick -m 1 -x <trunk-merge-commit>, which includes a reference to the trunk commit hash in the commit message of the cherry-picked version.)

@dra27

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Ah, ok - that'd be a case for merging (I rebased this one, since I wanted to keep the fix to the assertions and the fix to the comparisons in separate commits on trunk, but was less bothered about that on 4.08)

@damiendoligez damiendoligez deleted the damiendoligez:fix-gc-pointer-assertions branch Apr 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.