| Message ID | 20171025123056.3165-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | Disassembler patches | expand |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PULL 00/11] Disassembler patches
Type: series
Message-id: 20171025123056.3165-1-richard.henderson@linaro.org
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
t [tag update] patchew/1508006342-5304-1-git-send-email-mark.cave-ayland@ilande.co.uk -> patchew/1508006342-5304-1-git-send-email-mark.cave-ayland@ilande.co.uk
* [new tag] patchew/20171025123056.3165-1-richard.henderson@linaro.org -> patchew/20171025123056.3165-1-richard.henderson@linaro.org
Switched to a new branch 'test'
1b87049c52 disas: Add capstone as submodule
1a149369df disas: Remove monitor_disas_is_physical
6c35e6706c ppc: Support Capstone in disas_set_info
52e10752ca arm: Support Capstone in disas_set_info
4b31f39e81 i386: Support Capstone in disas_set_info
279e046dc1 disas: Support the Capstone disassembler library
4da4345ecf disas: Remove unused flags arguments
9e561441a6 target/arm: Don't set INSN_ARM_BE32 for CONFIG_USER_ONLY
9c25d63ac2 target/arm: Move BE32 disassembler fixup
6836256946 target/ppc: Convert to disas_set_info hook
667b551cc7 target/i386: Convert to disas_set_info hook
=== OUTPUT BEGIN ===
Checking PATCH 1/11: target/i386: Convert to disas_set_info hook...
Checking PATCH 2/11: target/ppc: Convert to disas_set_info hook...
Checking PATCH 3/11: target/arm: Move BE32 disassembler fixup...
ERROR: space prohibited between function name and open parenthesis '('
#46: FILE: disas/arm.c:3824:
+ status = arm_read_memory (addr, (bfd_byte *)b, 2, info);
ERROR: space prohibited between function name and open parenthesis '('
#55: FILE: disas/arm.c:3896:
+ status = arm_read_memory (pc, (bfd_byte *)b, size, info);
ERROR: space prohibited between function name and open parenthesis '('
#64: FILE: disas/arm.c:3913:
+ status = arm_read_memory (pc, (bfd_byte *)b, 4, info);
ERROR: space prohibited between function name and open parenthesis '('
#73: FILE: disas/arm.c:3929:
+ status = arm_read_memory (pc, (bfd_byte *)b, 2, info);
ERROR: code indent should never use tabs
#82: FILE: disas/arm.c:3943:
+^I status = arm_read_memory (pc + 2, (bfd_byte *)b, 2, info);$
ERROR: space prohibited between function name and open parenthesis '('
#82: FILE: disas/arm.c:3943:
+ status = arm_read_memory (pc + 2, (bfd_byte *)b, 2, info);
total: 6 errors, 0 warnings, 107 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/11: target/arm: Don't set INSN_ARM_BE32 for CONFIG_USER_ONLY...
Checking PATCH 5/11: disas: Remove unused flags arguments...
Checking PATCH 6/11: disas: Support the Capstone disassembler library...
Checking PATCH 7/11: i386: Support Capstone in disas_set_info...
Checking PATCH 8/11: arm: Support Capstone in disas_set_info...
Checking PATCH 9/11: ppc: Support Capstone in disas_set_info...
Checking PATCH 10/11: disas: Remove monitor_disas_is_physical...
Checking PATCH 11/11: disas: Add capstone as submodule...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On 25 October 2017 at 13:30, Richard Henderson <richard.henderson@linaro.org> wrote: > Support for Capstone, plus an arm32 fix. > > > r~ > > > The following changes since commit 3d7196d43bfe12efe98568cb60057e273652b99b: > > Merge remote-tracking branch 'remotes/kraxel/tags/usb-20171023-pull-request' into staging (2017-10-24 16:05:57 +0100) > > are available in the git repository at: > > git://github.com/rth7680/qemu.git tags/pull-dis-20171025 > > for you to fetch changes up to 383b90bc6a15f4b18ec34f9c0287b26f9a89fcb8: > > disas: Add capstone as submodule (2017-10-25 11:55:21 +0200) > > ---------------------------------------------------------------- > Capstone disassembler Hi. This failed to build for Windows: make[1]: *** No rule to make target '/home/petmay01/linaro/qemu-for-merges/build/w32-new/capstone/libcapstone.a'. Stop. Makefile:399: recipe for target 'subdir-capstone' failed FreeBSD ar also prints a warning: AR libcapstone.a ar: warning: creating /root/qemu/build/all/capstone/libcapstone.a though the build otherwise succeeds. Is it possible to silence this? (Otherwise it shows up in my build output scripts; I could silence it there but it would be neater if it just wasn't emitted.) thanks -- PMM
On 26 October 2017 at 07:07, Peter Maydell <peter.maydell@linaro.org> wrote:
> Hi. This failed to build for Windows:
Also, after it failed and I backed out the merge, the next thing I
tried to build failed everywhere with:
warning: unable to rmdir capstone: Directory not empty
error: pathspec 'capstone' did not match any file(s) known to git.
error: pathspec 'capstone' did not match any file(s) known to git.
That's bad, because it suggests that bisection is going to break
across this merge commit.
Dan, is this going to be a generic problem with the new submodule
stuff, or is it specific to something with how the capstone
submodule is being handled in this patchset?
thanks
-- PMM
On Thu, Oct 26, 2017 at 07:16:45AM +0100, Peter Maydell wrote: > On 26 October 2017 at 07:07, Peter Maydell <peter.maydell@linaro.org> wrote: > > Hi. This failed to build for Windows: > > Also, after it failed and I backed out the merge, the next thing I > tried to build failed everywhere with: > > warning: unable to rmdir capstone: Directory not empty > error: pathspec 'capstone' did not match any file(s) known to git. > error: pathspec 'capstone' did not match any file(s) known to git. > > That's bad, because it suggests that bisection is going to break > across this merge commit. > > Dan, is this going to be a generic problem with the new submodule > stuff, or is it specific to something with how the capstone > submodule is being handled in this patchset? If you make your checkout go back in time to before the submodule existed, git won't delete your checked out submodule - in case you have commits there you still care about. So it'll print that warning about unable to 'rmdir'. This should be harmless to the build process in general though. I'm not sure what's giving you the 'pathspec' message though ? I would expect anything to ignore the capstone dir - its just like any other untracked file once you go back in time before it was committed. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/26/2017 08:07 AM, Peter Maydell wrote: > On 25 October 2017 at 13:30, Richard Henderson > <richard.henderson@linaro.org> wrote: >> Support for Capstone, plus an arm32 fix. >> >> >> r~ >> >> >> The following changes since commit 3d7196d43bfe12efe98568cb60057e273652b99b: >> >> Merge remote-tracking branch 'remotes/kraxel/tags/usb-20171023-pull-request' into staging (2017-10-24 16:05:57 +0100) >> >> are available in the git repository at: >> >> git://github.com/rth7680/qemu.git tags/pull-dis-20171025 >> >> for you to fetch changes up to 383b90bc6a15f4b18ec34f9c0287b26f9a89fcb8: >> >> disas: Add capstone as submodule (2017-10-25 11:55:21 +0200) >> >> ---------------------------------------------------------------- >> Capstone disassembler > > > Hi. This failed to build for Windows: > > make[1]: *** No rule to make target > '/home/petmay01/linaro/qemu-for-merges/build/w32-new/capstone/libcapstone.a'. > Stop. > Makefile:399: recipe for target 'subdir-capstone' failed Ug. Ok, capstone is forcing the use of .lib for windows. > > FreeBSD ar also prints a warning: > AR libcapstone.a > ar: warning: creating /root/qemu/build/all/capstone/libcapstone.a Feh. Capstone has no ARFLAGS variable. So, no, I can't fix this without actually modifying the sub-makefile. r~
On 26 October 2017 at 08:06, Daniel P. Berrange <berrange@redhat.com> wrote: > I'm not sure what's giving you the 'pathspec' message though ? I would > expect anything to ignore the capstone dir - its just like any other > untracked file once you go back in time before it was committed. Sorry, just realized that was the output of my filtering of the log. Here's what I should have quoted: From git://git.linaro.org/people/pmaydell/qemu-arm + a872ea5...6315472 staging -> pmaydell/staging (forced update) warning: unable to rmdir capstone: Directory not empty error: pathspec 'capstone' did not match any file(s) known to git. Did you forget to 'git add'? make: Entering directory `/home/pm215/qemu/build/all' config-host.mak is out-of-date, running configure GIT ui/keycodemapdb dtc capstone make[1]: *** No rule to make target `all'. Stop. error: pathspec 'capstone' did not match any file(s) known to git. Did you forget to 'git add'? make: *** [git-submodule-update] Error 1 make: *** Waiting for unfinished jobs.... make: *** [subdir-capstone] Error 2 Install prefix /usr/local BIOS directory /usr/local/share/qemu firmware path /usr/local/share/qemu-firmware binary directory /usr/local/bin [rest of configure output skipped] replication support yes VxHS block device no make: Leaving directory `/home/pm215/qemu/build/all' It looks like the git update script thinks that there ought to be a 'capstone' submodule, which of course there isn't any more, so it barfs trying to update it. thanks -- PMM
On 10/26/2017 09:21 AM, Peter Maydell wrote: > On 26 October 2017 at 08:06, Daniel P. Berrange <berrange@redhat.com> wrote: >> I'm not sure what's giving you the 'pathspec' message though ? I would >> expect anything to ignore the capstone dir - its just like any other >> untracked file once you go back in time before it was committed. > > Sorry, just realized that was the output of my filtering of the > log. Here's what I should have quoted: > > From git://git.linaro.org/people/pmaydell/qemu-arm > + a872ea5...6315472 staging -> pmaydell/staging (forced update) > warning: unable to rmdir capstone: Directory not empty > error: pathspec 'capstone' did not match any file(s) known to git. > Did you forget to 'git add'? > make: Entering directory `/home/pm215/qemu/build/all' > config-host.mak is out-of-date, running configure > GIT ui/keycodemapdb dtc capstone > make[1]: *** No rule to make target `all'. Stop. > error: pathspec 'capstone' did not match any file(s) known to git. > Did you forget to 'git add'? > make: *** [git-submodule-update] Error 1 > make: *** Waiting for unfinished jobs.... > make: *** [subdir-capstone] Error 2 Looks like we need to make git-submodule-update depend on config-host.mak, so that we have already re-run configure, so that vanishing modules will have been de-selected. Forcing a configure re-run by hand would have masked this problem. I think all of the testing that I have been doing has been builds from empty build directories. r~
On Thu, Oct 26, 2017 at 08:21:48AM +0100, Peter Maydell wrote: > On 26 October 2017 at 08:06, Daniel P. Berrange <berrange@redhat.com> wrote: > > I'm not sure what's giving you the 'pathspec' message though ? I would > > expect anything to ignore the capstone dir - its just like any other > > untracked file once you go back in time before it was committed. > > Sorry, just realized that was the output of my filtering of the > log. Here's what I should have quoted: > > From git://git.linaro.org/people/pmaydell/qemu-arm > + a872ea5...6315472 staging -> pmaydell/staging (forced update) > warning: unable to rmdir capstone: Directory not empty > error: pathspec 'capstone' did not match any file(s) known to git. > Did you forget to 'git add'? > make: Entering directory `/home/pm215/qemu/build/all' > config-host.mak is out-of-date, running configure > GIT ui/keycodemapdb dtc capstone > make[1]: *** No rule to make target `all'. Stop. > error: pathspec 'capstone' did not match any file(s) known to git. > Did you forget to 'git add'? > make: *** [git-submodule-update] Error 1 > make: *** Waiting for unfinished jobs.... > make: *** [subdir-capstone] Error 2 > Install prefix /usr/local > BIOS directory /usr/local/share/qemu > firmware path /usr/local/share/qemu-firmware > binary directory /usr/local/bin > [rest of configure output skipped] > replication support yes > VxHS block device no > make: Leaving directory `/home/pm215/qemu/build/all' > > It looks like the git update script thinks that there ought to > be a 'capstone' submodule, which of course there isn't any more, > so it barfs trying to update it. Yeah, ok that makes more sense. The 'config-host.mak' rules have the list of desired submodules and those correspond to the state when you ran configure with the patches applied. Do we really expect make/configure todo the right thing when going backwards in time ? I've always assumed that if you go back in time when you need to do a 'git clean -f -x d' and re-run configure from scratch. Certainly in the past various makefile changes in QEMU would break, or silently not correctly recompile stuff when going backwards in time. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/26/2017 07:06 AM, Daniel P. Berrange wrote: > Do we really expect make/configure todo the right thing when going > backwards in time ? "Yes"? Ideally at least :) > I've always assumed that if you go back in time > when you need to do a 'git clean -f -x d' and re-run configure from> scratch. I certainly don't do that and would rather not have to think about it. I also don't think about calling it when bisecting, however looking at the man page, this is somehow suggested in the example: · Automatically bisect with temporary modifications (hot-fix): [...] # undo the tweak to allow clean flipping to the next commit git reset --hard This could be a warning displayed via a post-checkout hook, for this particular merge hash, or if you go way too back in time... > Certainly in the past various makefile changes in QEMU would> break, or silently not correctly recompile stuff when going backwards > in time. Travis is already taking too long, but we could add a such test (merge/build/checkout backward/configure/quick build), not sure if we gain much.
On Thu, Oct 26, 2017 at 10:29:59AM -0300, Philippe Mathieu-Daudé wrote: > On 10/26/2017 07:06 AM, Daniel P. Berrange wrote: > > Do we really expect make/configure todo the right thing when going > > backwards in time ? > > "Yes"? Ideally at least :) What we could do is get scripts/git-submodule.sh to filter the list of modules it receives, against the list of modules that exist. This would probably make it safe against going backwards over a commit that added a new submodule Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 26 October 2017 at 11:06, Daniel P. Berrange <berrange@redhat.com> wrote: > Do we really expect make/configure todo the right thing when going > backwards in time ? I've always assumed that if you go back in time > when you need to do a 'git clean -f -x d' and re-run configure from > scratch. Certainly in the past various makefile changes in QEMU would > break, or silently not correctly recompile stuff when going backwards > in time. Yes. Occasionally we accidentally break this, as you note, and then git bisection across that kind of boundary requires a painful build from clean. But almost always bisection (across small spans of history) works without having to do that, as do things like "check out this branch I was working on a month ago and do a build without bothering to clean first". In this case you broke my standard workflow for rolling back from an attempted merge that didn't actually build, which IME is a pretty rare thing to have go wrong. thanks -- PMM
On 10/25/2017 02:30 PM, Richard Henderson wrote: > Support for Capstone, plus an arm32 fix. > > ---------------------------------------------------------------- > Capstone disassembler > > ---------------------------------------------------------------- > Richard Henderson (11): > target/i386: Convert to disas_set_info hook > target/ppc: Convert to disas_set_info hook > target/arm: Move BE32 disassembler fixup > target/arm: Don't set INSN_ARM_BE32 for CONFIG_USER_ONLY > disas: Remove unused flags arguments > disas: Support the Capstone disassembler library > i386: Support Capstone in disas_set_info > arm: Support Capstone in disas_set_info > ppc: Support Capstone in disas_set_info > disas: Remove monitor_disas_is_physical > disas: Add capstone as submodule I think (but haven't bisected) that this series is now making 'make' noisy: $ make -j3 qemu-nbd make[1]: '/home/eblake/qemu-tmp/capstone/libcapstone.a' is up to date. make: 'qemu-nbd' is up to date. Can we figure out how to shut up make when libcapstone.a has nothing needed? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Support for Capstone, plus an arm32 fix. r~ The following changes since commit 3d7196d43bfe12efe98568cb60057e273652b99b: Merge remote-tracking branch 'remotes/kraxel/tags/usb-20171023-pull-request' into staging (2017-10-24 16:05:57 +0100) are available in the git repository at: git://github.com/rth7680/qemu.git tags/pull-dis-20171025 for you to fetch changes up to 383b90bc6a15f4b18ec34f9c0287b26f9a89fcb8: disas: Add capstone as submodule (2017-10-25 11:55:21 +0200) ---------------------------------------------------------------- Capstone disassembler ---------------------------------------------------------------- Richard Henderson (11): target/i386: Convert to disas_set_info hook target/ppc: Convert to disas_set_info hook target/arm: Move BE32 disassembler fixup target/arm: Don't set INSN_ARM_BE32 for CONFIG_USER_ONLY disas: Remove unused flags arguments disas: Support the Capstone disassembler library i386: Support Capstone in disas_set_info arm: Support Capstone in disas_set_info ppc: Support Capstone in disas_set_info disas: Remove monitor_disas_is_physical disas: Add capstone as submodule Makefile | 15 ++ include/disas/bfd.h | 11 +- include/disas/capstone.h | 38 ++++++ include/disas/disas.h | 4 +- include/exec/log.h | 4 +- disas.c | 308 ++++++++++++++++++++++++++++++------------ disas/arm.c | 21 ++- monitor.c | 29 +--- target/alpha/translate.c | 2 +- target/arm/cpu.c | 49 +++---- target/arm/translate-a64.c | 3 +- target/arm/translate.c | 3 +- target/cris/translate.c | 3 +- target/hppa/translate.c | 2 +- target/i386/cpu.c | 19 +++ target/i386/translate.c | 8 +- target/lm32/translate.c | 2 +- target/m68k/translate.c | 2 +- target/microblaze/translate.c | 2 +- target/mips/translate.c | 2 +- target/nios2/translate.c | 2 +- target/openrisc/translate.c | 2 +- target/ppc/translate.c | 5 +- target/ppc/translate_init.c | 27 ++++ target/s390x/translate.c | 2 +- target/sh4/translate.c | 2 +- target/sparc/translate.c | 2 +- target/tricore/translate.c | 2 +- target/unicore32/translate.c | 2 +- target/xtensa/translate.c | 2 +- .gitmodules | 3 + capstone | 1 + configure | 64 +++++++++ 33 files changed, 459 insertions(+), 184 deletions(-) create mode 100644 include/disas/capstone.h create mode 160000 capstone