On Mon, 8 Jul 2024 at 14:24, junjiehua <halouwo...@gmail.com> wrote:
>
> when building elf2dump with x86_64-w64-mingw32-gcc, fwrite is imported from
> msvcrt.dll. However, the implementation of msvcrt.dll!fwrite is buggy:
> it enters an infinite loop when the size of a single write exceeds 4GB.
> This patch addresses the issue by splitting large physical memory
> blocks into smaller chunks.
Hi; thanks for this patch.
(Does the library fwrite fail for > 4GB, or for >= 4GB ?)
> Signed-off-by: junjiehua <junjie...@tencent.com>
> ---
> contrib/elf2dmp/main.c | 19 +++++++++++++++----
> 1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c
> index d046a72ae6..1994553d95 100644
> --- a/contrib/elf2dmp/main.c
> +++ b/contrib/elf2dmp/main.c
> @@ -23,6 +23,8 @@
> #define INITIAL_MXCSR 0x1f80
> #define MAX_NUMBER_OF_RUNS 42
>
> +#define MAX_CHUNK_SIZE (128 * 1024 * 1024)
I think we could add a comment here, something like:
/*
* Maximum size to fwrite() to the output file at once;
* the MSVCRT runtime will not correctly handle fwrite()
* of more than 4GB at once.
*/
That will act as a reminder about why we do it.
(Does the library fwrite fail for > 4GB, or for >= 4GB ?
Your commit message says the former, so I've gone with that,
but if it's an "overflows 32 bit variable" kind of bug then
4GB exactly probably also doesn't work.)
Is there a particular reason to use 128MB here? If the
runtime only fails on 4GB or more, maybe we should use
a larger MAX_CHUNK_SIZE, like 2GB ?
> +
> typedef struct idt_desc {
> uint16_t offset1; /* offset bits 0..15 */
> uint16_t selector;
> @@ -434,13 +436,22 @@ static bool write_dump(struct pa_space *ps,
>
> for (i = 0; i < ps->block_nr; i++) {
> struct pa_block *b = &ps->block[i];
> + size_t offset = 0;
> + size_t chunk_size;
>
> printf("Writing block #%zu/%zu of %"PRIu64" bytes to file...\n", i,
> ps->block_nr, b->size);
> - if (fwrite(b->addr, b->size, 1, dmp_file) != 1) {
> - eprintf("Failed to write block\n");
> - fclose(dmp_file);
> - return false;
> +
> + while (offset < b->size) {
> + chunk_size = (b->size - offset > MAX_CHUNK_SIZE)
> + ? MAX_CHUNK_SIZE
> + : (b->size - offset);
You can write this as
chunk_size = MIN(b->size - offset, MAX_CHUNK_SIZE);
which I think is clearer. (Our osdep header provides MIN().)
> + if (fwrite(b->addr + offset, chunk_size, 1, dmp_file) != 1) {
> + eprintf("Failed to write block\n");
> + fclose(dmp_file);
> + return false;
> + }
> + offset += chunk_size;
I think we should abstract out the bug workaround into a
separate function, with the same API as fwrite(). Call
it do_fwrite() or something, and make all the fwrite()
calls use it. I know at the moment there's only two of
them, and one of them is the header so never 4GB, but
I think this more cleanly separates out the "work around
a runtime library problem" part from the main logic of
the program, and will mean that if we ever need to rearrange
how we write out the data in future it will be simple.
thanks
-- PMM
