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

Require __BYTE_ORDER__ to be defined for bi-endian target architectures. #1885

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

briansmith
Copy link
Owner

No description provided.

@briansmith briansmith self-assigned this Jan 9, 2024
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c4742e0) 96.02% compared to head (39db572) 96.02%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1885   +/-   ##
=======================================
  Coverage   96.02%   96.02%           
=======================================
  Files         136      136           
  Lines       20776    20776           
  Branches      226      226           
=======================================
  Hits        19950    19950           
  Misses        792      792           
  Partials       34       34           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@briansmith
Copy link
Owner Author

briansmith commented Jan 9, 2024

cc @uweigand @erichte-ibm @pkubaj. The basic idea here is to ensure that __BYTE_ORDER__ is defined properly since @uweigand's contribution to crypto/internal.h requires __BYTE_ORDER__ to be defined correctly, and to avoid using any other indication of (big-)endianness.

Copy link
Contributor

@ecnelises ecnelises left a comment

Choose a reason for hiding this comment

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

maybe off-topic but isn't mipsel little-endian?

Copy link
Contributor

@DavidHorton5339 DavidHorton5339 left a comment

Choose a reason for hiding this comment

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

LGTM - though please see suggestion about comment.

include/ring-core/target.h Outdated Show resolved Hide resolved
@DavidHorton5339
Copy link
Contributor

maybe off-topic but isn't mipsel little-endian?

MIPS is bi-endian, so although mipsel is the little-endian variant, OPENSSL_MIPS is insufficient on it own to uniquely identify the target elsewhere in the code. That's why BYTE_ORDER is needed too, and the purpose of this PR is to make sure that is set for bi-endian target architectures where this uncertainty exists.

Perhaps it would be clearer to prepend 'Require BYTE_ORDER to be defined for bi-endian target architectures.' to the comment block starting on line 45, and to replace 'big-endianness' with 'endianness'.

@briansmith
Copy link
Owner Author

replace 'big-endianness' with 'endianness'.

I implemented this suggestion.

@briansmith briansmith merged commit c72a5aa into main Jan 10, 2024
140 checks passed
@briansmith briansmith deleted the b/endian branch January 10, 2024 18:53
@briansmith
Copy link
Owner Author

Thanks for reviewing this. I really appreciate it.

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.

3 participants