<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, 16 Sep 2019 at 02:22, D. Hugh Redelmeier <<a href="mailto:hugh@mimosa.com" target="_blank">hugh@mimosa.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">| From: Andrew Cagney <<a href="mailto:andrew.cagney@gmail.com" target="_blank">andrew.cagney@gmail.com</a>><br>
<br>
Nice analysis.<br>
<br>
| But here's a different problem. We've code micro-optimized as follows:<br>
| <br>
| char buf[ID_LEN]<br>
| idtoa(&id1, buf);<br>
| <br>
| if (dbg) printf("id1 = %s\n", buf);<br>
| ...<br>
| if (dbg) printf("id 2 = %s\n", buf);<br>
| <br>
| presumably to minimise the number of string conversions. Except, with<br>
| certain irony, when !dbg and things matter, it will make an<br>
| unnecessary call.<br>
<br>
Generally those are cases where I was not bold enough to eliminate<br>
that code when I converted to the new clearer routines.<br>
<br>
I support your being bold. (Except when I disagree :-)<br>
<br>
Sometimes these things have been moved outside loops. But even in<br>
these cases your new routine would be helpful without moving the call<br>
into the loop.<br></blockquote></div><div class="gmail_quote"><br></div><div class="gmail_quote">... if it isn't a debug-log</div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
| However, I wonder if we we can tweak this further and eliminate the<br>
| need to declare the buffers; and the desire to optimise, vis:<br>
| <br>
| id_buf id_str(const id_t *id) __attribute__((const))<br>
| ...<br>
| printf("id1 = %s\n", id_str(&id1).buf);<br>
| printf("id2 = %s\n", id_str(&id2).buf);<br>
| printf("id1 = %s\n", id_str(&id1).buf);<br>
<br>
I like this idea, but only if you drop your objection to inline<br>
functions.<br>
<br></blockquote><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
A function that dereferences a pointer arg cannot have<br>
__attribute__((const)). So you must adjust the id_str function to<br>
receive the id_t "by-value".<br>
<br></blockquote><div><br></div><div>Ah, this clause:</div><div><br></div><div style="margin-left:40px">Note that a function that has pointer arguments and examines the data
pointed to must <i>not</i> be declared <code>const</code> 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.</div><div><br></div><div>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<br></div><div><a href="https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes" target="_blank">https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes</a></div><div><br></div><div>Fortunately we've plan B: "pure". It does seem to be a better fit:</div><div><br></div><div style="margin-left:40px">Some common examples of pure functions are <code>strlen</code> or <code>memcmp</code>.<br></div><div><br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
id_buf id_str(const id_t id) __attribute__((const))<br>
<br>
(With inlining there should be no cost to this change.)<br>
<br></blockquote><div><br></div><div>Using pure removes the need for this.<br></div><div><br></div><div>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.<br></div><div><br></div><div>However:<br></div><div><br></div><div>- 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))<br></div><div><br></div><div>- 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<br></div><div><br></div><div>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.</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
But there's another problem.<br>
<br>
I don't think that C guarantees that the id_buf value would live long<br>
enough. The expression is more explicitly written<br>
<br>
&id_str(&id1).buf[0]<br>
<br></blockquote><div><br></div><div>I wondered, but figured C couldn't be so dumb.</div><div>Clearly I'm wrong.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
As soon as the & is applied, I believe that C's semantics allows the<br>
temporary id_buf value to be discarded by the implementation. Before<br>
the pointer is passed to printf. So the pointer must not be<br>
dereferenced. Thus this code probably ventures into the dreaded<br>
"undefined behaviour".<br>
<br>
In my brief participation in the C++ Standards process, I learned that<br>
this was called the "lifetime of temporaries problem". It's a real<br>
mess in C++ but I think that it has been nailed down (with an<br>
arbitrary hack). There's much less need in C so I don't think that it<br>
has been addressed.<br>
<br>
Summary C++ has a hack that probably makes your function correct but C<br>
does not. Or more correctly, the idiomatic use of the function --<br>
the function itself is well-defined.<br>
<br>
See, for example<br>
<<a href="https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.cbclx01/lifetime_temp.htm" rel="noreferrer" target="_blank">https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.3.0/com.ibm.zos.v2r3.cbclx01/lifetime_temp.htm</a>><br>
<br>
A less ambitious use of your function might still be an improvement<br>
over the current code. Perhaps not worth the effort to refactor.<br>
<br>
const id_buf b1 = id_str(id1);<br>
printf("id1 = %s\n", b1.buf);<br>
<br>
const id_buf b2 = id_str(id2);<br>
printf("id2 = %s\n", b2.buf);<br>
<br>
const id_buf b3 = id_str(id3);<br>
printf("id3 = %s\n", b3.buf);<br>
<br></blockquote></div><div class="gmail_quote"> </div><div class="gmail_quote">Perhaps. It runs the risk of someone applying the "obvious" cleanup:<br></div><div class="gmail_quote"> printf("id3 = %s\n", id_str(id3));</div><div class="gmail_quote">oops.<br></div><div class="gmail_quote"><br></div><div class="gmail_quote">Another bug I forgot to mention is:</div><div class="gmail_quote"><br></div><div class="gmail_quote"> const char *s = "foo";</div><div class="gmail_quote"> if (val != NULL) {</div><div class="gmail_quote"> type_buf buf; /* XXX: must be at same scope as S */<br></div><div class="gmail_quote"> s = str_type(&valu, &buf);<br></div><div class="gmail_quote"> }</div><div class="gmail_quote"><br></div><div class="gmail_quote"><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
| <a href="https://lwn.net/Articles/285332/" rel="noreferrer" target="_blank">https://lwn.net/Articles/285332/</a><br>
| <br>
| - the need to declare the buffer is gone<br>
| - the compiler can eliminate duplicate calls<br>
| - the odds of passing wrong buffers is reduced further<br>
<br>
Yes.<br>
<br>
I thought that C was going to adopt something like that but I don't<br>
immediately see anything in the C-11 standard.<br>
<br>
| it should also be efficient. Small structures are returned in 1-2<br>
| registers; and larger structures are returned by reference, the above<br>
| is compiled as:<br>
| <br>
| id_buf tmp;<br>
| str_id(&tmp, &id)<br>
| tmp.buf<br>
| <br>
| making it equivalent to the old str_id() call.<br>
<br>
This operational model is not the definition of C. I'm against<br>
exploiting something that is technically Undefined Behaviour.<br>
_______________________________________________<br>
Swan-dev mailing list<br>
<a href="mailto:Swan-dev@lists.libreswan.org" target="_blank">Swan-dev@lists.libreswan.org</a><br>
<a href="https://lists.libreswan.org/mailman/listinfo/swan-dev" rel="noreferrer" target="_blank">https://lists.libreswan.org/mailman/listinfo/swan-dev</a><br>
</blockquote></div></div>