linux-kernel - Re: [PATCH 15/23] ivtv: Replace memcpy with struct assignment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <1351030129.2459.17.camel@palomino.walls.org>
Date:	Tue, 23 Oct 2012 18:08:48 -0400
From:	Andy Walls <awalls@...metrocast.net>
To:	Ezequiel Garcia <elezegarcia@...il.com>
Cc:	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	Julia.Lawall@...6.fr, kernel-janitors@...r.kernel.org,
	Peter Senna Tschudin <peter.senna@...il.com>
Subject: Re: [PATCH 15/23] ivtv: Replace memcpy with struct assignment

On Tue, 2012-10-23 at 16:57 -0300, Ezequiel Garcia wrote:
> This kind of memcpy() is error-prone. Its replacement with a struct
> assignment is prefered because it's type-safe and much easier to read.

This one is a code maintenance win. :)

See my comments at the end for the difference in assembled code on an
AMD x86_64 CPU using
$ gcc --version
gcc (GCC) 4.6.3 20120306 (Red Hat 4.6.3-2)


> Found by coccinelle. Hand patched and reviewed.
> Tested by compilation only.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> @@
> identifier struct_name;
> struct struct_name to;
> struct struct_name from;
> expression E;
> @@
> -memcpy(&(to), &(from), E);
> +to = from;
> // </smpl>
> 
> Cc: Andy Walls <awalls@...metrocast.net>

Signed-off-by: Andy Walls <awalls@...metrocast.net>


> Signed-off-by: Peter Senna Tschudin <peter.senna@...il.com>
> Signed-off-by: Ezequiel Garcia <elezegarcia@...il.com>
> ---
>  drivers/media/pci/ivtv/ivtv-i2c.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/pci/ivtv/ivtv-i2c.c b/drivers/media/pci/ivtv/ivtv-i2c.c
> index d47f41a..27a8466 100644
> --- a/drivers/media/pci/ivtv/ivtv-i2c.c
> +++ b/drivers/media/pci/ivtv/ivtv-i2c.c
> @@ -719,13 +719,10 @@ int init_ivtv_i2c(struct ivtv *itv)
>  		return -ENODEV;
>  	}
>  	if (itv->options.newi2c > 0) {
> -		memcpy(&itv->i2c_adap, &ivtv_i2c_adap_hw_template,
> -		       sizeof(struct i2c_adapter));
> +		itv->i2c_adap = ivtv_i2c_adap_hw_template;
>  	} else {
> -		memcpy(&itv->i2c_adap, &ivtv_i2c_adap_template,
> -		       sizeof(struct i2c_adapter));
> -		memcpy(&itv->i2c_algo, &ivtv_i2c_algo_template,
> -		       sizeof(struct i2c_algo_bit_data));
> +		itv->i2c_adap = ivtv_i2c_adap_template;
> +		itv->i2c_algo = ivtv_i2c_algo_template;
>  	}
>  	itv->i2c_algo.udelay = itv->options.i2c_clock_period / 2;
>  	itv->i2c_algo.data = itv;
> @@ -735,8 +732,7 @@ int init_ivtv_i2c(struct ivtv *itv)
>  		itv->instance);
>  	i2c_set_adapdata(&itv->i2c_adap, &itv->v4l2_dev);
>  
> -	memcpy(&itv->i2c_client, &ivtv_i2c_client_template,
> -	       sizeof(struct i2c_client));
> +	itv->i2c_client = ivtv_i2c_client_template;
>  	itv->i2c_client.adapter = &itv->i2c_adap;
>  	itv->i2c_adap.dev.parent = &itv->pdev->dev;
>  

I looked at the generated assembly with only this last change
implemented:

$ objdump -h -r -d -l -s orig-ivtv-i2c.o.sav | less
[...]
 07e0 00000000 69767476 20696e74 65726e61  ....ivtv interna
 07f0 6c000000 00000000 00000000 00000000  l...............
 0800 00000000 00000000 00000000 00000000  ................
 0810 00000000 00000000 00000000 00000000  ................
 0820 00000000 00000000 00000000 00000000  ................
 0830 00000000 00000000 00000000 00000000  ................
[...]
init_ivtv_i2c():
/home/andy/cx18dev/git/media_tree/drivers/media/video/ivtv/ivtv-i2c.c:738
    13bb:       48 c7 c6 00 00 00 00    mov    $0x0,%rsi
                        13be: R_X86_64_32S      .rodata+0x7e0
    13c2:       48 8d bb 30 04 01 00    lea    0x10430(%rbx),%rdi
    13c9:       b9 5a 00 00 00          mov    $0x5a,%ecx
    13ce:       f3 48 a5                rep movsq %ds:(%rsi),%es:(%rdi)


$ objdump -h -r -d -l -s orig-ivtv-i2c.o.sav | less
[...]
 07e0 00000000 69767476 20696e74 65726e61  ....ivtv interna
 07f0 6c000000 00000000 00000000 00000000  l...............
 0800 00000000 00000000 00000000 00000000  ................
 0810 00000000 00000000 00000000 00000000  ................
 0820 00000000 00000000 00000000 00000000  ................
 0830 00000000 00000000 00000000 00000000  ................
[...]
init_ivtv_i2c():
/home/andy/cx18dev/git/media_tree/drivers/media/video/ivtv/ivtv-i2c.c:738
    13bb:       48 8d bb 30 04 01 00    lea    0x10430(%rbx),%rdi
    13c2:       48 c7 c6 00 00 00 00    mov    $0x0,%rsi
                        13c5: R_X86_64_32S      .rodata+0x7e0
    13c9:       b9 5a 00 00 00          mov    $0x5a,%ecx
    13ce:       f3 48 a5                rep movsq %ds:(%rsi),%es:(%rdi)


The generated code is reordered, but essentially identical.  So I guess
in this instance, the preprocessor defines resolved such that an x86-64
optimized memcpy() function was not used from the linux kernel source.

Since all of these memcpy()'s are only called once for each board at
board initialization, performance here really doesn't matter here
anyway.  (Unless one is insanely trying to shave microseconds off boot
time :P )

With other memcpy()/assignement_operator replacement patches, you may
wish to keep performance in mind, if you are patching a frequently
called function.

Regards,
Andy


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/