-
Notifications
You must be signed in to change notification settings - Fork 12
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
implement __aligned_malloc() and __aligned_free() #59
Conversation
e0700ed
to
8bfd5e7
Compare
Be careful with this. Plan out your LE 3.1 migration strategy and include it in the changes. Consider these points:
|
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.
LGTM
Thanks. I've removed the comment; another issue is someone already using those new |
You should still put in support for LE 3.1. My comment wasn't based on the FIXME comment. It was just a general observation and experience with these kinds of features. Subtle things can cause problems down the road. The nice thing here is you can put the 3.1 support in and test the various combinations. |
Let's say we have a |
I'd either put a #if in the header to map __align_malloc to posix_memalign or change the implementation in the cpp file to use posix_memalign instead of the hack if targeting LE 3.1. You can tell if you are targeting LE 3.1 by including features.h and then checking the TARGET_LIB macro. I think both of these are safe. Test them to make sure. |
posix_memalign() has different args and return value, as documented here, and requires |
I'd change the parameter list for __aligned_malloc() to match posix_memalign() so it acts as a replacement. This will also make it easier for people to switch between the two. And on LE 3.1, you would make __aligned_free() call free(). |
Call posix_memalign() if building with `-mzos-target=zosv3r1` on z/OS 3.1, in which case `__aligned_free()` calls `free()`. Otherwise use a local implementation to align memory from `malloc()`.
On LE 3.1 zos-io.cc fails to pick up LOCK_* definitions from the system's sys/file.h.
__clockid_t is defined in sys/types.h if >= v2r5, CLOCK_REALTIME and CLOCK_MONOTONIC are already defined in the system's time.h if >= v2r5.
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.
Looks good. I made one minor nit about features.h
src/zos.cc
Outdated
@@ -26,6 +25,7 @@ | |||
#include <dlfcn.h> | |||
#include <errno.h> | |||
#include <fcntl.h> | |||
#include <features.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.
Since you included other system headers you don't need to include this one directly.
Also remove unneeded include of features.h since other system headers already include it.
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.
LGTM
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.
LGTM, thanks for adding the time.h fixes
This is for the local implementation to behave the same way in terms of the argument values and return.
I made minor change after you approved, please review the last 2 commits. Thanks. |
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.
LGTM
@@ -3104,10 +3104,16 @@ extern "C" void *__aligned_malloc(size_t size, size_t alignment) { | |||
return ptr; | |||
return nullptr; | |||
#else | |||
if (size == 0) |
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.
This might give different behaviour from malloc(0) or posix_memalign(&ptr, align, 0).
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.
According to posix_memalign on z/OS: "If size was specified to 0, posix_memalign() returns zero and sets memptr to a null pointer."
-mzos-target=zosv3r1
on z/OS 3.1, in which case__aligned_free()
callsfree()
; otherwise use a local implementation to align memory frommalloc()