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

[David Blaikie]

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)

>    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