[llvm] r188873 - MC CFG: Remap enough for the inserted instruction.

[Ahmed Bougacha]

On Wed, Aug 21, 2013 at 10:15 AM, David Blaikie <dblaikie at gmail.com> wrote:
> Just to echo Chandler's sentiments (made in reply to r188872) &
> provide some of my thoughts (which may or may not overlap with
> Chandler's) on a few of these patches...
>
> Consider my comments to this patch series as both review and
> meta-review, in many cases it would've been better if the answers to
> these questions had been obvious from the commits & code committed to
> make it easier for people reading code & performing archaeology on the
> commit history later.
>
> On Wed, Aug 21, 2013 at 12:27 AM, Ahmed Bougacha
> <ahmed.bougacha at gmail.com> wrote:
>> Author: ab
>> Date: Wed Aug 21 02:27:47 2013
>> New Revision: 188873
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=188873&view=rev
>> Log:
>> MC CFG: Remap enough for the inserted instruction.
>>
>> Modified:
>>     llvm/trunk/lib/MC/MCAtom.cpp
>>
>> Modified: llvm/trunk/lib/MC/MCAtom.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/MC/MCAtom.cpp?rev=188873&r1=188872&r2=188873&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/MC/MCAtom.cpp (original)
>> +++ llvm/trunk/lib/MC/MCAtom.cpp Wed Aug 21 02:27:47 2013
>> @@ -72,8 +72,8 @@ MCDataAtom *MCDataAtom::split(uint64_t S
>>  // MCTextAtom
>>
>>  void MCTextAtom::addInst(const MCInst &I, uint64_t Size) {
>> -  if (NextInstAddress > End)
>> -    remap(Begin, NextInstAddress);
>> +  if (NextInstAddress + Size - 1 > End)
>> +    remap(Begin, NextInstAddress + Size - 1);
>
> It's not clear what this fix was for - is this a future-proofing
> change? If so, I think it's sufficiently non-obvious that it might've
> been better off going in with whatever change required it.
> A fix to existing undefined behavior? Did we already have a test case
> that was hitting UB because of this? Could we add one, even though it
> wouldn't've necessarily failed before the change (if it would fail
> under Valgrind or ASan/UBSan/etc that would be great)

So, there’s no undefined behavior involved, the atom boundaries are
just incorrectly computed in somewhat rare cases (last instruction is
larger than 1 byte).
Looking at it again, the testcase is actually easy, r188923.

-- Ahmed

>>    Insts.push_back(MCDecodedInst(I, NextInstAddress, Size));
>>    NextInstAddress += Size;
>>  }
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits