[llvm-commits] [llvm] r138636 - /llvm/trunk/lib/Target/ARM/Disassembler/ARMDisassembler.cpp

[Jim Grosbach]

On Aug 26, 2011, at 11:45 AM, Benjamin Kramer wrote:

> On Fri, Aug 26, 2011 at 11:37, Jim Grosbach <grosbach at apple.com> wrote:
>> 
>> On Aug 26, 2011, at 11:30 AM, Benjamin Kramer wrote:
>> 
>>> On Fri, Aug 26, 2011 at 11:26, Jim Grosbach <grosbach at apple.com> wrote:
>>>> Testcase?
>>> 
>>> It's not directly testable from llvm-mc, but the valgrind buildbot
>>> complains about uninitialized variables without this patch.
>> 
>> That's odd. Along what execution path? The MC dissassembler and the Enhanced disassembler both early exit when failure is returned and never reference the Size value. At least according to my reading the code, anyway. What am I missing?
> 
> The relevant code in llvm-mc's Disassembler.cpp:62 looks like this:

Ah, OK. That's why I didn't see it. llvm-mc is calling it directly, I was looking in the C bindings.
> 
>> uint64_t Size;
>> uint64_t Index;
>> 
>> for (Index = 0; Index < Bytes.size(); Index += Size) {
>>   MCInst Inst;
>> 
>>   MCDisassembler::DecodeStatus S;
>>   S = DisAsm.getInstruction(Inst, Size, memoryObject, Index,
>>                             /*REMOVE*/ nulls());
>>   switch (S) {
>>   case MCDisassembler::Fail:
>>     SM.PrintMessage(SMLoc::getFromPointer(Bytes[Index].second),
>>                     "invalid instruction encoding", "warning");
>>     if (Size == 0)
>>       Size = 1; // skip illegible bytes
>>     break;
> 
> Size is passed by reference to DisAsm.getInstruction. When
> getInstruction doesn't set Size in the first iteration then Index
> becomes an undefined value in the next iteration. Some of our tests
> contain only invalid bytes and valgrind will complain.


Makes sense. Thanks!

-Jim