Skip to content

Attempt to fix a probable bug introduced by GPR#156.#279

Merged
alainfrisch merged 1 commit intoocaml:trunkfrom
alainfrisch:fix_frametables
Nov 5, 2015
Merged

Attempt to fix a probable bug introduced by GPR#156.#279
alainfrisch merged 1 commit intoocaml:trunkfrom
alainfrisch:fix_frametables

Conversation

@alainfrisch
Copy link
Copy Markdown
Contributor

@alainfrisch alainfrisch commented Nov 3, 2015

See my last note on #156.

It would be interesting to re-run benchmarks done when accepting #156 to see if the gains are confirmed after this fix.

@alainfrisch alainfrisch mentioned this pull request Nov 3, 2015
Closed
alainfrisch added a commit that referenced this pull request Nov 5, 2015
@alainfrisch
Merge pull request #279 from alainfrisch/fix_frametables
3cc3932 
Attempt to fix a probable bug introduced by GPR#156.
@alainfrisch alainfrisch merged commit 3cc3932 into ocaml:trunk Nov 5, 2015
@alainfrisch
Copy link
Copy Markdown
Contributor Author

alainfrisch commented Nov 5, 2015

We've got confirmation that this patch fixes the segfaults on several tests for our customer, so I've merged this PR.

I confirm this patch is the cause of some segfaults; we've seen this on Coq with flambda at high inlining > levels.

@mshinwell @chambart Can you confirm that the patch proposed here (now merged) fixes those segfaults?

@chambart
Copy link
Copy Markdown
Contributor

chambart commented Nov 6, 2015

@alainfrisch The fix seems correct. In fact it was missing only missing

   frametables = new_frametables

There is no need to move the last expressions into the if branches (frametables is correctly NULL at that point in the first branch)

Sorry for the bug, it seems that I introduced it while renaming things in the patch chambart@3fdaf02#diff-3c7717e893a739a54e5fbc92ec4b7100L148

@alainfrisch
Copy link
Copy Markdown
Contributor Author

alainfrisch commented Nov 6, 2015

frametables is correctly NULL at that point in the first branch

I don't understand. Since frametables is NULL, tail->next = frametables will set tail->next to NULL, but tail still points to the last new frametable; this will in effect remove previous frametables from the linked list. Or did I miss something?

@chambart
Copy link
Copy Markdown
Contributor

chambart commented Nov 6, 2015

Effectively, sorry...

@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Nov 13, 2015

Is this solved to everyone's satisfaction? If so please close this PR as well as PR#156.

@alainfrisch
Copy link
Copy Markdown
Contributor Author

alainfrisch commented Nov 13, 2015

I closed #156, but for some reason, I don't see a "Close" button on this one.

@gasche
Copy link
Copy Markdown
Member

gasche commented Nov 13, 2015

It is automatically considered closed (in fact "Merged") since you merged the corresponding patch -- this PR does not appear in the list of open PRs anymore.

@alainfrisch
Copy link
Copy Markdown
Contributor Author

alainfrisch commented Nov 13, 2015

Ah thanks, I was confused by Xavier's request.

@xavierleroy
Copy link
Copy Markdown
Contributor

xavierleroy commented Nov 13, 2015

My bad, I was probably looking at an outdated Web page generated by Github.

stedolan pushed a commit to stedolan/ocaml that referenced this pull request Feb 20, 2020
@kayceesrk
Merge pull request ocaml#279 from ctk21/ephe_clean_assert_fix
e0b939f 
Fix assert in caml_ephe_clean
mshinwell pushed a commit to mshinwell/ocaml that referenced this pull request Jan 6, 2021
@lthls
Various improvements related to unboxing and meet (ocaml#279)
9faf883 
* Various improvements related to unboxing and meet

* Review: Catch invalid field reads earlier

* Add comment on Row_like.For_blocks.get_field
chambart pushed a commit to chambart/ocaml-1 that referenced this pull request Jan 4, 2022
@lthls
Sort_lifted_constants: fix duplication of definitions (ocaml#279)
79fa7a9
EmileTrotignon pushed a commit to EmileTrotignon/ocaml that referenced this pull request Jan 12, 2024
@tmattio
Syndicated RSS feed (ocaml#279)
a10ec04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alainfrisch @chambart @xavierleroy @gasche