[llvm-commits] [PATCH] PR12696: Encode 'nonlazybind' and 'address_safety' properly in BitcodeWriter

[Meador Inge]

On Fri, May 4, 2012 at 12:25 PM, Chris Lattner <clattner at apple.com> wrote:

> On May 3, 2012, at 10:26 PM, Meador Inge wrote:
>
>> Hi All,
>>
>> This patch fixes PR12696 which reported that the BitcodeWriter was not
>> encoding the
>> 'nonlazybind' and 'address_safety' attributes correctly.  Alessandro
>> Pignotti suggested
>> the original patch.  I wrote the test case and regression tested the change.
>
> This looks very close, but I don't like the magic numbers in the bitcode writer.  If there is no convenient enum in Attributes.h, can you add a new enum
> with the magic number there?

Which magic numbers, though?  In this particular case there are many
magic numbers used for shifting and masking.  The complication arises
from the fact that we are picking apart the attribute set to expand
the alignment and then encoding that expanded alignment with the other
attributes (a similar problem for decoding exist in the bitcode
reader).  If the magic numbers were to be replaced for this case, then
you would probably want enum/constants for all the masks and shifts.
I think that is overkill.  Also, you can only go so for hiding the
magic for bit twiddling stuff like this.

One alternative is to hoist the operations out of the bitcode
implementation into encoding/decoding inline functions in
Attributes.h.  This way the bit twiddling is consolidated into a
single place as a part of the attributes implementation (which we
already do for similar operations like constructing alignments).  The
attached patch implements this approach and extends the test case to
cover all function and parameter attributes.

Also, maybe addressing the "// FIXME: remove in LLVM 3.0" FIXMEs would
simplify things.  Although, I am not completely sure what needs to be
done to address those FIXMEs.  If (when) they are addressed at least
with the inline function approach the places being fixed will be right
next to each other :-)

-- Meador
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr12696.patch
Type: application/octet-stream
Size: 6937 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120504/904dd9e3/attachment.obj>