Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values
On Mon, 18 May 2020 at 12:20, Philippe Mathieu-Daudé wrote: > On 5/18/20 11:46 AM, Peter Maydell wrote: > > Not necessarily a bad idea, but don't we have an awful > > lot of places that ignore the result that we'd need > > to fix first? > > Yes, there are various places to fix. I wanted to have a preview first, > and not start working on this if it is later rejected. Most of the cases > are hardware specific and require studying each hardware behavior. Well, patches that fix "we should check and handle errors but we don't" bugs are pretty uncontroversial. How ugly does the code for "call the function and deliberately ignore the result in a way that doesn't trigger the warning" look ? Assuming it's reasonably straightforward to write code for a device that really does ignore the transaction status then I don't think that there would be a problem with adding the warn-unused-result attributes, if and when we got all the existing instances fixed. The other part of this is really just priorities: it seems likely that a lot of the places which ignore the return code are going to be in devices which we don't care strongly about, so if fixing them all is going to take a long time because we have to look up the details of dozens of obscure bits of hardware, then maybe there's more important cleanup we might prefer to do first. It depends a bit on whether there are 30 of these callsites, or 3... thanks -- PMM
Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values
On 5/18/20 11:46 AM, Peter Maydell wrote: On Sun, 17 May 2020 at 17:48, Philippe Mathieu-Daudé wrote: Various places ignore the MemTxResult indicator of transaction failed. Some cases might be justified (DMA?) while other are probably bugs. To avoid ignoring transaction errors, suggestion is to mark functions returning MemTxResult with warn_unused_result attribute. Not necessarily a bad idea, but don't we have an awful lot of places that ignore the result that we'd need to fix first? Yes, there are various places to fix. I wanted to have a preview first, and not start working on this if it is later rejected. Most of the cases are hardware specific and require studying each hardware behavior.
Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values
On Sun, 17 May 2020 at 17:48, Philippe Mathieu-Daudé wrote: > > Various places ignore the MemTxResult indicator of > transaction failed. Some cases might be justified > (DMA?) while other are probably bugs. To avoid > ignoring transaction errors, suggestion is to mark > functions returning MemTxResult with > warn_unused_result attribute. Not necessarily a bad idea, but don't we have an awful lot of places that ignore the result that we'd need to fix first? thanks -- PMM
Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values
Patchew URL: https://patchew.org/QEMU/20200517164817.5371-1-f4...@amsat.org/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === CC hw/display/bcm2835_fb.o In file included from /tmp/qemu-test/src/hw/core/loader.c:327: /tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf64': /tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result] address_space_write(as ? as : _space_memory, ^~~~ addr, MEMTXATTRS_UNSPECIFIED, --- In file included from /tmp/qemu-test/src/hw/core/loader.c:305: /tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf32': /tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result] address_space_write(as ? as : _space_memory, ^~~~ addr, MEMTXATTRS_UNSPECIFIED, --- data, file_size); /tmp/qemu-test/src/hw/core/loader.c: In function 'rom_reset': /tmp/qemu-test/src/hw/core/loader.c:1149:13: error: ignoring return value of 'address_space_write_rom', declared with attribute warn_unused_result [-Werror=unused-result] address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED, ^~~ rom->data, rom->datasize); ~ cc1: all warnings being treated as errors make: *** [/tmp/qemu-test/src/rules.mak:69: hw/core/loader.o] Error 1 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 664, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=cbc78cc15c714fd48ef89d560cf39ed2', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-3un9zd0v/src/docker-src.2020-05-17-14.15.52.2490:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=cbc78cc15c714fd48ef89d560cf39ed2 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-3un9zd0v/src' make: *** [docker-run-test-mingw@fedora] Error 2 real3m18.237s user0m9.100s The full log is available at http://patchew.org/logs/20200517164817.5371-1-f4...@amsat.org/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values
Patchew URL: https://patchew.org/QEMU/20200517164817.5371-1-f4...@amsat.org/ Hi, This series failed the asan build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1 === TEST SCRIPT END === CC hw/display/ati_2d.o CC hw/display/ati_dbg.o In file included from /tmp/qemu-test/src/hw/core/loader.c:305: /tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result] address_space_write(as ? as : _space_memory, ^~~ In file included from /tmp/qemu-test/src/hw/core/loader.c:327: /tmp/qemu-test/src/include/hw/elf_ops.h:556:21: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result] address_space_write(as ? as : _space_memory, ^~~ /tmp/qemu-test/src/hw/core/loader.c:1149:13: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result] address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED, ^~~ ~~~ 3 errors generated. make: *** [/tmp/qemu-test/src/rules.mak:69: hw/core/loader.o] Error 1 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 664, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=7ba3b942a61f4047b03d411633b03028', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-3dfs_s0u/src/docker-src.2020-05-17-14.08.31.28840:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=7ba3b942a61f4047b03d411633b03028 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-3dfs_s0u/src' make: *** [docker-run-test-debug@fedora] Error 2 real4m49.028s user0m9.839s The full log is available at http://patchew.org/logs/20200517164817.5371-1-f4...@amsat.org/testing.asan/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH 0/2] exec/memory: Enforce checking MemTxResult values
Patchew URL: https://patchew.org/QEMU/20200517164817.5371-1-f4...@amsat.org/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === CC hw/cpu/core.o In file included from /tmp/qemu-test/src/hw/core/loader.c:327:0: /tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf64': /tmp/qemu-test/src/include/hw/elf_ops.h:556:40: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result] address_space_write(as ? as : _space_memory, ^ In file included from /tmp/qemu-test/src/hw/core/loader.c:305:0: /tmp/qemu-test/src/include/hw/elf_ops.h: In function 'load_elf32': /tmp/qemu-test/src/include/hw/elf_ops.h:556:40: error: ignoring return value of 'address_space_write', declared with attribute warn_unused_result [-Werror=unused-result] address_space_write(as ? as : _space_memory, ^ /tmp/qemu-test/src/hw/core/loader.c: In function 'rom_reset': /tmp/qemu-test/src/hw/core/loader.c:1149:36: error: ignoring return value of 'address_space_write_rom', declared with attribute warn_unused_result [-Werror=unused-result] address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED, ^ cc1: all warnings being treated as errors make: *** [hw/core/loader.o] Error 1 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 664, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=f2955b005f4543d8a28b7197af9c37a0', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ov2cj0l6/src/docker-src.2020-05-17-14.03.09.21320:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=f2955b005f4543d8a28b7197af9c37a0 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ov2cj0l6/src' make: *** [docker-run-test-quick@centos7] Error 2 real3m37.618s user0m9.689s The full log is available at http://patchew.org/logs/20200517164817.5371-1-f4...@amsat.org/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[PATCH 0/2] exec/memory: Enforce checking MemTxResult values
Various places ignore the MemTxResult indicator of transaction failed. Some cases might be justified (DMA?) while other are probably bugs. To avoid ignoring transaction errors, suggestion is to mark functions returning MemTxResult with warn_unused_result attribute. Philippe Mathieu-Daudé (2): exec/memory: Let address_space_read/write_cached() propagate MemTxResult exec/memory: Emit warning when MemTxResult is ignored include/exec/memory.h | 50 +++ exec.c| 16 +++--- 2 files changed, 40 insertions(+), 26 deletions(-) -- 2.21.3
