[prev in list] [next in list] [prev in thread] [next in thread] 

List:       cfe-dev
Subject:    Re: [cfe-dev] Alignment problems in Clang's internal data structures?
From:       Reid Kleckner <rnk () google ! com>
Date:       2015-02-27 18:17:46
Message-ID: CACs=ty+9Qs3YOHF+GeiEwH8yRekd0TcGsV6jc7zRYjwKVeoxYw () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]


On Tue, Feb 17, 2015 at 1:42 AM, Renato Golin <renato.golin@linaro.org>
wrote:

> On 17 February 2015 at 00:38, Ahmed Bougacha <ahmed.bougacha@gmail.com>
> wrote:
> >   std::size_t Size = sizeof(DeclRefExpr);
> >   if (...)
> >     Size += sizeof(...);
> >   ...
> >   void *Mem = Context.Allocate(Size, llvm::alignOf<DeclRefExpr>());
>
> This sounds wrong. You should have something like:
>
>    std::size_t Size = sizeof(DeclRefExpr);
>    std::size_t Align = llvm::alignOf<DeclRefExpr>();
>    if (...) {
>      Size += sizeof(...);
>      Align = std::max(Align, llvm::alignOf<...>());
>    }
>    ...
>    void *Mem = Context.Allocate(Size, Align);


This isn't enough either. You need to update the accessors to account for
alignment. They currently look like this:

  NamedDecl *&getInternalFoundDecl() {
    assert(hasFoundDecl());
    if (hasQualifier())
      return *reinterpret_cast<NamedDecl **>(&getInternalQualifierLoc() +
1);
    return *reinterpret_cast<NamedDecl **>(this + 1);
  }

I do *not* want to audit Clang for the 'reinterpret_cast<Foo*>(this + 1)'
pattern and add extra alignment code. We should just make sure that
anything allocated this way requires only pointer-size alignment.

I put this on http://llvm.org/pr22727.

[Attachment #5 (text/html)]

<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Feb 17, 2015 \
at 1:42 AM, Renato Golin <span dir="ltr">&lt;<a href="mailto:renato.golin@linaro.org" \
target="_blank">renato.golin@linaro.org</a>&gt;</span> wrote:<br><blockquote \
class="gmail_quote" style="margin:0px 0px 0px \
0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span \
class="">On 17 February 2015 at 00:38, Ahmed Bougacha &lt;<a \
href="mailto:ahmed.bougacha@gmail.com">ahmed.bougacha@gmail.com</a>&gt; wrote:<br> \
&gt;     std::size_t Size = sizeof(DeclRefExpr);<br> &gt;     if (...)<br>
&gt;        Size += sizeof(...);<br>
&gt;     ...<br>
&gt;     void *Mem = Context.Allocate(Size, llvm::alignOf&lt;DeclRefExpr&gt;());<br>
<br>
</span>This sounds wrong. You should have something like:<br>
<span class=""><br>
     std::size_t Size = sizeof(DeclRefExpr);<br>
</span>     std::size_t Align = llvm::alignOf&lt;DeclRefExpr&gt;();<br>
<span class="">     if (...) {<br>
        Size += sizeof(...);<br>
</span>        Align = std::max(Align, llvm::alignOf&lt;...&gt;());<br>
     }<br>
     ...<br>
     void *Mem = Context.Allocate(Size, Align);</blockquote><div><br></div><div>This \
isn&#39;t enough either. You need to update the accessors to account for alignment. \
They currently look like this:</div><div><br></div><div><div>   NamedDecl \
*&amp;getInternalFoundDecl() {</div><div>      assert(hasFoundDecl());</div><div>     \
if (hasQualifier())</div><div>         return *reinterpret_cast&lt;NamedDecl \
**&gt;(&amp;getInternalQualifierLoc() + 1);</div><div>      return \
*reinterpret_cast&lt;NamedDecl **&gt;(this + 1);</div><div>   \
}</div></div><div><br></div><div>I do *not* want to audit Clang for the \
&#39;reinterpret_cast&lt;Foo*&gt;(this + 1)&#39; pattern and add extra alignment \
code. We should just make sure that anything allocated this way requires only \
pointer-size alignment.</div><div><br></div><div>I put this on <a \
href="http://llvm.org/pr22727">http://llvm.org/pr22727</a>.</div></div></div></div>



_______________________________________________
cfe-dev mailing list
cfe-dev@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


[prev in list] [next in list] [prev in thread] [next in thread]