| Message ID | 20220226180723.1706285-5-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On 2/26/22 08:07, Peter Maydell wrote: > Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert. > Instead return NULL; this is in line with the posix_memalign() API, > and is valid to pass to _aligned_free() (which will do nothing). > > This change is a preparation for sharing the qemu_try_memalign() > code between Windows and POSIX -- at the moment only the Windows > version has the assert that size != 0. > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- > util/oslib-win32.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) That would be my fault, I believe. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 2/26/22 08:07, Peter Maydell wrote: > Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert. > Instead return NULL; this is in line with the posix_memalign() API, > and is valid to pass to _aligned_free() (which will do nothing). > > This change is a preparation for sharing the qemu_try_memalign() > code between Windows and POSIX -- at the moment only the Windows > version has the assert that size != 0. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > util/oslib-win32.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > index 05857414695..8c1c64719d7 100644 > --- a/util/oslib-win32.c > +++ b/util/oslib-win32.c > @@ -48,13 +48,16 @@ void *qemu_try_memalign(size_t alignment, size_t size) > { > void *ptr; > > - g_assert(size != 0); > if (alignment < sizeof(void *)) { > alignment = sizeof(void *); > } else { > g_assert(is_power_of_2(alignment)); > } > - ptr = _aligned_malloc(size, alignment); > + if (size) { > + ptr = _aligned_malloc(size, alignment); > + } else { > + ptr = NULL; > + } Oh, should we set errno to something here? Otherwise a random value will be used by qemu_memalign. r~ > trace_qemu_memalign(alignment, size, ptr); > return ptr; > }
On Sun, 27 Feb 2022 at 00:56, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/26/22 08:07, Peter Maydell wrote: > > Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert. > > Instead return NULL; this is in line with the posix_memalign() API, > > and is valid to pass to _aligned_free() (which will do nothing). > > > > This change is a preparation for sharing the qemu_try_memalign() > > code between Windows and POSIX -- at the moment only the Windows > > version has the assert that size != 0. > > > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > > --- > > util/oslib-win32.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/util/oslib-win32.c b/util/oslib-win32.c > > index 05857414695..8c1c64719d7 100644 > > --- a/util/oslib-win32.c > > +++ b/util/oslib-win32.c > > @@ -48,13 +48,16 @@ void *qemu_try_memalign(size_t alignment, size_t size) > > { > > void *ptr; > > > > - g_assert(size != 0); > > if (alignment < sizeof(void *)) { > > alignment = sizeof(void *); > > } else { > > g_assert(is_power_of_2(alignment)); > > } > > - ptr = _aligned_malloc(size, alignment); > > + if (size) { > > + ptr = _aligned_malloc(size, alignment); > > + } else { > > + ptr = NULL; > > + } > > Oh, should we set errno to something here? > Otherwise a random value will be used by qemu_memalign. Yeah, I guess so, though the errno to use isn't obvious. Maybe EINVAL? The alternative would be to try to audit all the callsites to confirm they don't ever try to allocate 0 bytes and then have the assert for both Windows and POSIX versions... -- PMM
On 2/27/22 02:54, Peter Maydell wrote: >>> + if (size) { >>> + ptr = _aligned_malloc(size, alignment); >>> + } else { >>> + ptr = NULL; >>> + } >> >> Oh, should we set errno to something here? >> Otherwise a random value will be used by qemu_memalign. > > Yeah, I guess so, though the errno to use isn't obvious. Maybe EINVAL? > > The alternative would be to try to audit all the callsites to > confirm they don't ever try to allocate 0 bytes and then have > the assert for both Windows and POSIX versions... Alternately, force size == 1, so that we always get a non-NULL value that can be freed. That's a change on the POSIX side as well, of course. r~
On Sun, 27 Feb 2022 at 18:36, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 2/27/22 02:54, Peter Maydell wrote: > >>> + if (size) { > >>> + ptr = _aligned_malloc(size, alignment); > >>> + } else { > >>> + ptr = NULL; > >>> + } > >> > >> Oh, should we set errno to something here? > >> Otherwise a random value will be used by qemu_memalign. > > > > Yeah, I guess so, though the errno to use isn't obvious. Maybe EINVAL? > > > > The alternative would be to try to audit all the callsites to > > confirm they don't ever try to allocate 0 bytes and then have > > the assert for both Windows and POSIX versions... > > Alternately, force size == 1, so that we always get a non-NULL value that can be freed. > That's a change on the POSIX side as well, of course. Yes, I had a look at what actual malloc() implementations tend to do, and the answer seems to be that forcing size to 1 gives less weird behaviour for the application. So here that would be if (size == 0) { size++; } ptr = _aligned_malloc(size, alignment); We don't need to do anything on the POSIX side (unless we want to enforce consistency of handling the size==0 case). I'd quite like to get this series in before softfreeze (though mostly just for my personal convenience so it's not hanging around as a loose end I have to come back to after we reopen for 7.1). Does anybody object if I squash in that change and put this in a pullrequest, or would you prefer to see a v2 series first? thanks -- PMM
On 3/3/22 06:55, Peter Maydell wrote: >> Alternately, force size == 1, so that we always get a non-NULL value that can be freed. >> That's a change on the POSIX side as well, of course. > > Yes, I had a look at what actual malloc() implementations tend > to do, and the answer seems to be that forcing size to 1 gives > less weird behaviour for the application. So here that would be > > if (size == 0) { > size++; > } > ptr = _aligned_malloc(size, alignment); > > We don't need to do anything on the POSIX side (unless we want to > enforce consistency of handling the size==0 case). I would do this unconditionally. The POSIX manpage says that either NULL or a unique pointer is a valid return value into *memptr here for size == 0. What we want in our caller is NULL if and only if error. > I'd quite like to get this series in before softfreeze (though mostly > just for my personal convenience so it's not hanging around as a > loose end I have to come back to after we reopen for 7.1). Does anybody > object if I squash in that change and put this in a pullrequest, > or would you prefer to see a v2 series first? I'm happy with a squash and PR. r~
On Thu, 3 Mar 2022 at 23:02, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 3/3/22 06:55, Peter Maydell wrote: > >> Alternately, force size == 1, so that we always get a non-NULL value that can be freed. > >> That's a change on the POSIX side as well, of course. > > > > Yes, I had a look at what actual malloc() implementations tend > > to do, and the answer seems to be that forcing size to 1 gives > > less weird behaviour for the application. So here that would be > > > > if (size == 0) { > > size++; > > } > > ptr = _aligned_malloc(size, alignment); > > > > We don't need to do anything on the POSIX side (unless we want to > > enforce consistency of handling the size==0 case). > > I would do this unconditionally. The POSIX manpage says that either NULL or a unique > pointer is a valid return value into *memptr here for size == 0. What we want in our > caller is NULL if and only if error. Mm, I guess. I was trying to avoid changing the POSIX-side behaviour, but this seems safe enough. -- PMM
diff --git a/util/oslib-win32.c b/util/oslib-win32.c index 05857414695..8c1c64719d7 100644 --- a/util/oslib-win32.c +++ b/util/oslib-win32.c @@ -48,13 +48,16 @@ void *qemu_try_memalign(size_t alignment, size_t size) { void *ptr; - g_assert(size != 0); if (alignment < sizeof(void *)) { alignment = sizeof(void *); } else { g_assert(is_power_of_2(alignment)); } - ptr = _aligned_malloc(size, alignment); + if (size) { + ptr = _aligned_malloc(size, alignment); + } else { + ptr = NULL; + } trace_qemu_memalign(alignment, size, ptr); return ptr; }
Currently if qemu_try_memalign() is asked to allocate 0 bytes, we assert. Instead return NULL; this is in line with the posix_memalign() API, and is valid to pass to _aligned_free() (which will do nothing). This change is a preparation for sharing the qemu_try_memalign() code between Windows and POSIX -- at the moment only the Windows version has the assert that size != 0. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- util/oslib-win32.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)