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

Add 32-bit big-endian PowerPC support #1676

Merged
merged 3 commits into from
Oct 2, 2023
Merged

Add 32-bit big-endian PowerPC support #1676

merged 3 commits into from
Oct 2, 2023

Conversation

briansmith
Copy link
Owner

No description provided.

@briansmith briansmith self-assigned this Oct 1, 2023
@briansmith
Copy link
Owner Author

@erichte-ibm PTAL.

@briansmith
Copy link
Owner Author

error[E0463]: can't find crate for `profiler_builtins`
  |
  = note: the compiler may have been built without the profiler runtime

We need the change to rust-lang/rust analogous to rust-lang/rust#105389 for this target and probably for the 64-bit big-endian version too.

@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #1676 (13650a3) into main (baa823b) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1676      +/-   ##
==========================================
+ Coverage   95.98%   95.99%   +0.01%     
==========================================
  Files         134      134              
  Lines       19953    19953              
  Branches      199      199              
==========================================
+ Hits        19151    19153       +2     
+ Misses        765      763       -2     
  Partials       37       37              
Files Coverage Δ
src/lib.rs 34.61% <ø> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@briansmith
Copy link
Owner Author

@runlevel5 Perhaps you know who should look at this?

@briansmith briansmith merged commit 3878b1b into main Oct 2, 2023
190 checks passed
@briansmith briansmith deleted the b/ppc branch October 2, 2023 01:25
@runlevel5
Copy link

@runlevel5 Perhaps you know who should look at this?

@classilla I am wondering if you could help Brian review this Pull Request? Many thanks in advance

@pkubaj
Copy link
Contributor

pkubaj commented Oct 2, 2023

This commit probably introduces misdetection of powerpc64 (big-endian). The reason is that __powerpc__ is defined on both 64-bits and 32-bits. Since you check for __powerpc__ before checking for __powerpc64__ it will go first. The solution would be to explicitly check #if defined(__powerpc__) && !defined(__powerpc64__) or simply move the __powerpc__ check below __powerpc64__.

@pkubaj
Copy link
Contributor

pkubaj commented Oct 2, 2023

Also, this line:

#elif (defined(__PPC64__) || defined(__powerpc64__)) && \
      (defined(_LITTLE_ENDIAN) || defined(_BIG_ENDIAN))

It's not necessary to check endianness here, there were no middle-endian PPC and I doubt ring even supports middle-endian :)

@classilla
Copy link

What @pkubaj said. Please note that I have relatively few 32-bit systems now running anything other than old versions of Mac OS or Mac OS X.

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