[llvm] r231701 - DwarfAccelTable: fix obvious typo.

[David Blaikie]

On Mon, Mar 9, 2015 at 3:14 PM, Frédéric Riss <friss at apple.com> wrote:

>
> On Mar 9, 2015, at 2:56 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
>
> On Mon, Mar 9, 2015 at 2:45 PM, Frédéric Riss <friss at apple.com> wrote:
>
>>
>> On Mar 9, 2015, at 2:21 PM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>>
>>
>> On Mon, Mar 9, 2015 at 2:09 PM, Frederic Riss <friss at apple.com> wrote:
>>
>>> Author: friss
>>> Date: Mon Mar  9 16:09:50 2015
>>> New Revision: 231701
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=231701&view=rev
>>> Log:
>>> DwarfAccelTable: fix obvious typo.
>>>
>>> I have a test for that issue, but I didn't include it in the commit as
>>> it's
>>> a 200KB file for a pretty minor issue. (The reason the file is so big is
>>> that it needs > 1024 variables/functions to trigger and that with debug
>>> information.
>>>
>>> The issue/fix on the other side is totally trivial. If poeple want the
>>> test
>>> commited, I can do that. It just didn't seem worth it to me.
>>>
>>> Modified:
>>>     llvm/trunk/lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp
>>>
>>> Modified: llvm/trunk/lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp?rev=231701&r1=231700&r2=231701&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/AsmPrinter/DwarfAccelTable.cpp Mon Mar  9
>>> 16:09:50 2015
>>> @@ -54,7 +54,7 @@ void DwarfAccelTable::ComputeBucketCount
>>>    // Then compute the bucket size, minimum of 1 bucket.
>>>    if (num > 1024)
>>>      Header.bucket_count = num / 4;
>>> -  if (num > 16)
>>> +  else if (num > 16)
>>>      Header.bucket_count = num / 2;
>>>
>>
>> Should this whole if/else if chain just be rewritten in terms of a
>> continuous function from num to bucket count? (which could perhaps be unit
>> tested - perhaps the accel table in general could be unit tested & exercise
>> this case reasonably) - looks something like "num / ((log2 num + 1) / 2)"
>> or some such?
>>
>>
>> I found that typo because I’m using this code to emit the accelerator
>> tables in llvm-dsymutil and comparing them to the output of the ‘reference’
>> implementation which is in Darwin’s dsymutil.
>>
>
> Worth throwing some kind of C fuzzer at the two tools & searching for
> cases where the output is different? (if there's an easy way to constrain
> the input to produce exactly matching output between the two) Dunno - just
> a thought.
>
>
> Once I upstreamed all of llvm-dsymutil, we’ll be doing bit-for-bit
> comparison over large codebases (everything that’s easily testable
> internally), it’s not fuzzing but still pretty intensive validation. For
> the record, I found that and the hash collision issues by testing only on
> the clang binaries.
>

Cool


>
> Both implementations had the same logic for computing the number of
>> buckets (except for the typo in this code obviously), thus I think the
>> intention was really to limit the division factor to 4.
>>
>> What was the failure mode?
>>
>>
>> The resulting table was functional[*], you’d just end up with twice the
>> expected bucket count.
>>
>
> Thanks for the background. (well, as you go, maybe you'll find that some
> different/better forms of testing could be helpful - given an accelerator
> table should be somewhat ADT-like, I'd lean towards some unit testing,
> perhaps)
>
>
> I tend to agree with you, the main issue is that the output is done
> through the MC layer, so it is quite hard to test. Even if we’d have the
> ability to stream it to a buffer, it wouldn’t be easy to test. We could
> maybe rewrite all the logic to go to an intermediate form that would be
> programmatically testable… I’m really not sure it’s worth it.
>

Fair enough - just something to keep an eye out for opportunities as you go
through it, I suppose.


>
> Fred
>
>
>> Fred
>>
>> *: generally functional. In fact the implementation is totally broken
>> when there are hash collisions, which I’m going to address next.
>>
>
> Fun.
>
>
>>
>>    else
>>>      Header.bucket_count = num > 0 ? num : 1;
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150309/0939a46e/attachment.html>