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(-)

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

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
>
>



-- 
Marc-André Lureau

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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