[Libguestfs] [PATCH nbdkit] server: Add -D nbdkit.environ=1 to dump the environment

[Laszlo Ersek]

On 5/8/23 23:52, Eric Blake wrote:
> On Mon, May 08, 2023 at 10:33:25PM +0100, Richard W.M. Jones wrote:
>>>>  
>>>
>>> I was surprised not to see any assignment to the new global variable,
>>> but then I found "server/debug-flags.c"; that's some serious wizardry.
>>
>> With the side effect, as it turns out, that it's very hard to
>> implement for language plugins.  Rust is fine.  OCaml can be done by
>> adding a C object to the link.  Other languages, not really :-(
> 
> And not today's problem to solve.
> 
>>
>>> Side thought: should we not make both "nbdkit_debug_environ" (and
>>> friends), and the target of the "sym" pointer in "apply_debug_flags",
>>> volatile? Aliasing doesn't get much more obscure than this, and I wonder
>>> if this has the potential to blow up later, when compilers (and
>>> processors) get even more clever.
>>>
>>> Of course POSIX could always say "this is just required to work", which
>>> I guess would be fine, but currently it doesn't seem to say it.

[*]

>>
>> I guess Eric will know for sure, but isn't it generally true that all
>> global variables can be modified by any function call?  My model is
>> that they can modified at any time by any called function; even
>> through a pointer so not really visible to the compiler.
> 
> Marking a variable as volatile will force the compiler to reload it on
> each access, which can slow down loops that read the debug variable.
> But in general, these are not variables we are frequently changing (we
> do it at most once, during startup, based on the environment); and a
> compiler can't completely eliminate global variables unless it is
> doing some heavy LTO.  I see no reason why we would want to mark it
> volatile, because the only thing that would guarantee is that the
> compiler no longer hoists repeated reads of the globals outside of
> loops, even though our loops won't be changing the debug state.

Using "nbdkit_debug_environ" as an example:

The entire source code contains no assignment to "nbdkit_debug_environ",
not even through a pointer derived from "nbdkit_debug_environ". A super
smart compiler might validly assume that "nbdkit_debug_environ" is never
written to, therefore replace all read accesses to
"nbdkit_debug_environ" with constants (and open-code or remove the
dependent, conditional code).

So there is a reason why we *might* want to mark the variable as
volatile: compilers are getting too smart. I'm not saying it is
*practical*; in fact I'm sure dlopen() and dlsym() *guarantee* that
Rich's code will work as intended, as written. I guess I'm just asking
for POSIX to spell that out.

... and, in retrospect, it does! So my statement at [*] was wrong! The
dlsym() spec includes:

    The return value from dlsym(), cast to a pointer to the type of the
    named symbol, can be used to call (in the case of a function) or
    access the contents of (in the case of a data object) the named
    symbol.

OK! This is all I wanted to see.

(This language covers, for example, the inevitable requirement that the
I-Cache be flushed somewhere inside dlopen() and/or dlsym(); otherwise a
just-loaded function couldn't be called safely!)

> I suppose something like LTO could provide a compiler an opportunity
> to say that no .o linked into the final image modifies the variable
> directly, which may hurt nbdkit.* debug variables in the main
> executable, but I don't see how the compiler could ever optimize away
> access to globals in .so plugins (since the very nature of .so's is
> that they will be dynamically run by code that the compiler didn't
> see).

I don't like arguments of this sort. They all seem very logical, even
obvious... until the next major release of gcc or clang, that is. It's
not safe (albeit practical) when our arguments are limited by our own
imagination. I've lost trust in the direction in which compilers have
been developed.

>  This is all waaaaay outside of the bounds of what POSIX says,

In my opinion, it isn't; I did end up finding the important passage that
I missed (or failed to recall) earlier!

> so at best we are just relying on "does it work for how we are using
> it" - but since we are only using it for debugging, where it is doing
> what we want, I'm not too worried about trying to make this comply
> with some unwritten spec.  Sometimes, looking the other way when there
> is dark magic present is the prudent action.
> 

I do agree with your last statement 100%, in general!

Anyway, just to confirm: thanks for talking this through with me, I
agree that the code is safe, and it's even based on clear POSIX standardese.

Thanks!
Laszlo