| Message ID | 20170718061005.29518-20-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
Hi On Mon, Jul 17, 2017 at 11:09 PM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > Applied using the Coccinelle semantic patch scripts/coccinelle/use_osdep.cocci > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > > There is no entry for this file in MAINTAINERS, should it go under TCG/Overall? That would make sense to me. > > disas.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/disas.c b/disas.c > index d335c55bbf..8b59448286 100644 > --- a/disas.c > +++ b/disas.c > @@ -151,7 +151,7 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info *info, > info->read_memory_func(pc, buf, n, info); > > for (i = 0; i < n; ++i) { > - if (i % 32 == 0) { > + if (QEMU_IS_ALIGNED(i, 32)) { > info->fprintf_func(info->stream, "\n%s: ", prefix); > } > info->fprintf_func(info->stream, "%02x", buf[i]); > -- > 2.13.2 > >
18.07.2017 09:09, Philippe Mathieu-Daudé wrote: > @@ -151,7 +151,7 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info *info, > info->read_memory_func(pc, buf, n, info); > > for (i = 0; i < n; ++i) { > - if (i % 32 == 0) { > + if (QEMU_IS_ALIGNED(i, 32)) { > info->fprintf_func(info->stream, "\n%s: ", prefix); > } > info->fprintf_func(info->stream, "%02x", buf[i]); This does not seem to be related to _alignment_ per se, it is just formatting, 32 entries per line, so that output looks more-or less compact and stays readable. To me it is a strange move, not so much logical. It doesn't matter much either way, but with IS_ALIGNED, to me at least, it looks a bit less logical than now. Thanks, /mjt
On 18.07.2017 13:07, Marc-André Lureau wrote: > Hi > > On Mon, Jul 17, 2017 at 11:09 PM, Philippe Mathieu-Daudé > <f4bug@amsat.org> wrote: >> Applied using the Coccinelle semantic patch scripts/coccinelle/use_osdep.cocci [...] >> diff --git a/disas.c b/disas.c >> index d335c55bbf..8b59448286 100644 >> --- a/disas.c >> +++ b/disas.c >> @@ -151,7 +151,7 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info *info, >> info->read_memory_func(pc, buf, n, info); >> >> for (i = 0; i < n; ++i) { >> - if (i % 32 == 0) { >> + if (QEMU_IS_ALIGNED(i, 32)) { >> info->fprintf_func(info->stream, "\n%s: ", prefix); >> } >> info->fprintf_func(info->stream, "%02x", buf[i]); This looks wrong to me. QEMU_IS_ALIGNED should be used for addresses and similar things. This part here is about pretty printing a hex dump. The code should likely be converted to use qemu_hexdump() instead, I guess. Thomas
On 18/07/2017 16:43, Thomas Huth wrote: > On 18.07.2017 13:07, Marc-André Lureau wrote: >> Hi >> >> On Mon, Jul 17, 2017 at 11:09 PM, Philippe Mathieu-Daudé >> <f4bug@amsat.org> wrote: >>> Applied using the Coccinelle semantic patch scripts/coccinelle/use_osdep.cocci > [...] >>> diff --git a/disas.c b/disas.c >>> index d335c55bbf..8b59448286 100644 >>> --- a/disas.c >>> +++ b/disas.c >>> @@ -151,7 +151,7 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info *info, >>> info->read_memory_func(pc, buf, n, info); >>> >>> for (i = 0; i < n; ++i) { >>> - if (i % 32 == 0) { >>> + if (QEMU_IS_ALIGNED(i, 32)) { >>> info->fprintf_func(info->stream, "\n%s: ", prefix); >>> } >>> info->fprintf_func(info->stream, "%02x", buf[i]); > > This looks wrong to me. QEMU_IS_ALIGNED should be used for addresses and > similar things. This part here is about pretty printing a hex dump. > > The code should likely be converted to use qemu_hexdump() instead, I guess. qemu_hexdump only works with FILE*. But I agree that QEMU_IS_ALIGNED looks weird here. I think it should mostly be used when the argument is a pointer, to hide the cast. Uses for non-pointer arguments should be decided on a one-by-one basis; "is the first argument an address" is definitely a good thing to consider. Another might be "is the second argument a variable", because "i % n == 0" is less efficient than "(i & (n - 1)) == 0". Paolo Paolo
On 07/23/2017 09:52 AM, Paolo Bonzini wrote: > > But I agree that QEMU_IS_ALIGNED looks weird here. I think it should > mostly be used when the argument is a pointer, to hide the cast. Uses > for non-pointer arguments should be decided on a one-by-one basis; "is > the first argument an address" is definitely a good thing to consider. > Another might be "is the second argument a variable", because "i % n == > 0" is less efficient than "(i & (n - 1)) == 0". How often is QEMU_IS_ALIGNED used on non-power-of-2? Would it be worth rewriting it into bit-wise ops, at the expense of declaring that any code aligning to other values must open-code their own %? Can we have the compiler help us ensure that the second argument is a power-of-2? Conversely, if the compiler is given a constant for the second argument, and is not optimizing modulo power of two into bitwise ops (at least at CFLAGS=-O2), then that's a compiler bug.
diff --git a/disas.c b/disas.c index d335c55bbf..8b59448286 100644 --- a/disas.c +++ b/disas.c @@ -151,7 +151,7 @@ static int print_insn_objdump(bfd_vma pc, disassemble_info *info, info->read_memory_func(pc, buf, n, info); for (i = 0; i < n; ++i) { - if (i % 32 == 0) { + if (QEMU_IS_ALIGNED(i, 32)) { info->fprintf_func(info->stream, "\n%s: ", prefix); } info->fprintf_func(info->stream, "%02x", buf[i]);
Applied using the Coccinelle semantic patch scripts/coccinelle/use_osdep.cocci Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- There is no entry for this file in MAINTAINERS, should it go under TCG/Overall? disas.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)