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

icu-devel: update PowerPC patch #16922

Closed
wants to merge 1 commit into from
Closed

Conversation

catap
Copy link
Contributor

@catap catap commented Dec 8, 2022

Description

This is updated version of patch by request from upstream.

I've skip ci because it can't work on icu-devel port because of conflict with icu port.

See: unicode-org/icu#2257

Type(s)
  • enhancement
Tested on

macOS 12.6 21G115 x86_64
Xcode 14.1 14B47b

Verification

Have you

  • followed our Commit Message Guidelines?
  • squashed and minimized your commits?
  • checked that there aren't other open pull requests for the same change?
  • referenced existing tickets on Trac with full URL?
  • checked your Portfile with port lint --nitpick?
  • tried existing tests with sudo port test?
  • tried a full install with sudo port -vst install?
  • tested basic functionality of all binary files?

@kencu
Copy link
Contributor

kencu commented Dec 8, 2022

the balign to align thing is not needed -- that was just a misconfiguration on that user's system (PATH was set wrong, so wrong assembler was being used).

@catap
Copy link
Contributor Author

catap commented Dec 8, 2022

@kencu do you mean that clean PowerPC (without MacPorts) can build it?

@kencu
Copy link
Contributor

kencu commented Dec 8, 2022

@kencu do you mean that clean PowerPC (without MacPorts) can build it?

not many builds are tested like that — almost nothing will build on a clean powerpc without macports, esp Tiger. We only use the system toolchain to bootstrap, as you know.

What I mean is the whole balign issue was a mistake, based on that user’s misconfigured system. You can ignore it and revert the balign patch.

@catap
Copy link
Contributor Author

catap commented Dec 8, 2022

@kencu upstream would like to include it :) and asked me to adjust my patch. Here I'd like simple to test it before push to upstream.

@kencu
Copy link
Contributor

kencu commented Dec 9, 2022

Yeah, OK. I guess we are where we are then :>

@kencu
Copy link
Contributor

kencu commented Dec 10, 2022

Attention: it turns out that it is quite possible that "balign" is not the same as "align" in PowerPC assembly.

"balign" refers to "align-to-bytes", ie .balign 8 aligns on an 8-byte boundary.
"p2align" refers to "align-to-power-of-two" ie .p2align 3 aligns on an 8-byte boundary.

"align" was not very specific, and meant different things on different platforms, so compilers moved away from using it.

eg: https://reviews.llvm.org/D16549

but it looks like "align" in PowerPC assembly means "p2align" and not "balign", according to that LLVM review. If so, substituting "balign" for "align" might do the wrong thing.

I don't have the 100% definitive PowerPC assembler manual right at hand -- I do have it somewhere. Looking.

But NB regarding your upstream PR.

As before, the current cctools assembler we use on Tiger appears to support ".balign" just fine, although the ancient ancient ancient system one at /usr/bin/as appears to not support that.

@catap
Copy link
Contributor Author

catap commented Dec 10, 2022

@kencu also, switch from .align to .baling was made a couple years ago: https://unicode-org.atlassian.net/browse/ICU-10305

:)

@kencu
Copy link
Contributor

kencu commented Dec 11, 2022

Yeah, that was the right move, and so it should probably not to "align" at all, but p2align instead.

".balign 16 == .align 4 == .p2align 4"

Or just scrap the plan ;> -- that would be my vote. After all, this was all based on one user's misconfigured system :>

@kencu
Copy link
Contributor

kencu commented Dec 11, 2022

let me try building icu-devel on my "pure" Tiger PPC system without the balign patch. I presume it will build just fine, as ICU built just fine there.

@kencu
Copy link
Contributor

kencu commented Dec 11, 2022

Just to confirm that icu-devel builds just fine with TIger PPC with no balign patch at all (as did icu).

@catap
Copy link
Contributor Author

catap commented Dec 11, 2022

@kencu out of my curiosity: may I ask you to try to build ICU without macports?

@kencu
Copy link
Contributor

kencu commented Dec 11, 2022

actually, never mind that last post… I’m too much of a purist sometimes. Will leave you to it.

@catap catap marked this pull request as draft December 11, 2022 15:40
@catap
Copy link
Contributor Author

catap commented Dec 11, 2022

I've switched it to draft until upstream decided something about this patch

@catap catap closed this Apr 20, 2023
@catap
Copy link
Contributor Author

catap commented Apr 20, 2023

It is irrelevant since the patch was included into upstream and update of ICU delivers it

@catap catap reopened this Apr 20, 2023
@catap catap closed this Apr 20, 2023
@catap catap deleted the icu-devel branch April 20, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants