<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://reviewboard.asterisk.org/r/1549/">https://reviewboard.asterisk.org/r/1549/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 31st, 2011, 12:34 p.m., <b>mjordan</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://reviewboard.asterisk.org/r/1549/diff/2/?file=21505#file21505line1548" style="color: black; font-weight: bold; text-decoration: underline;">/branches/1.8/main/utils.c</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">static int add_string_pool(struct ast_string_field_mgr *mgr, struct ast_string_field_pool **pool_head,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1548</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="p">(</span><span class="n">pool</span> <span class="o">=</span> <span class="n">__ast_<span class="hl">c</span>alloc</span><span class="p">(</span><span class="mi"><span class="hl">1</span></span><span class="p"><span class="hl">,</span></span><span class="hl"> </span><span class="n">alloc_size</span><span class="p">,</span> <span class="n">file</span><span class="p">,</span> <span class="n">lineno</span><span class="p">,</span> <span class="n">func</span><span class="p">)))</span> <span class="p">{</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">1548</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span> <span class="p">(</span><span class="o">!</span><span class="p">(</span><span class="n">pool</span> <span class="o">=</span> <span class="n">__ast_<span class="hl">m</span>alloc</span><span class="p">(</span><span class="n">alloc_size</span><span class="p">,</span> <span class="n">file</span><span class="p">,</span> <span class="n">lineno</span><span class="p">,</span> <span class="n">func</span><span class="p">)))</span> <span class="p">{</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This actually goes against the recommendations in the coding guidelines (2.17.3) - is there a specific reason to not use ast_calloc?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The memset after the malloc is for a smaller amount of memory than the entire allocation. The change appears safe, but I question why we'd even care about this optimization when it adds more complexity.</pre>
<br />
<p>- David</p>
<br />
<p>On October 31st, 2011, 3:47 a.m., wdoekes wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://reviewboard.asterisk.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Asterisk Developers.</div>
<div>By wdoekes.</div>
<p style="color: grey;"><i>Updated Oct. 31, 2011, 3:47 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This patch fixes that Asterisk can be properly built on certain architectures that dislike misalignment. (In the case of the bug reporter, an ARM.)
==Background==
Currently the 16bit ast_string_field_allocation used in the is not aligned, it can be stored on an 8bit boundary. Certain machines will either SIGBUS over this or simply give wrong results. For the Sparc an #ifdef was added to alleviate the problem.
==Problems with current approach==
(1) The x86 can cope with misaligned integers, but for performance, aligned ints are better.
(2) The #ifdef did not catch all architectures that dislike misalignment.
(3) The code in the #ifdef falsely assumes that the ast_string_field_allocation is at most 2 bytes large. If this were to change one day, things would start to fail again.
==Possible fixes==
(1) Remove the #ifdef, always run the Sparc code and patch it to cope with larger than 16bit ast_string_field_allocation's.
(2) Alter all ast_string_field_allocation code to ensure that base and used stay aligned. Then we won't need to check and re-align later on.
I chose fix #2 because I believe this to be marginally faster and more logical. This does involve the use of the gcc __attribute__((aligned)). But the other code is full of gcc attributes, so I don't think I'm breaking a build anywhere with this.
Regards,
Walter</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I replaced:
typedef uint16_t ast_string_field_allocation;
with:
typedef uint64_t ast_string_field_allocation;
Then I looked at a small sample of base and used during operation.
They were always 64bit aligned.</pre>
</td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://issues.asterisk.org/jira/browse/ASTERISK-17310">ASTERISK-17310</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/branches/1.8/include/asterisk/utils.h <span style="color: grey">(342659)</span></li>
<li>/branches/1.8/main/utils.c <span style="color: grey">(342659)</span></li>
<li>/branches/1.8/include/asterisk/stringfields.h <span style="color: grey">(342659)</span></li>
</ul>
<p><a href="https://reviewboard.asterisk.org/r/1549/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>