Re: [PATCH] optimize explicit_bzero for size

[Rich Felker <dalias@libc.org>]

On Thu, Jun 28, 2018 at 08:57:29PM +0300, Alexander Monakov wrote:
> Avoid saving/restoring the incoming argument by reusing memset return value.
> ---
> 
> I think it's unfortunate that the commit adding explicit_bzero does not say
> the rationale for the magic empty asm; LTO being the "obvious" explanation,
> of course, IMHO is not a reason to omit the explanation.

LTO is the only plausible "mechanical" reason I know of, but formally
it's just about producing a dependency on the stores.

> Does it imply an
> intention to support LTO,

Yes, that's always been the intention. Last I checked there was a
linker-side bug whereby LTO broke crt1.c (due to failure to consider
the reference from the file-scope asm) which we might want to
workaround by forcibly disabling LTO for startfiles, but the intent is
that everything in musl be correct without relying on extern calls as
some sort of magic barriers.

> and if so, would other magic asms elsewhere be
> accepted if they help with LTO issues?

Ideally no -- explicit_bzero is special because its whole purpose is
to do something that doesn't really make sense in the abstract model
of the language, but that's nonetheless desired for real-world
hardening. I'm not aware of anything else like that. If there are
places where there's a real problem caused by "lack of barriers" with
LTO, we should first try to fix it in a way that's correct with regard
to the abstract model, I think.

>  src/string/explicit_bzero.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/string/explicit_bzero.c b/src/string/explicit_bzero.c
> index 3d270040..f2e12f23 100644
> --- a/src/string/explicit_bzero.c
> +++ b/src/string/explicit_bzero.c
> @@ -3,6 +3,6 @@
>  
>  void explicit_bzero(void *d, size_t n)
>  {
> -	memset(d, 0, n);
> +	d = memset(d, 0, n);
>  	__asm__ __volatile__ ("" : : "r"(d) : "memory");
>  }
> -- 
> 2.11.0

Or if you like it:

-	memset(d, 0, n);
-	__asm__ __volatile__ ("" : : "r"(d) : "memory");
+	__asm__ __volatile__ ("" : : "r"(memset(d, 0, n)) : "memory");

Not sure if this is nice or hideous... ;-)

Rich