geom_dev: Use kenv 'dumpdev' in the same way as rc/etc.d/dumpon

[cem]

Summary

Skip over any /dev/ prefix so that values appropriate for rc/etc.d/dumpon will
match GEOM device names. If a 'dumpdev' kenv does not start with '/dev/', the
behavior is unchanged.

Per discussion on freebsd-current@.

Test Plan

buildkernel succeeds.

[markj]

Looks ok to me.

sys/geom/geom_dev.c

119 ↗

(On Diff #8911)

Why make this static? Doesn't that just force a symbol to be emitted for no real reason?

[avg]

This is a simple enough, bikeshed-grade change, so here it goes.
Why have a new file-scope variable at all? Can't we just put the strncmp("/dev/") code into init_dumpdev?

[cem]

sys/geom/geom_dev.c

119 ↗

(On Diff #8911)

'Static' because conceptually it only needs to be initialized once. I think Clang is smart enough to do the right thing with or without 'static' at -O2. I don't believe it forces a symbol to be emitted:

$ nm geom_dev.o |grep devprefix
$ nm geom_dev.o |grep dumpdev
0000000000000008 b dumpdev
0000000000000000 b env_dumpdev
0000000000000dc0 t g_dev_setdumpdev

Wikipedia has this simplified blurb about it:

Static local variables: variables declared as static inside a function are statically allocated, thus keep their memory cell throughout all program execution, while having the same scope of visibility as automatic local variables, meaning remain local to the function.

Anyway, I'm okay dropping static if you want me to, because I expect Clang will generate the same code either way.

Here's the code now (with 'static'):

(gdb) disas /mr g_dev_init
118     {
   0xffffffff809da0d0 <+0>:     55      push   %rbp
   0xffffffff809da0d1 <+1>:     48 89 e5        mov    %rsp,%rbp

119             static const char *devprefix = "/dev/";
120
121             dumpdev = env_dumpdev = kern_getenv("dumpdev");
   0xffffffff809da0d4 <+4>:     53      push   %rbx
   0xffffffff809da0d5 <+5>:     50      push   %rax
   0xffffffff809da0d6 <+6>:     48 c7 c7 5e d3 41 81    mov    $0xffffffff8141d35e,%rdi
   0xffffffff809da0dd <+13>:    e8 be a4 06 00  callq  0xffffffff80a445a0 <kern_getenv>
   0xffffffff809da0e2 <+18>:    48 89 c3        mov    %rax,%rbx
   0xffffffff809da0e5 <+21>:    48 89 1c 25 a0 bb b4 81 mov    %rbx,0xffffffff81b4bba0
   0xffffffff809da0ed <+29>:    48 89 1c 25 a8 bb b4 81 mov    %rbx,0xffffffff81b4bba8

(gdb) p &dumpdev
$1 = (char **) 0xffffffff81b4bba8 <dumpdev>
(gdb) p &env_dumpdev
$2 = (char **) 0xffffffff81b4bba0 <env_dumpdev>

122             if (strncmp(dumpdev, devprefix, strlen(devprefix)) == 0)
   0xffffffff809da0f5 <+37>:    48 c7 c7 8d c0 3e 81    mov    $0xffffffff813ec08d,%rdi

(gdb) p (char *)0xffffffff813ec08d
$4 = 0xffffffff813ec08d "/dev/"

   0xffffffff809da0fc <+44>:    e8 ff 4b 19 00  callq  0xffffffff80b6ed00 <strlen>
   0xffffffff809da101 <+49>:    48 c7 c6 8d c0 3e 81    mov    $0xffffffff813ec08d,%rsi
   0xffffffff809da108 <+56>:    48 89 df        mov    %rbx,%rdi
   0xffffffff809da10b <+59>:    48 89 c2        mov    %rax,%rdx
   0xffffffff809da10e <+62>:    e8 dd 4c 19 00  callq  0xffffffff80b6edf0 <strncmp>
   0xffffffff809da113 <+67>:    85 c0   test   %eax,%eax
   0xffffffff809da115 <+69>:    75 14   jne    0xffffffff809da12b <g_dev_init+91>

123                     dumpdev += strlen(devprefix);
   0xffffffff809da117 <+71>:    48 c7 c7 8d c0 3e 81    mov    $0xffffffff813ec08d,%rdi
   0xffffffff809da11e <+78>:    e8 dd 4b 19 00  callq  0xffffffff80b6ed00 <strlen>
   0xffffffff809da123 <+83>:    48 01 04 25 a8 bb b4 81 add    %rax,0xffffffff81b4bba8

I'm actually kind of surprised it was too dumb to dedupe the strlen().

124     }
   0xffffffff809da12b <+91>:    48 83 c4 08     add    $0x8,%rsp
   0xffffffff809da12f <+95>:    5b      pop    %rbx
   0xffffffff809da130 <+96>:    5d      pop    %rbp
   0xffffffff809da131 <+97>:    c3      retq

[cem]

In D3725#76830, @avg wrote:

This is a simple enough, bikeshed-grade change, so here it goes.
Why have a new file-scope variable at all? Can't we just put the strncmp("/dev/") code into init_dumpdev?

Sigh. Sure.

[cem]

Per avg@, drop the extra variable and just do it in init_dumpdev().

[markj]

sys/geom/geom_dev.c

118 ↗

(On Diff #8912)

Fair enough! For some reason I was convinced that static variables always have associated symbols.

[avg]

head/sys/geom/geom_dev.c

156

Just a nit. I think that it would have been better to define devprefix as const char[]. Then sizeof() could be used instead of strlen(). strlen on a string that is known at the compile time is just "not nice" for my taste.

[cem]

head/sys/geom/geom_dev.c

156

strlen makes intent obvious — find the length of this string. sizeof() - 1 is less nice, IMO.

Anyway, there shouldn't be a performance penalty. The compiler should be able to compute strlen of a string known at compile time, at compile time :-). That clang does not in the kernel is kind of sad. E.g.:

#include <stdio.h>
#include <string.h>

void
main(void)
{
        const char *c = "hello";
        size_t n;

        n = strlen(c);
        printf("%zu\n", n); 
}

$ cc -O2 b.c
$ gdb791 a.out
...
(gdb) disas main
   0x0000000000400930 <+0>:     push   %rbp
   0x0000000000400931 <+1>:     mov    %rsp,%rbp
   0x0000000000400934 <+4>:     mov    $0x4009ae,%edi
   0x0000000000400939 <+9>:     mov    $0x5,%esi      <<< strlen("hello")
   0x000000000040093e <+14>:    xor    %eax,%eax
   0x0000000000400940 <+16>:    pop    %rbp
   0x0000000000400941 <+17>:    jmpq   0x40049c <printf@plt>

I'm not sure why clang isn't making that optimization for the kernel build.

[markj]

Is clang allowed to assume anything about the behaviour of strlen in a freestanding environment?

[cem]

In D3725#76978, @markj wrote:

Is clang allowed to assume anything about the behaviour of strlen in a freestanding environment?

Maybe not, at least without some sort of hint. I think it should be able to assume strlen matches standard C strlen expectations with some sort of hint.

(Alternatively, I don't know that there is a lot of value in allowing non-conforming strlen functions even in a freestanding environment. I'd be okay with Clang assuming standard strlen behavior in freestanding mode and erroring hard if an invalid implementation is provided.)