-
Notifications
You must be signed in to change notification settings - Fork 101
Kitkat fix #9
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
Kitkat fix #9
Conversation
patches/jsc.patch
Outdated
| #include "DebugHeap.h" | ||
| #include "BAssert.h" | ||
| #include "BPlatform.h" | ||
| +#include <posix_memalign.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are patching this I'd actually prefer if we just remove the place where posix_memalign is being called and add an assert(false) there to make sure the app would crash when it gets to that place (we claim that this code is not being used as it is only used when JSC debugging is enabled)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we just implement posix_memalign as assert(false)?
| #endif | ||
|
|
||
| #endif // !__LP64__ | ||
| #include_next <math.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add small comment explaning what's going on with this file
kmagiera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left few comments inline, can you take a look @ukasiu ?
|
Id like to avoid issues in the future when we update JSC and it may have
other references to posix_memalign. I'd prefer in that case to learn about
that during compilation not in the runtime.
Now we need to patch file that uses posix_memalign anyways, so it doesn't
make any difference if we replace me malign call with assert(false) but in
my opinion it makes our intent more clear to understand
|
fixes #8