Skip to content

Fix leftover TODO in runtime/startup_byt.c#11051

Merged
xavierleroy merged 1 commit intoocaml:trunkfrom
gadmm:unlock_error
Feb 23, 2022
Merged

Fix leftover TODO in runtime/startup_byt.c#11051
xavierleroy merged 1 commit intoocaml:trunkfrom
gadmm:unlock_error

Conversation

@gadmm
Copy link
Copy Markdown
Contributor

@gadmm gadmm commented Feb 23, 2022

Fixes the following:

../runtime/ocamlrun ../boot/ocamlc -use-prims ../runtime/primitives -strict-sequence -absname -w +a-4-9-41-42-44-45-48-70 -g -warn-error +A -bin-annot -nostdlib -principal -safe-string -strict-formats  -nopervasives -c camlinternalFormatBasics.mli
Fatal error: Fatal error during unlock: Operation not permitted

Program received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
49	../sysdeps/unix/sysv/linux/raise.c: Aucun fichier ou dossier de ce type.
(gdb) bt
0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
1  0x00007ffff7c61864 in __GI_abort () at abort.c:79
2  0x00005555555890b2 in caml_fatal_error (msg=msg@entry=0x55555559c224 "Fatal error during %s: %s\n") at misc.c:117
3  0x000055555558a4d5 in caml_plat_fatal_error (action=action@entry=0x55555559d04b "unlock", err=<optimized out>)
    at platform.c:36
4  0x0000555555580765 in check_err (err=<optimized out>, action=0x55555559d04b "unlock") at caml/platform.h:130
5  caml_plat_unlock (m=<optimized out>) at caml/platform.h:163
6  channel_mutex_unlock_default (chan=<optimized out>) at io.c:86
7  0x0000555555581217 in check_pending (channel=0x5555555ea5e0) at io.c:120
8  check_pending (channel=0x5555555ea5e0) at io.c:115
9  caml_getblock (channel=channel@entry=0x5555555ea5e0, p=p@entry=0x7fffffffda30 "\240\332\377\377\002",
    len=len@entry=20) at io.c:400
10 0x0000555555581309 in caml_really_getblock (chan=chan@entry=0x5555555ea5e0,
    p=p@entry=0x7fffffffda30 "\240\332\377\377\002", n=n@entry=20) at io.c:430
11 0x000055555557d9c3 in caml_input_val (chan=chan@entry=0x5555555ea5e0) at intern.c:752
12 0x000055555559667f in caml_main (argv=0x7fffffffdbf8) at startup_byt.c:560
13 0x000055555556b2b2 in main (argc=<optimized out>, argv=<optimized out>) at main.c:37

(obtained on a private branch while working on signals, so it is normal that you do not see it by yourself when compiling)

Please review my solution thoroughly, I am not sure what difficulty the TODO comment was alluding to.

Is there any other TODO/FIXME left in the code that should be fixed before 5.0?

@gadmm
Copy link
Copy Markdown
Contributor Author

gadmm commented Feb 23, 2022

startup_byt.c:560:3: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
  560 |   value global_data = caml_input_val(chan);
      |   ^~~~~

Since 5.0 intends to require a C11 compiler, it is a good time to get rid of this restriction @xavierleroy.

configure.ac Outdated
cc_warnings=''],
[outputobj='-o $(EMPTY)'
warn_error_flag='-Werror'
cc_warnings='-Wall -Wdeclaration-after-statement'])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that the PR itself should be about the code change (from the title), and that this configuration change is unrelated, requires more discussion, and should not be included in the PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened here: #11052

@gasche
Copy link
Copy Markdown
Member

gasche commented Feb 23, 2022

You split the configuration-related change in #11052, thanks! If you can make the present PR work without it, I'm happy to approve it. (I think that just adding {...} blocks in middle of statements is possible, and those blocks can have local bindings? Otherwise moving the declaration ahead would work.)

@gadmm
Copy link
Copy Markdown
Contributor Author

gadmm commented Feb 23, 2022

Please review my solution thoroughly, I am not sure what difficulty the TODO comment was alluding to.

cc @ctk21

Copy link
Copy Markdown
Contributor

@xavierleroy xavierleroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well spotted! Thanks for the fix.

@xavierleroy
Copy link
Copy Markdown
Contributor

Note that 4.14 has the same lock/unlock, added 2 years ago : e678885 .

@gadmm
Copy link
Copy Markdown
Contributor Author

gadmm commented Feb 23, 2022

That is exactly why I was wondering if there is more to it!

@ctk21
Copy link
Copy Markdown
Contributor

ctk21 commented Feb 23, 2022

The patch looks reasonable to me but I'm not an expert in the io locking.
The comment was put in because I noticed the discrepancy with ocaml/ocaml at the time, but wasn't sure why multicore had diverged.

Fixes the following:

../runtime/ocamlrun ../boot/ocamlc -use-prims ../runtime/primitives -strict-sequence -absname -w +a-4-9-41-42-44-45-48-70 -g -warn-error +A -bin-annot -nostdlib -principal -safe-string -strict-formats  -nopervasives -c camlinternalFormatBasics.mli
Fatal error: Fatal error during unlock: Operation not permitted

Program received signal SIGABRT, Aborted.
__GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
49	../sysdeps/unix/sysv/linux/raise.c: Aucun fichier ou dossier de ce type.
(gdb) bt
0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
1  0x00007ffff7c61864 in __GI_abort () at abort.c:79
2  0x00005555555890b2 in caml_fatal_error (msg=msg@entry=0x55555559c224 "Fatal error during %s: %s\n") at misc.c:117
3  0x000055555558a4d5 in caml_plat_fatal_error (action=action@entry=0x55555559d04b "unlock", err=<optimized out>)
    at platform.c:36
4  0x0000555555580765 in check_err (err=<optimized out>, action=0x55555559d04b "unlock") at caml/platform.h:130
5  caml_plat_unlock (m=<optimized out>) at caml/platform.h:163
6  channel_mutex_unlock_default (chan=<optimized out>) at io.c:86
7  0x0000555555581217 in check_pending (channel=0x5555555ea5e0) at io.c:120
8  check_pending (channel=0x5555555ea5e0) at io.c:115
9  caml_getblock (channel=channel@entry=0x5555555ea5e0, p=p@entry=0x7fffffffda30 "\240\332\377\377\002",
    len=len@entry=20) at io.c:400
10 0x0000555555581309 in caml_really_getblock (chan=chan@entry=0x5555555ea5e0,
    p=p@entry=0x7fffffffda30 "\240\332\377\377\002", n=n@entry=20) at io.c:430
11 0x000055555557d9c3 in caml_input_val (chan=chan@entry=0x5555555ea5e0) at intern.c:752
12 0x000055555559667f in caml_main (argv=0x7fffffffdbf8) at startup_byt.c:560
13 0x000055555556b2b2 in main (argc=<optimized out>, argv=<optimized out>) at main.c:37
@xavierleroy
Copy link
Copy Markdown
Contributor

OK, thanks for the info. I am pretty confident about this fix, so let me merge now.

@xavierleroy xavierleroy merged commit 53fbcc1 into ocaml:trunk Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants