<html><head></head><body>Hello!<br><br>Just a quick reply from my phone. When adding these checks, I also wanted to zero out slab objects but then I realized that these objects should be initialized anyway after allocation and in most cases all of these would be rewritten twice.<br><br>The preferred way of using slab objects is therefore a full init by structure assignment after alloc like this:<br><br>struct foo *f = sl_alloc(...);<br>*f = (struct foo) {...};<br><br>In cases of other allocations, there are allocz variants to zero the allocated memory instead of having to call memset, yet slabs are intended to be a fixed-size structure allocator which corresponds to a possibility of direct structure assignment.<br><br>I hope this is sufficient explanation. Feel free to dispute it or discuss anyway, I may be wrong somehow.<br><br>Maria<br><br><div class="gmail_quote">On November 22, 2020 7:01:52 PM GMT+01:00, "Toke Høiland-Jørgensen" <toke@toke.dk> wrote:<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">andreas@rammhold.de writes:<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;">On 18:21 22.11.20, Toke Høiland-Jørgensen wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">andreas@rammhold.de writes:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">I've been playing with the BIRD source code and implementing a new<br>extension to Babel but that is not what this mail is about.<br></blockquote><br>Still curious - which extension? :)<br></blockquote><br> The V4 over/via V6 draft. I would link to it but it seems the IETF<br> website is currently unreachable (or I am being cloudflared…).<br></blockquote><br>Ah, cool! That was on my list as well, but good to know you're working<br>on it! I'll let you have at it, then :)<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">Breakpoint 1, add_tail (n=0x234ac30, l=0x241f620) at ./lib/lists.c:82<br>#0 add_tail (n=0x234ac30, l=0x241f620) at ./lib/lists.c:82<br>#1 sl_alloc (s=0x21bd7d0) at lib/slab.c:264<br></blockquote><br>This is that call to add_tail in slab.c, with a few lines of context:<br><br>full_partial:<br> rem_node(&h->n);<br> add_tail(&s->full_heads, &h->n);<br> goto redo;<br><br>Since add_tail is being called immediately after rem_node(), and they I<br>both inlines, my guess would be that this comes from the compiler<br>optimising out the clearing of n->prev and n->next in rem_node() since<br>they are immediately being reset.<br></blockquote><br> Interesting. The compiler removes the first assignment becuase it is<br> rewritten (across function boundaries/after inlining) but doesn't<br> realize that it is being read?<br></blockquote><br>Hmm, yeah, good point, if you have a read in-between that optimisation<br>really shouldn't happen - but maybe the compiler is rearranging things<br>in other interesting ways? I guess you'd have to inspect the generated<br>code to know for sure...<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;"><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #8ae234; padding-left: 1ex;">Are these all false positives or should there be more cleanup code on<br>these list entries?<br></blockquote><br>Speaking for the backtraces you saw in the Babel code, these all come<br>from variants of the following pattern:<br><br>struct babel_msg_node *msgn = sl_alloc(p->msg_slab);<br>/* initialise msgn->msg */<br>add_tail(&queue, NODE msgn);<br><br>where babel_msg_node is just:<br><br>struct babel_msg_node {<br> node n;<br> union babel_msg msg;<br>};<br><br>Since add_tail sets n->next and n->prev right after those asserts, no<br>harm is actually done and the memory ends up fully initialised, so I'm<br>not sure if there's any reason to do anything about it: It's really<br>only a problem because of the debug asserts.<br></blockquote><br> That was my hope as well. Maybe there are some attributes / qualifiers<br> that could be added to the variables (when the debug flag is set) to<br> prohibit this optimisation? Having these checks is probably useful (e.g.<br> for CI, tests & development) but only if it doesn't break the entire<br> software.<br></blockquote><br>Yeah, it is a bit annoying that it breaks debug builds. My preference<br>would be to just have the slab code zero out objects, but let's see what<br>the Bird devs think :)<br><br>-Toke<br><br></pre></blockquote></div><br>-- <br>Sent from my Android device with K-9 Mail. Please excuse my brevity.</body></html>