Add missing local root in win32 Unix.select#9051
Conversation
|
cc our local-roots experts, I guess @let-def, @stedolan and @jhjourdan. |
|
Smoking gun on my fork's AppVeyor with |
xavierleroy
left a comment
There was a problem hiding this comment.
Congratulations and thanks for the detective work!
The fixes are correct as far as I can say.
Concerning GC rule 5, it is a hard rule: caml_alloc_small does not initialize the contents of the new block, while Store_field, which is a wrapper around caml_modify, reads the old value of the modified field. So, the original code was reading uninitialized memory....
| value newres = caml_alloc_small(2, 0); | ||
| Field(newres, 0) = s; | ||
| Field(newres, 1) = res; | ||
| res = newres; |
There was a problem hiding this comment.
For the record: the original code (with Begin_roots2) was modeled after the fdset_to_fdlist function from otherlibs/unix/select.c, which still uses... old-style Begin_roots2/End_roots root management.
There was a problem hiding this comment.
Indeed - I think it's OK only to "upgrade" these macros when something's actually changed/corrected?
There was a problem hiding this comment.
(the Unix version obviously has no problem, since the representation of fd's is simpler)
|
Could you please add a Changes entry in the 4.10 section? We'll cherry-pick to 4.10, of course. |
|
Thanks @xavierleroy - I was happy to keep it without a |
Add missing local root in win32 `Unix.select` (cherry picked from commit dd492d8)
While treating Field() as an lvalue and assigning directly to it is possible, it appears that this is not always safe. Tests have shown that when storing Int64 values into structured blocks (tuples) by direct assignment to Field(), the resulting code can fault under load attempting to access an Int64 at an invalid address (0x1). Therefore, we will play it safe and use Store_field() throughout. /cc @stedolan
This fixes #9043.
The patch is organised in three commits. The first is the simplest fix:
It appears that the planets aligned (consistently) in such a way that msvc64 AppVeyor was routinely triggering a GC at the
caml_alloc_smallandfdlistwas moved. The planets had to be aligned to the point that the test was failing only as part ofmake alland not asmake one.I did a limited test to confirm the problem by manually inserting a call
caml_gc_full_majorimmediately after thevalue s = Field(…line. This causes all the Windows ports to freeze at this stage, suggesting a problem. With theBegin_roots3version, they continue to work.The second commit converts the entire function to use
CAMLparaminstead (I think it only usedBegin_roots2because it was taken from an old function which predates those macros).While going through
select.c, I noticed uses ofStore_fieldfollowingcaml_alloc_smallwhich violates GC Rule 5 (although I'm not sure that it's a problematic violation of Rule 5, since the expressions in question don't allocate).