Skip to content

Avoid atomic 64-bit load on Debian armel#13267

Merged
gasche merged 2 commits intoocaml:5.2from
glondu:check-atomic-load64-ro
Jul 1, 2024
Merged

Avoid atomic 64-bit load on Debian armel#13267
gasche merged 2 commits intoocaml:5.2from
glondu:check-atomic-load64-ro

Conversation

@glondu
Copy link
Copy Markdown
Contributor

@glondu glondu commented Jun 28, 2024

Bug: #13234

Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

This looks fairly nice to me (having a configure check for the feature itself is a nice touch). @dustanddreams, do you agree with this approach?

glondu pushed a commit to glondu/ocaml that referenced this pull request Jun 28, 2024
@glondu glondu force-pushed the check-atomic-load64-ro branch from 3e8935a to 7181694 Compare June 28, 2024 07:43
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I approve of this change, but I would wait for @dustanddreams' opinion before merging.

@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 28, 2024

This changes required a substantial amount of work, so I think it would deserve a Changes entry. If I had to write one myself, I would suggest something like this:

- #13234, #13267: fix a compatibility issue with Debian armel (armv5) systems
  (Stéphane Glondu, review by Miod Vallat and Vincent Laviron)

(I chose to attribute the substantial debug and guessing work done by other people as "review", and did not mention my own review which is rather trivial in comparison.)

@gasche
Copy link
Copy Markdown
Member

gasche commented Jun 28, 2024

You should regenerate it yourself and can include this in the patch. (As you run "git push", you can raise your hand and declare to yourself (but out loud) that you are not xz-ing us.)

@glondu
Copy link
Copy Markdown
Contributor Author

glondu commented Jun 28, 2024

You should regenerate it yourself and can include this in the patch.

Done.

(As you run "git push", you can raise your hand and declare to yourself (but out loud) that you are not xz-ing us.)

If so, that wouldn't be deliberate. I've been maintaining OCaml in Debian for more than 15 years and I am attached to my reputation :-)

glondu pushed a commit to glondu/ocaml that referenced this pull request Jun 28, 2024
@glondu glondu force-pushed the check-atomic-load64-ro branch from 02e8888 to 2f1c14d Compare June 28, 2024 09:39
@ghost
Copy link
Copy Markdown

ghost commented Jul 1, 2024

I agree with #13234 (comment) that this should better be a compile-time choice. (__ARM_ARCH <= 5 or something similar)

glondu pushed a commit to glondu/ocaml that referenced this pull request Jul 1, 2024
@glondu glondu force-pushed the check-atomic-load64-ro branch from 2f1c14d to 36b008a Compare July 1, 2024 07:54
@glondu
Copy link
Copy Markdown
Contributor Author

glondu commented Jul 1, 2024

OK, I removed the configure-time detection.

@glondu glondu force-pushed the check-atomic-load64-ro branch from 36b008a to 679dfc1 Compare July 1, 2024 09:27
@glondu glondu changed the title Detect if atomic 64-bit load from RO memory works Avoid atomic 64-bit load on Debian armel Jul 1, 2024
@glondu glondu force-pushed the check-atomic-load64-ro branch from 679dfc1 to 8dcdf0d Compare July 1, 2024 11:29
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I'm happy as well with the new implementation (it is worse in some aspects (it won't automatically work for other systems that would have the same restriction) and better in others (it works better for mild cross-compilation scenarios).) Thanks @dustanddreams for your feedback!

@gasche gasche merged commit e1be7d1 into ocaml:5.2 Jul 1, 2024
gasche added a commit that referenced this pull request Jul 1, 2024
Avoid atomic 64-bit load on Debian armel

(cherry picked from commit e1be7d1)
@glondu glondu deleted the check-atomic-load64-ro branch July 1, 2024 12:50
@gasche
Copy link
Copy Markdown
Member

gasche commented Jul 1, 2024

Thanks! I cherry-picked in trunk as a37ec68.

raspbian-autopush pushed a commit to raspbian-packages/ocaml that referenced this pull request Aug 20, 2024
Bug: ocaml/ocaml#13234
Forwarded: ocaml/ocaml#13267

Gbp-Pq: Name 0010-Avoid-atomic-64-bit-load-on-Debian-armel.patch
raspbian-autopush pushed a commit to raspbian-packages/ocaml that referenced this pull request Aug 20, 2024
Bug: ocaml/ocaml#13234
Forwarded: ocaml/ocaml#13267

Gbp-Pq: Name 0010-Avoid-atomic-64-bit-load-on-Debian-armel.patch
raspbian-autopush pushed a commit to raspbian-packages/ocaml that referenced this pull request Sep 5, 2024
Bug: ocaml/ocaml#13234
Forwarded: ocaml/ocaml#13267

Gbp-Pq: Name 0010-Avoid-atomic-64-bit-load-on-Debian-armel.patch
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.

2 participants