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

[Ted Kremenek]

On Dec 23, 2008, at 10:41 PM, Zhongxing Xu wrote:

>
>
> On Wed, Dec 24, 2008 at 2:16 PM, Ted Kremenek <kremenek at apple.com>  
> wrote:
>
> On Dec 23, 2008, at 5:01 PM, Zhongxing Xu wrote:
>
>>
>>
>> On Wed, Dec 24, 2008 at 7:29 AM, Ted Kremenek <kremenek at apple.com>  
>> wrote:
>> 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?
>>
>> It will be negative. But in that case,  BitsInCurWord will be 0,  
>> the final result is 0.
>
> Okay.  If GetCurrentBitNo() is broken then it should be fixed.  I  
> personally haven't looked at it in detail recently, so I'm not  
> certain what the ramifications would be if we changed it or why it  
> seems to work correctly now (or maybe it doesn't?).
>
> I do think GetCurrentBitNo() is broken, because it is not returning  
> the bit # we are reading. But it works now, partially because we  
> have code like this:
>
>      if (WordBitNo) {
> -      NextChar -= 4;
>        Read(static_cast<unsigned>(WordBitNo));
>
> I don't know if there is other code doing the similar compensate work.


Hmm.  This is definitely worth investigating further.  At the very  
least there should be more comments documenting the invariants/ 
assumptions in the code with regards to the bit location.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20081223/20ca5406/attachment.html>