llvm dev - Jul 2015 - [LLVMdev] [llvm] [libunwind] r207467 misprint

Hi Nick!

In r207467 you added code(libunwind: DwarfInstructions.hpp):

      assert(lastReg <= (int)cieInfo.returnAddressRegister
                 && "register range does not contain return address
register");
      for (int i = 0; i <= lastReg; ++i) {
.....
          else if (i == (int)cieInfo.returnAddressRegister)

There is misprint here: lastReg should be >(int)cieInfo.returnAddressRegister
in assert.

So patch is:

Index: DwarfInstructions.hpp
==================================================================---
DwarfInstructions.hpp (revision 242137)
+++ DwarfInstructions.hpp (working copy)
@@ -170,7 +170,7 @@
       const int lastReg = R::lastDwarfRegNum();
       assert((int)CFI_Parser<A>::kMaxRegisterNumber > lastReg
&&
              "register range too large");
-      assert(lastReg <= (int)cieInfo.returnAddressRegister &&
+      assert(lastReg >= (int)cieInfo.returnAddressRegister &&
              "register range does not contain return address
register");
       for (int i = 0; i <= lastReg; ++i) {
         if (prolog.savedRegisters[i].location !If you agree, can you please
commit it, because I have no commit access.

Thank you!
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150714/434958f7/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DwarfInstructions.hpp.patch
Type: application/octet-stream
Size: 659 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150714/434958f7/attachment.obj>

Daniil,

It looks like those asserts were added by Dan Albert.

225187  danalbert       assert((int)CFI_Parser<A>::kMaxRegisterNumber >
lastReg &&
225187  danalbert              "register range too large");
225187  danalbert       assert(lastReg <= (int)cieInfo.returnAddressRegister
&&
225187  danalbert              "register range does not contain return
address register");

The assert for the return register is complicated because stack based calling
conventions (like x86[_64]) don’t have a register for the return value.  Instead
the dwarf makes up a number (x86 uses 8) for the return address number.  This
assert happens to work as-is for x86[_64] because the return register number ==
lastReg.

-Nick

> On Jul 14, 2015, at 4:45 AM, Daniil Troshkov <troshkovdanil at
gmail.com> wrote:
> 
> Hi Nick!
>  
> In r207467 you added code(libunwind: DwarfInstructions.hpp):
>  
>       assert(lastReg <= (int)cieInfo.returnAddressRegister
>                  && "register range does not contain return
address register");
>       for (int i = 0; i <= lastReg; ++i) {
> .....
>           else if (i == (int)cieInfo.returnAddressRegister)
>  
> There is misprint here: lastReg should be >=
(int)cieInfo.returnAddressRegister in assert.
>  
> So patch is:
>  
> Index: DwarfInstructions.hpp
> ==================================================================> ---
DwarfInstructions.hpp (revision 242137)
> +++ DwarfInstructions.hpp (working copy)
> @@ -170,7 +170,7 @@
>        const int lastReg = R::lastDwarfRegNum();
>        assert((int)CFI_Parser<A>::kMaxRegisterNumber > lastReg
&&
>               "register range too large");
> -      assert(lastReg <= (int)cieInfo.returnAddressRegister &&
> +      assert(lastReg >= (int)cieInfo.returnAddressRegister &&
>               "register range does not contain return address
register");
>        for (int i = 0; i <= lastReg; ++i) {
>          if (prolog.savedRegisters[i].location !> If you agree, can you
please commit it, because I have no commit access.
>  
> Thank you!
> <DwarfInstructions.hpp.patch>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150715/af2295bf/attachment.html>

Looks like my change to that line was just a drive-by reformatting from
r225187. The assert was added in r207467.

On Wed, Jul 15, 2015 at 2:22 PM, Nick Kledzik <kledzik at apple.com>
wrote:
> Daniil,
>
> It looks like those asserts were added by Dan Albert.
>
> 225187  danalbert       assert((int)CFI_Parser<A>::kMaxRegisterNumber
>
> lastReg &&
> 225187  danalbert              "register range too large");
> 225187  danalbert       assert(lastReg <>
(int)cieInfo.returnAddressRegister &&
> 225187  danalbert              "register range does not contain return
> address register");
>
> The assert for the return register is complicated because stack based
> calling conventions (like x86[_64]) don’t have a register for the return
> value.  Instead the dwarf makes up a number (x86 uses 8) for the return
> address number.  This assert happens to work as-is for x86[_64] because the
> return register number == lastReg.
>
> -Nick
>
>
> On Jul 14, 2015, at 4:45 AM, Daniil Troshkov <troshkovdanil at
gmail.com>
> wrote:
>
> Hi Nick!
>
> In r207467 you added code(libunwind: DwarfInstructions.hpp):
>
>       assert(lastReg <= (int)cieInfo.returnAddressRegister
>                  && "register range does not contain return
address
> register");
>       for (int i = 0; i <= lastReg; ++i) {
> .....
>           else if (i == (int)cieInfo.returnAddressRegister)
>
> There is misprint here: lastReg should be >>
(int)cieInfo.returnAddressRegister in assert.
>
> So patch is:
>
> Index: DwarfInstructions.hpp
> ==================================================================> ---
DwarfInstructions.hpp (revision 242137)
> +++ DwarfInstructions.hpp (working copy)
> @@ -170,7 +170,7 @@
>        const int lastReg = R::lastDwarfRegNum();
>        assert((int)CFI_Parser<A>::kMaxRegisterNumber > lastReg
&&
>               "register range too large");
> -      assert(lastReg <= (int)cieInfo.returnAddressRegister &&
> +      assert(lastReg >= (int)cieInfo.returnAddressRegister &&
>               "register range does not contain return address
register");
>        for (int i = 0; i <= lastReg; ++i) {
>          if (prolog.savedRegisters[i].location !> If you agree, can you
please commit it, because I have no commit access.
>
> Thank you!
> <DwarfInstructions.hpp.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150715/0f9d080e/attachment.html>