[PATCH] D13189: Add support for sub-byte aligned writes to lib/Support/Endian.h

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Sep-29, at 13:41, Teresa Johnson <tejohnson at google.com> wrote:
> 
> On Tue, Sep 29, 2015 at 11:36 AM, Teresa Johnson <tejohnson at google.com> wrote:
>> On Tue, Sep 29, 2015 at 9:48 AM, Teresa Johnson <tejohnson at google.com> wrote:
>>> On Tue, Sep 29, 2015 at 9:43 AM, Duncan P. N. Exon Smith
>>> <dexonsmith at apple.com> wrote:
>>>> 
>>>>> On 2015-Sep-28, at 17:20, Teresa Johnson <tejohnson at google.com> wrote:
>>>>> 
>>>>> On Mon, Sep 28, 2015 at 5:12 PM, Duncan P. N. Exon Smith
>>>>> <dexonsmith at apple.com> wrote:
>>>>>> 
>>>>>>> On 2015-Sep-25, at 22:02, Teresa Johnson <tejohnson at google.com> wrote:
>>>>>>> 
>>>>>>> tejohnson created this revision.
>>>>>>> tejohnson added a reviewer: dexonsmith.
>>>>>>> tejohnson added a subscriber: llvm-commits.
>>>>>>> 
>>>>>>> As per Duncan's review for D12536, I extracted the sub-byte bit aligned
>>>>>>> reading and writing code into lib/Support. Added calls from
>>>>>>> BackpatchWord.
>>>>>>> 
>>>>>>> I made the support general instead of tailoring it to the specific
>>>>>>> use case and data type in the BackpatchWord case. However, I am not
>>>>>>> sure how to add unit testing for the other data types. I did test it
>>>>>>> for signed types with negative values to ensure the shifting/masking
>>>>>>> handled the sign bit properly, by temporarily adding some write/read
>>>>>>> calls into llvm operating on a temporary array.
>>>>>>> 
>>>>>>> http://reviews.llvm.org/D13189
>>>>>>> 
>>>>>>> Files:
>>>>>>> include/llvm/Bitcode/BitstreamWriter.h
>>>>>>> include/llvm/Support/Endian.h
>>>>>>> 
>>>>>>> <D13189.35794.patch>
>>>>>> 
>>>>>> I wonder if these would be hard to unit-test?  If not, it might be nice
>>>>>> to add one or two.  I imagine if someone inserts a bug, it'll be easier
>>>>>> to track down unit test failures than bitcode tests.  I'm not too
>>>>>> concerned, since this is tested implicitly by the bitcode tests.
>>>>> 
>>>>> It would be great to unittest them, I'm just not sure how, especially
>>>>> for any other data types like signed, since the calls just don't exist
>>>>> for that case yet. For the unsigned 32-bit case being called in
>>>>> BackpatchWord, I don't think we need unittests since it is tested
>>>>> pretty heavily by all the bitcode tests. I'm also not really sure how
>>>>> to unittest it in that case anyway? The alignment of the calls to
>>>>> BackpatchWord aren't easily controllable (I could create a test that
>>>>> causes BackpatchWord to be called with a particular alignment today,
>>>>> but as bitcode changes that alignment will drift).
>>>> 
>>>> I wasn't suggesting unit testing BackpatchWord() -- rather, the new utility functions you have here in lib/Support (somewhere in unittest/Support I imagine).
>>>> 
>>>> You could write to a char array and check the contents, or put something in a char array and read from it.  It'll take some care to make the tests valid for both little endian and big endian hosts, but that's probably just some byte swap logic.
>>> 
>>> That's what I did when I was testing it - but I manually added these
>>> calls into LLVM in a random place where I knew they would get
>>> executed. My question was where/how to add calls to these new routines
>>> so that they are unittested. I.e. the LLVM tests I am familiar with
>>> just use bitcode and run a tool like llvm-as, opt, etc to compile it.
>>> Can you point me at an example where we add calls to other lib/
>>> routines just for unittesting purposes so I can see how to do that?
>> 
>> I poked around some more and found that there are some unittests like
>> this in lib/Fuzzer/test (and some others in clang/unittests/) that use
>> gtest, so I could do something like that. I.e. create a
>> lib/Support/test directory that is similar to lib/Fuzzer/test in
>> setup, and create some tests that invoke the new routines and test
>> them on some arrays. Let me know if you meant something different.
> 
> Nevermind, found the Support unittests directory, will create some
> tests. Sorry for the confusion.

Great!