[llvm] r231701 - DwarfAccelTable: fix obvious typo.

[Frédéric Riss]

> 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 <mailto:friss at apple.com>> wrote:
> 
>> On Mar 9, 2015, at 2:21 PM, David Blaikie <dblaikie at gmail.com <mailto:dblaikie at gmail.com>> wrote:
>> 
>> 
>> 
>> On Mon, Mar 9, 2015 at 2:09 PM, Frederic Riss <friss at apple.com <mailto: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 <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 <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.

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

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 <mailto:llvm-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <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/bfc246b8/attachment.html>