Patchwork [2/5] support: don't pass to resolv_test_start a big struct by value

login
register
mail settings
Submitter Konstantin Kharlamov
Date March 24, 2019, 12:21 p.m.
Message ID <20190324122121.6430-3-Hi-Angel@yandex.ru>
Download mbox | patch
Permalink /patch/31963/
State New
Headers show

Comments

Konstantin Kharlamov - March 24, 2019, 12:21 p.m.
Fixes LGTM warning: "This parameter of type resolv_redirect_config is
88 bytes - consider passing a const pointer/reference instead."

Signed-off-by: Konstantin Kharlamov <Hi-Angel@yandex.ru>
---
 support/resolv_test.c | 33 ++++++++++++++++-----------------
 support/resolv_test.h |  2 +-
 2 files changed, 17 insertions(+), 18 deletions(-)
Florian Weimer - March 25, 2019, 8:33 a.m.
* Konstantin Kharlamov:

> diff --git a/support/resolv_test.h b/support/resolv_test.h
> index c9e48205ab..880330ad5c 100644
> --- a/support/resolv_test.h
> +++ b/support/resolv_test.h
> @@ -116,7 +116,7 @@ void resolv_test_init (void);
>     needed.  As a side effect, NSS is reconfigured to use nss_dns only
>     for aplicable databases, and the process may enter a network
>     namespace for better isolation.  */
> -struct resolv_test *resolv_test_start (struct resolv_redirect_config);
> +struct resolv_test *resolv_test_start (const struct resolv_redirect_config*);

This patch would adjusting all the tests that call resolv_test_start,
and these changes are missing from the patch.  It is unclear how this
change would be an improvement because most tests call
resolv_test_start exactly once and the parameter object is never used
again.  Passing a pointer requires writing the argument object to the
stack *and* supplying its address to resolv_test_start, which requires
more work.  (Maybe some targets have more optimized code for struct
initialization than passing many zero arguments, but that's a GCC
issue which will eventually be fixed.)
Konstantin Kharlamov - March 25, 2019, 10:33 a.m.
On Пн, Mar 25, 2019 at 11:33:28, Florian Weimer <fw@deneb.enyo.de> 
wrote:
> * Konstantin Kharlamov:
> 
>>  diff --git a/support/resolv_test.h b/support/resolv_test.h
>>  index c9e48205ab..880330ad5c 100644
>>  --- a/support/resolv_test.h
>>  +++ b/support/resolv_test.h
>>  @@ -116,7 +116,7 @@ void resolv_test_init (void);
>>      needed.  As a side effect, NSS is reconfigured to use nss_dns 
>> only
>>      for aplicable databases, and the process may enter a network
>>      namespace for better isolation.  */
>>  -struct resolv_test *resolv_test_start (struct 
>> resolv_redirect_config);
>>  +struct resolv_test *resolv_test_start (const struct 
>> resolv_redirect_config*);
> 
> This patch would adjusting all the tests that call resolv_test_start,
> and these changes are missing from the patch.  It is unclear how this
> change would be an improvement because most tests call
> resolv_test_start exactly once and the parameter object is never used
> again.  Passing a pointer requires writing the argument object to the
> stack *and* supplying its address to resolv_test_start, which requires
> more work.  (Maybe some targets have more optimized code for struct
> initialization than passing many zero arguments, but that's a GCC
> issue which will eventually be fixed.)

Wow, are you saying that in the assembly the address that gets passed 
to resolve_test_start() would be not the address of struct, but instead 
an address of an addres on a stack that is an address of the struct…? 
That doesn't sound right, is that some calling convention? I've never 
heard of this.
Florian Weimer - March 25, 2019, 10:35 a.m.
* Konstantin Kharlamov:

> On Пн, Mar 25, 2019 at 11:33:28, Florian Weimer <fw@deneb.enyo.de> 
> wrote:
>> * Konstantin Kharlamov:
>> 
>>>  diff --git a/support/resolv_test.h b/support/resolv_test.h
>>>  index c9e48205ab..880330ad5c 100644
>>>  --- a/support/resolv_test.h
>>>  +++ b/support/resolv_test.h
>>>  @@ -116,7 +116,7 @@ void resolv_test_init (void);
>>>      needed.  As a side effect, NSS is reconfigured to use nss_dns 
>>> only
>>>      for aplicable databases, and the process may enter a network
>>>      namespace for better isolation.  */
>>>  -struct resolv_test *resolv_test_start (struct 
>>> resolv_redirect_config);
>>>  +struct resolv_test *resolv_test_start (const struct 
>>> resolv_redirect_config*);
>> 
>> This patch would adjusting all the tests that call resolv_test_start,
>> and these changes are missing from the patch.  It is unclear how this
>> change would be an improvement because most tests call
>> resolv_test_start exactly once and the parameter object is never used
>> again.  Passing a pointer requires writing the argument object to the
>> stack *and* supplying its address to resolv_test_start, which requires
>> more work.  (Maybe some targets have more optimized code for struct
>> initialization than passing many zero arguments, but that's a GCC
>> issue which will eventually be fixed.)
>
> Wow, are you saying that in the assembly the address that gets passed 
> to resolve_test_start() would be not the address of struct, but instead 
> an address of an addres on a stack that is an address of the struct…?

With the pointer-to-struct argument, you need to write the temporary
object to the stack and pass its address.

With the struct argument, you need to write the temporary object to
the stack, in the form of an argument list.

The second case avoids computing and passing the address of the
temporary object.
Andreas Schwab - March 25, 2019, 10:54 a.m.
On Mär 25 2019, Florian Weimer <fw@deneb.enyo.de> wrote:

> With the pointer-to-struct argument, you need to write the temporary
> object to the stack and pass its address.
>
> With the struct argument, you need to write the temporary object to
> the stack, in the form of an argument list.
>
> The second case avoids computing and passing the address of the
> temporary object.

On the other hand, the second case requires more complex parameter
setup, when the argument is passed partially in registers.

Andreas.
Konstantin Kharlamov - March 25, 2019, 10:58 a.m.
On Пн, Mar 25, 2019 at 13:35:56, Florian Weimer <fw@deneb.enyo.de> 
wrote:
> * Konstantin Kharlamov:
> 
>>  On Пн, Mar 25, 2019 at 11:33:28, Florian Weimer <fw@deneb.enyo.de>
>>  wrote:
>>>  * Konstantin Kharlamov:
>>> 
>>>>   diff --git a/support/resolv_test.h b/support/resolv_test.h
>>>>   index c9e48205ab..880330ad5c 100644
>>>>   --- a/support/resolv_test.h
>>>>   +++ b/support/resolv_test.h
>>>>   @@ -116,7 +116,7 @@ void resolv_test_init (void);
>>>>       needed.  As a side effect, NSS is reconfigured to use nss_dns
>>>>  only
>>>>       for aplicable databases, and the process may enter a network
>>>>       namespace for better isolation.  */
>>>>   -struct resolv_test *resolv_test_start (struct
>>>>  resolv_redirect_config);
>>>>   +struct resolv_test *resolv_test_start (const struct
>>>>  resolv_redirect_config*);
>>> 
>>>  This patch would adjusting all the tests that call 
>>> resolv_test_start,
>>>  and these changes are missing from the patch.  It is unclear how 
>>> this
>>>  change would be an improvement because most tests call
>>>  resolv_test_start exactly once and the parameter object is never 
>>> used
>>>  again.  Passing a pointer requires writing the argument object to 
>>> the
>>>  stack *and* supplying its address to resolv_test_start, which 
>>> requires
>>>  more work.  (Maybe some targets have more optimized code for struct
>>>  initialization than passing many zero arguments, but that's a GCC
>>>  issue which will eventually be fixed.)
>> 
>>  Wow, are you saying that in the assembly the address that gets 
>> passed
>>  to resolve_test_start() would be not the address of struct, but 
>> instead
>>  an address of an addres on a stack that is an address of the 
>> struct…?
> 
> With the pointer-to-struct argument, you need to write the temporary
> object to the stack and pass its address.
> 
> With the struct argument, you need to write the temporary object to
> the stack, in the form of an argument list.
> 
> The second case avoids computing and passing the address of the
> temporary object.

Can't reproduce here. Testcase:

    $ cat test.c
    #include <stdlib.h>

    struct Foo {
        int a;
        char b;
    };

    void get_foo(struct Foo*);

    int main() {
        struct Foo foo = {};
        get_foo(&foo);
    }
    $ gcc test.c -S -fverbose-asm -o a.S -g3 -O0

Produces the following code for main() in a.S file (snip of a relevant 
part):

        .globl	main
        .type	main, @function
    main:
    .LFB6:
        .file 1 "test.c"
        .loc 1 10 12
        .cfi_startproc
        pushq	%rbp	#
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq	%rsp, %rbp	#,
        .cfi_def_cfa_register 6
        subq	$16, %rsp	#,
    # test.c:10: int main() {
        .loc 1 10 12
        movq	%fs:40, %rax	# MEM[(<address-space-1> long unsigned int 
*)40B], tmp91
        movq	%rax, -8(%rbp)	# tmp91, D.2539
        xorl	%eax, %eax	# tmp91
    # test.c:11:     struct Foo foo = {};
        .loc 1 11 16
        movq	$0, -16(%rbp)	#, foo
    # test.c:12:     get_foo(&foo);
        .loc 1 12 5
        leaq	-16(%rbp), %rax	#, tmp89
        movq	%rax, %rdi	# tmp89,
        call	get_foo@PLT	#
        movl	$0, %eax	#, _5
    # test.c:13: }

The 3 actions being done just before get_foo(), commented:

        leaq	-16(%rbp), %rax	# put address of the struct to rax register
        movq	%rax, %rdi      # move content of rax into rdi (for 
whatever reason)
        call	get_foo@PLT     # call the function

Note, it doesn't put the address into stack, it passes it in registers, 
just as I'd expect.
Florian Weimer - March 25, 2019, 11:16 a.m.
* Konstantin Kharlamov:

> On Пн, Mar 25, 2019 at 13:35:56, Florian Weimer <fw@deneb.enyo.de> 
> wrote:
>> * Konstantin Kharlamov:
>> 
>>>  On Пн, Mar 25, 2019 at 11:33:28, Florian Weimer <fw@deneb.enyo.de>
>>>  wrote:
>>>>  * Konstantin Kharlamov:
>>>> 
>>>>>   diff --git a/support/resolv_test.h b/support/resolv_test.h
>>>>>   index c9e48205ab..880330ad5c 100644
>>>>>   --- a/support/resolv_test.h
>>>>>   +++ b/support/resolv_test.h
>>>>>   @@ -116,7 +116,7 @@ void resolv_test_init (void);
>>>>>       needed.  As a side effect, NSS is reconfigured to use nss_dns
>>>>>  only
>>>>>       for aplicable databases, and the process may enter a network
>>>>>       namespace for better isolation.  */
>>>>>   -struct resolv_test *resolv_test_start (struct
>>>>>  resolv_redirect_config);
>>>>>   +struct resolv_test *resolv_test_start (const struct
>>>>>  resolv_redirect_config*);
>>>> 
>>>>  This patch would adjusting all the tests that call 
>>>> resolv_test_start,
>>>>  and these changes are missing from the patch.  It is unclear how 
>>>> this
>>>>  change would be an improvement because most tests call
>>>>  resolv_test_start exactly once and the parameter object is never 
>>>> used
>>>>  again.  Passing a pointer requires writing the argument object to 
>>>> the
>>>>  stack *and* supplying its address to resolv_test_start, which 
>>>> requires
>>>>  more work.  (Maybe some targets have more optimized code for struct
>>>>  initialization than passing many zero arguments, but that's a GCC
>>>>  issue which will eventually be fixed.)
>>> 
>>>  Wow, are you saying that in the assembly the address that gets 
>>> passed
>>>  to resolve_test_start() would be not the address of struct, but 
>>> instead
>>>  an address of an addres on a stack that is an address of the 
>>> struct…?
>> 
>> With the pointer-to-struct argument, you need to write the temporary
>> object to the stack and pass its address.
>> 
>> With the struct argument, you need to write the temporary object to
>> the stack, in the form of an argument list.
>> 
>> The second case avoids computing and passing the address of the
>> temporary object.
>
> Can't reproduce here. Testcase:

You need to look at this:

struct data {
  long a0, a1, a2, a3, a4, a5, a6, a7, a8, a9;
};

void object_parameter (struct data);
void pointer_parameter (const struct data *);

void
call_object_parameter (void)
{
  object_parameter ((struct data) { 0 });
}

void
call_pointer_parameter (void)
{
  struct data obj = { 0 };
  pointer_parameter (&obj);
}

This produces:

call_object_parameter:
	subq	$88, %rsp
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	pushq	$0
	call	object_parameter@PLT
	addq	$168, %rsp
	ret

call_pointer_parameter:
	subq	$88, %rsp
	pxor	%xmm0, %xmm0
	movq	%rsp, %rdi
	movaps	%xmm0, (%rsp)
	movaps	%xmm0, 16(%rsp)
	movaps	%xmm0, 32(%rsp)
	movaps	%xmm0, 48(%rsp)
	movaps	%xmm0, 64(%rsp)
	call	pointer_parameter@PLT
	addq	$88, %rsp
	ret

The additional instruction is:

	movq	%rsp, %rdi

This is *required* by the ABI.  The rest of the differences are due to
inefficiencies in the i386 backend and could be fixed there.  But the
movq *cannot* be optimized out for the pointer-passing case.
Konstantin Kharlamov - March 25, 2019, 9:45 p.m.
В Пн, мар 25, 2019 at 12:16, Florian Weimer <fw@deneb.enyo.de> 
написал:
> * Konstantin Kharlamov:
> 
>>  On Пн, Mar 25, 2019 at 13:35:56, Florian Weimer <fw@deneb.enyo.de>
>>  wrote:
>>>  * Konstantin Kharlamov:
>>> 
>>>>   On Пн, Mar 25, 2019 at 11:33:28, Florian Weimer 
>>>> <fw@deneb.enyo.de>
>>>>   wrote:
>>>>>   * Konstantin Kharlamov:
>>>>> 
>>>>>>    diff --git a/support/resolv_test.h b/support/resolv_test.h
>>>>>>    index c9e48205ab..880330ad5c 100644
>>>>>>    --- a/support/resolv_test.h
>>>>>>    +++ b/support/resolv_test.h
>>>>>>    @@ -116,7 +116,7 @@ void resolv_test_init (void);
>>>>>>        needed.  As a side effect, NSS is reconfigured to use 
>>>>>> nss_dns
>>>>>>   only
>>>>>>        for aplicable databases, and the process may enter a 
>>>>>> network
>>>>>>        namespace for better isolation.  */
>>>>>>    -struct resolv_test *resolv_test_start (struct
>>>>>>   resolv_redirect_config);
>>>>>>    +struct resolv_test *resolv_test_start (const struct
>>>>>>   resolv_redirect_config*);
>>>>> 
>>>>>   This patch would adjusting all the tests that call
>>>>>  resolv_test_start,
>>>>>   and these changes are missing from the patch.  It is unclear how
>>>>>  this
>>>>>   change would be an improvement because most tests call
>>>>>   resolv_test_start exactly once and the parameter object is never
>>>>>  used
>>>>>   again.  Passing a pointer requires writing the argument object 
>>>>> to
>>>>>  the
>>>>>   stack *and* supplying its address to resolv_test_start, which
>>>>>  requires
>>>>>   more work.  (Maybe some targets have more optimized code for 
>>>>> struct
>>>>>   initialization than passing many zero arguments, but that's a 
>>>>> GCC
>>>>>   issue which will eventually be fixed.)
>>>> 
>>>>   Wow, are you saying that in the assembly the address that gets
>>>>  passed
>>>>   to resolve_test_start() would be not the address of struct, but
>>>>  instead
>>>>   an address of an addres on a stack that is an address of the
>>>>  struct…?
>>> 
>>>  With the pointer-to-struct argument, you need to write the 
>>> temporary
>>>  object to the stack and pass its address.
>>> 
>>>  With the struct argument, you need to write the temporary object to
>>>  the stack, in the form of an argument list.
>>> 
>>>  The second case avoids computing and passing the address of the
>>>  temporary object.
>> 
>>  Can't reproduce here. Testcase:
> 
> You need to look at this:
> 
> struct data {
>   long a0, a1, a2, a3, a4, a5, a6, a7, a8, a9;
> };
> 
> void object_parameter (struct data);
> void pointer_parameter (const struct data *);
> 
> void
> call_object_parameter (void)
> {
>   object_parameter ((struct data) { 0 });
> }
> 
> void
> call_pointer_parameter (void)
> {
>   struct data obj = { 0 };
>   pointer_parameter (&obj);
> }
> 
> This produces:
> 
> call_object_parameter:
> 	subq	$88, %rsp
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	pushq	$0
> 	call	object_parameter@PLT
> 	addq	$168, %rsp
> 	ret
> 
> call_pointer_parameter:
> 	subq	$88, %rsp
> 	pxor	%xmm0, %xmm0
> 	movq	%rsp, %rdi
> 	movaps	%xmm0, (%rsp)
> 	movaps	%xmm0, 16(%rsp)
> 	movaps	%xmm0, 32(%rsp)
> 	movaps	%xmm0, 48(%rsp)
> 	movaps	%xmm0, 64(%rsp)
> 	call	pointer_parameter@PLT
> 	addq	$88, %rsp
> 	ret
> 
> The additional instruction is:
> 
> 	movq	%rsp, %rdi
> 
> This is *required* by the ABI.  The rest of the differences are due to
> inefficiencies in the i386 backend and could be fixed there.  But the
> movq *cannot* be optimized out for the pointer-passing case.

Okay, so, %rsp is a stack pointer. What happens on that line is that we 
copy stack pointer to %rdi. This is done because %rdi stores the first 
argument; and the %rsp, incidentally, points to the beginning of the 
struct.

My point stands because this code does not store a pointer to the 
struct on a stack. Again, both %rsp and %rdi, that happened to have the 
pointer, are registers.

Now, as I understand your 2-nd point is that, barring backend 
inefficiency, the call_object_parameter() should contain less code 
because there's no need for "movq %rsp, %rdi". Well… It could be true 
for the testcase here, because we create an object and pass it 
immediately. But in real world the object usually exists! Then, in case 
with pointer you only need to pass a pointer; whereas otherwise you 
would have to copy the whole thing to a stack.

(sorry for delay, I'm mostly free to reply on evenings; or, rather, 
it's night here :D)

Patch

diff --git a/support/resolv_test.c b/support/resolv_test.c
index f400026cd1..ab4af9b684 100644
--- a/support/resolv_test.c
+++ b/support/resolv_test.c
@@ -1103,44 +1103,43 @@  set_search_path (const struct resolv_redirect_config* config)
 }
 
 struct resolv_test *
-resolv_test_start (struct resolv_redirect_config config)
+resolv_test_start (const struct resolv_redirect_config *config)
 {
   /* Apply configuration defaults.  */
-  if (config.nscount == 0)
-    config.nscount = resolv_max_test_servers;
+  const int nscount = (config->nscount == 0)? resolv_max_test_servers : 0;
 
   struct resolv_test *obj = xmalloc (sizeof (*obj));
   *obj = (struct resolv_test) {
-    .config = config,
+    .config = *config,
     .lock = PTHREAD_MUTEX_INITIALIZER,
   };
 
-  if (!config.disable_redirect)
+  if (!config->disable_redirect)
     resolv_test_init ();
 
   /* Create all the servers, to reserve the necessary ports.  */
-  for (int server_index = 0; server_index < config.nscount; ++server_index)
-    if (config.disable_redirect && config.server_address_overrides != NULL)
+  for (int server_index = 0; server_index < nscount; ++server_index)
+    if (config->disable_redirect && config->server_address_overrides != NULL)
       make_server_sockets_for_address
         (obj->servers + server_index,
-         config.server_address_overrides[server_index]);
+         config->server_address_overrides[server_index]);
     else
       make_server_sockets (obj->servers + server_index);
 
   /* Start server threads.  Disable the server ports, as
      requested.  */
-  for (int server_index = 0; server_index < config.nscount; ++server_index)
+  for (int server_index = 0; server_index < nscount; ++server_index)
     {
       struct resolv_test_server *server = obj->servers + server_index;
-      if (config.servers[server_index].disable_udp)
+      if (config->servers[server_index].disable_udp)
         {
           xclose (server->socket_udp);
           server->socket_udp = -1;
         }
-      else if (!config.single_thread_udp)
+      else if (!config->single_thread_udp)
         server->thread_udp = start_server_thread (obj, server_index,
                                                   server_thread_udp);
-      if (config.servers[server_index].disable_tcp)
+      if (config->servers[server_index].disable_tcp)
         {
           xclose (server->socket_tcp);
           server->socket_tcp = -1;
@@ -1149,10 +1148,10 @@  resolv_test_start (struct resolv_redirect_config config)
         server->thread_tcp = start_server_thread (obj, server_index,
                                                   server_thread_tcp);
     }
-  if (config.single_thread_udp)
+  if (config->single_thread_udp)
     start_server_thread_udp_single (obj);
 
-  if (config.disable_redirect)
+  if (config->disable_redirect)
     return obj;
 
   int timeout = 1;
@@ -1176,7 +1175,7 @@  resolv_test_start (struct resolv_redirect_config config)
     }
   _res.retrans = timeout;
   _res.retry = 4;
-  _res.nscount = config.nscount;
+  _res.nscount = nscount;
   _res.options = RES_INIT | RES_RECURSE | RES_DEFNAMES | RES_DNSRCH;
   _res.ndots = 1;
   if (test_verbose)
@@ -1187,7 +1186,7 @@  resolv_test_start (struct resolv_redirect_config config)
       printf ("info: new _res.nscount value: %d\n", _res.nscount);
       printf ("info: new _res.ndots value: %d\n", _res.ndots);
     }
-  for (int server_index = 0; server_index < config.nscount; ++server_index)
+  for (int server_index = 0; server_index < nscount; ++server_index)
     {
       TEST_VERIFY_EXIT (obj->servers[server_index].address.sin_port != 0);
       _res.nsaddr_list[server_index] = obj->servers[server_index].address;
@@ -1203,7 +1202,7 @@  resolv_test_start (struct resolv_redirect_config config)
         }
     }
 
-  set_search_path (&config);
+  set_search_path (config);
 
   return obj;
 }
diff --git a/support/resolv_test.h b/support/resolv_test.h
index c9e48205ab..880330ad5c 100644
--- a/support/resolv_test.h
+++ b/support/resolv_test.h
@@ -116,7 +116,7 @@  void resolv_test_init (void);
    needed.  As a side effect, NSS is reconfigured to use nss_dns only
    for aplicable databases, and the process may enter a network
    namespace for better isolation.  */
-struct resolv_test *resolv_test_start (struct resolv_redirect_config);
+struct resolv_test *resolv_test_start (const struct resolv_redirect_config*);
 
 /* Call this function at the end of resolver testing, to free
    resources and report pending errors (if any).  */