[cfe-dev] BitstreamReader::GetCurrentBitNo() counterintuitive

[Ted Kremenek]

Part of the problem is that "Deserialize.cpp" (in llvm) uses  
GetCurrentBitNo() in a whole bunch of places.  AST serialization/ 
deserialization is the only client of the serialization library, and  
there aren't that many test cases in the clang test suite for AST  
serialization because it just hasn't been completed yet.  I have a  
feeling that changing the behavior of GetCurrentBitNo() will break  
things.

I'm actually a little surprised that GetCurrentBitNo() needs to be  
changed.  While the AST serialization work is incomplete, it has  
worked for non-trivial cases.  Since it uses GetCurrentBitNo() to jump  
around the bitcode file, I'm suspicious that anything needs to be  
changed.

Also, what happens if NextChar == FirstChar?  Wouldn't (NextChar- 
FirstChar-4)*8 be a negative number?

On Dec 23, 2008, at 11:39 AM, Chris Lattner wrote:

>
> On Dec 23, 2008, at 12:36 AM, Zhongxing Xu wrote:
>
>> As the comments said, BitstreamReader::GetCurrentBitNo() should
>> return the bit # of the bit we are reading. But the current
>> implementation actually return 4 more bytes than the bit we are
>> reading. It calculate the bit number by
>>
>> (NextChar-FirstChar)*8 + ((32-BitsInCurWord) & 31);
>>
>> But 'NextChar' is already after the word that we are reading. I
>> propose to fix this by the following patch.
>
> Hi Zhongxing,
>
> This is ok as long as it doesn't cause any regressions in the *llvm*
> dejagnu regression tests, thanks!
>
> -Chris
>
>>
>>
>> Index: include/llvm/Bitcode/BitstreamReader.h
>> ===================================================================
>> --- include/llvm/Bitcode/BitstreamReader.h    （版本 61317）
>> +++ include/llvm/Bitcode/BitstreamReader.h    （工作副本）
>> @@ -114,7 +114,8 @@
>>
>>   /// GetCurrentBitNo - Return the bit # of the bit we are reading.
>>   uint64_t GetCurrentBitNo() const {
>> -    return (NextChar-FirstChar)*8 + ((32-BitsInCurWord) & 31);
>> +    return (NextChar-FirstChar-4)*8 + (BitsInCurWord ? (32-
>> BitsInCurWord) & 31
>> +                                                     : 32);
>>   }
>>
>>   /// JumpToBit - Reset the stream to the specified bit number.
>> @@ -130,7 +131,6 @@
>>
>>     // Skip over any bits that are already consumed.
>>     if (WordBitNo) {
>> -      NextChar -= 4;
>>       Read(static_cast<unsigned>(WordBitNo));
>>     }
>>   }
>>
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev