Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
On Thu, 3 Mar 2022 at 23:02, Richard Henderson
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
Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
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~
Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
On Sun, 27 Feb 2022 at 18:36, Richard Henderson
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
Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
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~
Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
On Sun, 27 Feb 2022 at 00:56, Richard Henderson
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
> > ---
> > 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
Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
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
---
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;
}
Re: [PATCH 4/9] util/oslib-win32: Return NULL on qemu_try_memalign() with zero size
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 --- util/oslib-win32.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) That would be my fault, I believe. Reviewed-by: Richard Henderson r~
