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 crash in HasKeyBag on SPARC Solaris 11 #2196

Merged
merged 3 commits into from
Feb 20, 2018

Conversation

dimpase
Copy link
Member

@dimpase dimpase commented Feb 19, 2018

hopefully the compiler is clever enough to optimise this away

hopefully the compiler is clever enough to optimise this away
Copy link
Member

@fingolfin fingolfin 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 OK in principle to me, however, it would be even better if there was an explanatory comment here. Something like this:

The pointer p may not be aligned, which means that directly writing to it can incur a major performance penalty or even trigger a segfault on certain architectures (e.g. ARM, SPARC). Thus we use memcpy here, with the implicit hope that on archs which don't need this, the compiler will optimize it back into a direct copy (verified to happen with GCC and clang on x86_64)

@dimpase
Copy link
Member Author

dimpase commented Feb 20, 2018

Should I amend the commit to include the paragraph suggested above?
Perhaps it's easier that whoever merges this adds this into the commit message.

@fingolfin
Copy link
Member

This comment really should be in the code, not the commit message (where it is invisible when reading the code)

@dimpase
Copy link
Member Author

dimpase commented Feb 20, 2018

This comment really should be in the code

OK, done.

src/intfuncs.c Outdated
@@ -307,10 +307,18 @@ static inline uint64_t rotl64 ( uint64_t x, int8_t r )
//-----------------------------------------------------------------------------
// Block read - if your platform needs to do endian-swapping or can only
// handle aligned reads, do the conversion here
//
// The pointer p may not be aligned, which means that directly writing to it can
Copy link
Contributor

Choose a reason for hiding this comment

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

very picky, it's "reading it", not "writing to it"

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, fixed.

@ChrisJefferson ChrisJefferson merged commit 16f7cc7 into gap-system:master Feb 20, 2018
@fingolfin fingolfin changed the title always use memcpy to enforce alignment in getblock8 Fix crash HasKeyBag on SPARC Solaris 11 Mar 28, 2018
@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) labels Mar 28, 2018
@fingolfin fingolfin changed the title Fix crash HasKeyBag on SPARC Solaris 11 Fix crash in HasKeyBag on SPARC Solaris 11 Mar 28, 2018
@fingolfin fingolfin added the release notes: added PRs introducing changes that have since been mentioned in the release notes label Mar 28, 2018
@dimpase dimpase deleted the alwaysmemcpy branch March 31, 2018 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: bug: crash Issues describing bugs that cause GAP to crash, and PRs fixing them (used for release notes) kind: bug Issues describing general bugs, and PRs fixing them release notes: added PRs introducing changes that have since been mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants