[2/5] support: don't pass to resolv_test_start a big struct by value
Commit Message
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(-)
Comments
* 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.)
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.
* 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.
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.
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.
* 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.
В Пн, мар 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)
@@ -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;
}
@@ -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). */