[llvm] r231701 - DwarfAccelTable: fix obvious typo.

[David Blaikie]

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.


> 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)


>
> 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/787dd96e/attachment.html>