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 #17871 (inconsistency with native_compute and primitive floats) #17872

Merged
merged 1 commit into from Jul 20, 2023

Conversation

proux01
Copy link
Contributor

@proux01 proux01 commented Jul 19, 2023

The Array.of_list in Nativevalues.args_of_accu could result in an OCaml double array when some argument was a primitive float, making all other argument sometimes wrongfully appear as float themselves.

Fixes / closes #17871

  • Added / updated test-suite.
  • Added changelog.
  • Added / updated documentation. (I guess we have to update the cirtical bugs file)
    • Documented any new / changed user messages.
    • Updated documented syntax by running make doc_gram_rsts.
  • Opened overlay pull requests.

@proux01 proux01 requested a review from a team as a code owner July 19, 2023 09:32
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 19, 2023
@proux01 proux01 added kind: fix This fixes a bug or incorrect documentation. part: native compiler needs: changelog entry This should be documented in doc/changelog. part: primitive types Primitive ints, floats, arrays, etc. labels Jul 19, 2023
Copy link
Contributor

@silene silene left a comment

Choose a reason for hiding this comment

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

It is worth adding a testcase and an entry in dev/doc/critical-bugs.

kernel/nativevalues.ml Outdated Show resolved Hide resolved
pretyping/nativenorm.ml Show resolved Hide resolved
@herbelin
Copy link
Member

Fix was quick!

So, iiuc, it is that Array.of_list takes internal-representation decisions on the implicit assumption that all elements of an non-empty list have the same internal representation than the first element of the list. The assumption is justified by typing, but may be wrong in the presence of magic.

Conversely, can we say that OCaml runtime has no proper notion of untyped array (as soon as floats are around)?

@proux01 proux01 requested a review from a team as a code owner July 19, 2023 12:16
@proux01 proux01 removed the needs: changelog entry This should be documented in doc/changelog. label Jul 19, 2023
@proux01
Copy link
Contributor Author

proux01 commented Jul 19, 2023

So, iiuc, it is that Array.of_list takes internal-representation decisions on the implicit assumption that all elements of an non-empty list have the same internal representation than the first element of the list. The assumption is justified by typing, but may be wrong in the presence of magic.

Conversely, can we say that OCaml runtime has no proper notion of untyped array (as soon as floats are around)?

Yes and this makes perfect sense since, as Pierre Marie likes to recall, "Obj.magic is not part of OCaml".

@silene
Copy link
Contributor

silene commented Jul 19, 2023

Array.of_list takes internal-representation decisions on the implicit assumption that all elements of an non-empty list have the same internal representation than the first element of the list.

Yes and no. This is not specific to Array.of_list. This applies to Array.make and [|....|], and by extension to any function that creates an array. For instance, Array.of_list calls Array.make using the head of the list. If this element happens to be a pointer to a floating-point number, then Array.make decides to create an array of floating-point numbers rather than an array of pointers to floating-point numbers.

@proux01 proux01 added this to the 8.18+rc1 milestone Jul 19, 2023
@proux01
Copy link
Contributor Author

proux01 commented Jul 19, 2023

@maximedenes @gares I'm putting the 8.18+rc1 milestone as this is an inconsistency fix but feel free to do whateve you want with that milestone.

kernel/nativevalues.ml Outdated Show resolved Hide resolved
@silene silene self-assigned this Jul 19, 2023
@proux01
Copy link
Contributor Author

proux01 commented Jul 19, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 19, 2023
The Array.of_list in Nativevalues.args_of_accu could result in an
OCaml double array when some argument was a primitive float, making
all other argument sometimes wrongfully appear as float themselves.
@coqbot-app coqbot-app bot added the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 19, 2023
@proux01
Copy link
Contributor Author

proux01 commented Jul 19, 2023

@coqbot run full ci

@coqbot-app coqbot-app bot removed the needs: full CI The latest GitLab pipeline that ran was a light CI. Say "@coqbot run full ci" to get a full CI. label Jul 19, 2023
@proux01
Copy link
Contributor Author

proux01 commented Jul 20, 2023

@silene CI green

@silene
Copy link
Contributor

silene commented Jul 20, 2023

@coqbot merge now

@coqbot-app coqbot-app bot merged commit 333968e into coq:master Jul 20, 2023
6 of 7 checks passed
@coqbot-app coqbot-app bot added this to Request 8.18+rc1 inclusion in Coq 8.18 Jul 20, 2023
@proux01 proux01 deleted the fix_17871 branch July 20, 2023 07:48
gares added a commit to gares/coq that referenced this pull request Jul 24, 2023
@coqbot-app coqbot-app bot moved this from Request 8.18+rc1 inclusion to Shipped in 8.18+rc1 in Coq 8.18 Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: fix This fixes a bug or incorrect documentation. part: native compiler part: primitive types Primitive ints, floats, arrays, etc.
Projects
Coq 8.18
Shipped in 8.18+rc1
Development

Successfully merging this pull request may close these issues.

native_compute + evars or binders + primitive floats gives a proof of False
4 participants