[Swan-dev] const version of str_*()

[Andrew Cagney]

On Mon, 16 Sep 2019 at 02:22, D. Hugh Redelmeier <hugh at mimosa.com> wrote:

> | From: Andrew Cagney <andrew.cagney at gmail.com>
>
> Nice analysis.
>
> | But here's a different problem.  We've code micro-optimized as follows:
> |
> |   char buf[ID_LEN]
> |   idtoa(&id1, buf);
> |
> |   if (dbg) printf("id1 = %s\n", buf);
> |   ...
> |   if (dbg) printf("id 2 = %s\n", buf);
> |
> | presumably to minimise the number of string conversions.  Except, with
> | certain irony, when !dbg and things matter, it will make an
> | unnecessary call.
>
> Generally those are cases where I was not bold enough to eliminate
> that code when I converted to the new clearer routines.
>
> I support your being bold.  (Except when I disagree :-)
>
> Sometimes these things have been moved outside loops.  But even in
> these cases your new routine would be helpful without moving the call
> into the loop.
>

... if it isn't a debug-log



> | However, I wonder if we we can tweak this further and eliminate the
> | need to declare the buffers; and the desire to optimise, vis:
> |
> |    id_buf id_str(const id_t *id) __attribute__((const))
> |    ...
> |    printf("id1 = %s\n", id_str(&id1).buf);
> |    printf("id2 = %s\n", id_str(&id2).buf);
> |    printf("id1 = %s\n", id_str(&id1).buf);
>
> I like this idea, but only if you drop your objection to inline
> functions.
>
>
A function that dereferences a pointer arg cannot have
> __attribute__((const)).  So you must adjust the id_str function to
> receive the id_t "by-value".
>
>
Ah, this clause:

Note that a function that has pointer arguments and examines the data
pointed to must *not* be declared const if the pointed-to data might change
between successive invocations of the function. In general, since a
function cannot distinguish data that might change from data that cannot,
const functions should never take pointer or, in C++, reference arguments.
Likewise, a function that calls a non-const function usually must not be
const itself.

good catch, but it honestly leaves me confused.  I'm guessing it's a
limitation of the compiler - it doesn't track if the values contents
changed between invocations
https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes

Fortunately we've plan B: "pure".  It does seem to be a better fit:

Some common examples of pure functions are strlen or memcmp.



> id_buf id_str(const id_t id) __attribute__((const))
>
> (With inlining there should be no cost to this change.)
>
>
Using pure removes the need for this.

BTW, I'm not a particular fan of how all the functions take const type
pointers rather than values - I've been holding my nose.

However:

- the compiler already in-lines jam_<type>() into str_<type>() (I know this
from debugging);  forcing inline <type>_str() would just bloat the calling
functions (and the added code would be "cold" as it is typically if(debug))

- modern ABIs, as in anything from the '90's on, when given "struct s v",
stuff small struts into registers and pass large ones by reference - the
'80's dogma of 'const struct *' being faster is dead

OTOH, for both of these we'll never see any measurable benefit - Pluto
spends its time computing DH and/or performing increasingly obscure
O(#STATE) operations.


But there's another problem.
>
> I don't think that C guarantees that the id_buf value would live long
> enough.  The expression is more explicitly written
>
>         &id_str(&id1).buf[0]
>
>
I wondered, but figured C couldn't be so dumb.
Clearly I'm wrong.

As soon as the & is applied, I believe that C's semantics allows the
> temporary id_buf value to be discarded by the implementation.  Before
> the pointer is passed to printf.  So the pointer must not be
> dereferenced.  Thus this code probably ventures into the dreaded
> "undefined behaviour".
>
> In my brief participation in the C++ Standards process, I learned that
> this was called the "lifetime of temporaries problem".  It's a real
> mess in C++ but I think that it has been nailed down (with an
> arbitrary hack).  There's much less need in C so I don't think that it
> has been addressed.
>
> Summary C++ has a hack that probably makes your function correct but C
> does not.  Or more correctly, the idiomatic use of the function --
> the function itself is well-defined.
>
> See, for example
> <
> https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.cbclx01/lifetime_temp.htm
> >
>
> A less ambitious use of your function might still be an improvement
> over the current code.  Perhaps not worth the effort to refactor.
>
>         const id_buf b1 = id_str(id1);
>         printf("id1 = %s\n", b1.buf);
>
>         const id_buf b2 = id_str(id2);
>         printf("id2 = %s\n", b2.buf);
>
>         const id_buf b3 = id_str(id3);
>         printf("id3 = %s\n", b3.buf);
>
>
Perhaps.  It runs the risk of someone applying the "obvious" cleanup:
     printf("id3 = %s\n", id_str(id3));
oops.

Another bug I forgot to mention is:

   const char *s = "foo";
   if (val != NULL) {
      type_buf buf; /* XXX: must be at same scope as S */
      s = str_type(&valu, &buf);
   }


| https://lwn.net/Articles/285332/
> |
> | - the need to declare the buffer is gone
> | - the compiler can eliminate duplicate calls
> | - the odds of passing wrong buffers is reduced further
>
> Yes.
>
> I thought that C was going to adopt something like that but I don't
> immediately see anything in the C-11 standard.
>
> | it should also be efficient.  Small structures are returned in 1-2
> | registers; and larger structures are returned by reference, the above
> | is compiled as:
> |
> |    id_buf tmp;
> |    str_id(&tmp, &id)
> |    tmp.buf
> |
> | making it equivalent to the old str_id() call.
>
> This operational model is not the definition of C.  I'm against
> exploiting something that is technically Undefined Behaviour.
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20190916/290e8c55/attachment.html>