This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][MIPS] Fix offsets of all register sets and add MSA regset and FRE=1 mode support.
ClosedPublic

Authored by slthakur on Jul 3 2015, 3:10 AM.

Details

Summary

This patch :

  • Fixes offsets of all register sets for Mips.
  • Adds MSA register set and FRE=1 mode support for FP register set.
  • Separates lldb register numbers and register infos of freebsd/mips64 from linux/mips64.
  • Re-orders the register numbers of all kinds for mips to be consistent with freebsd order of register numbers.
  • Eliminates ENABLE_128_BIT_SUPPORT and union ValueData from Scalar.cpp and uses llvm::APInt and llvm::APFloat for all integer and floating point types.

Diff Detail

Repository
rL LLVM

Event Timeline

slthakur updated this revision to Diff 29005.Jul 3 2015, 3:10 AM
slthakur retitled this revision from to [LLDB][MIPS] Fix offsets of all register sets and add MSA regset and FRE=1 mode support..
slthakur updated this object.
slthakur added reviewers: clayborg, emaste, jaydeep.
slthakur set the repository for this revision to rL LLVM.
slthakur updated this revision to Diff 29017.Jul 3 2015, 7:04 AM

Removed #if define ENABLE_128_BIT_SUPPORT from reg infos and UserArea structure as it is not required there.

clayborg requested changes to this revision.Jul 6 2015, 9:59 AM
clayborg edited edge metadata.

I haven't been too happy about our current 128 bit integer support (before this patch). We should probably try to fix this in a way that works on all platforms and get rid of the ENABLE_128_BIT_SUPPORT all together. We can probably use a llvm::APInt in lldb_private::Scalar and get support for integers of any size. We just need to convert many of the functions in lldb_private::Scalar to use the llvm::APInt class and implement the C type promotion rules correctly when using this class. Because if you make a MIPS debugger on a 32 bit system, we don't want the debugger to fail to show certain types or perform DWARF expressions (since they use a stack of lldb_private::Value objects which contain a lldb_private::Scalar objects)...

It would be great to take care of so the debugger doesn't just drop some types on the floor when there is no 128 bit integer support natively on the current machine running LLDB.

This revision now requires changes to proceed.Jul 6 2015, 9:59 AM
slthakur updated this revision to Diff 29534.Jul 13 2015, 2:14 AM
slthakur edited edge metadata.
  • Used llvm:APInt for 128 bit integers as suggested by @clayborg.
clayborg requested changes to this revision.Jul 13 2015, 10:32 AM
clayborg edited edge metadata.

So Scalar can switch to using llvm::APInt for all integer values. Right now there are functions that assign and use llvm::APInt, but they don't guarantee that these llvm::APInt values are actually 128 bit. We should be able to switch over for all integers. So this:

union ValueData
{
    int                 sint;
    unsigned int        uint;
    long                slong;
    unsigned long       ulong;
    long long           slonglong;
    unsigned long long  ulonglong;
    float               flt;
    double              dbl;
    long double         ldbl;
};

Could end up being:

llvm::APInt m_integer;
llvm::APFloat m_float;

We get rid of the union here since they are classes. Then we should be able to update all the code to use the "m_integer" or "m_float" to represent integers and floats of any size. We just need to implement type promotion and ensure that it works for signed/unsigned types correctly...

This revision now requires changes to proceed.Jul 13 2015, 10:32 AM
slthakur updated this revision to Diff 30444.Jul 22 2015, 8:49 PM
slthakur updated this object.
slthakur edited edge metadata.
  • Eliminated ValueData from Scalar and used llvm::APInt and llvm::APFloat for all integer and floating point types.
slthakur updated this revision to Diff 30466.EditedJul 23 2015, 3:26 AM
slthakur edited edge metadata.

Increasing patch context and adding #defines for constants

clayborg requested changes to this revision.Jul 23 2015, 2:42 PM
clayborg edited edge metadata.

Great stuff in Scalar. One more thing to fix is that RegisterValue should probably get rid of its union and use have the following ivars:

RegisterValue::Type m_type;
Scalar m_scalar;
struct 
{
    uint8_t bytes[kMaxRegisterByteSize]; // This must be big enough to hold any register for any supported target.
    uint8_t length;
    lldb::ByteOrder byte_order;
} buffer;

This would simplify a bunch of stuff in RegisterValue where it had its own union. Then any use of "m_data.uintXXX" or "m_data.ieee_XXX" in RegisterValue should use m_scalar. If we can encapsulate that, this should be good to go.

include/lldb/Core/Scalar.h
118–119

We shouldn't need this function anymore. APInt can support integers of any size...

346

Why do we need m_long_double?

This revision now requires changes to proceed.Jul 23 2015, 2:42 PM

Hi,

One more thing to fix is that RegisterValue should probably get rid of its union and use have the following ivars:

RegisterValue::Type m_type;
Scalar m_scalar;
struct
{

uint8_t bytes[kMaxRegisterByteSize]; // This must be big enough to hold any register for any supported target.
 uint8_t length;
 lldb::ByteOrder byte_order;

} buffer;

This would simplify a bunch of stuff in RegisterValue where it had its own union. Then any use of "m_data.uintXXX" or >"m_data.ieee_XXX" in RegisterValue should use m_scalar.

I will updated the patch with the required changes in RegisterValue. Thanks for the comments.

include/lldb/Core/Scalar.h
346

I could not see a generic float semantic for long doubles in llvm::APFloat.
The two semantics that support long doubles are x87DoubleExtended and IEEEquad.
How do I distinguish between these two semantics according to architecture in Scalar?
Could I use IEEEquad for all architectures?

We should detect the float type from the architecture. We might need to specify an extra parameter when setting a Scalar to a long double and specify if it is to use the x87DoubleExtended or IEEEquad. Each clang::ASTContext already has been setup with a correct architecture and the clang::ASTContext has the float semantics for each different type. We should require clients of Scalar to properly specify the type of float, or only allow people to set float values in the Scalar class using llvm::APFloat. This would leave it up to the user to set the float correctly using the float semantics in the clang::ASTContext before setting the value into the Scalar...

include/lldb/Core/Scalar.h
57–59

Possible solution here is to:

  • change all these constructors over to take a llvm::APFloat
  • change the long double constructor to also take a bool that indicates if this should be an x87DoubleExtended or IEEEquad...
slthakur updated this revision to Diff 30896.Jul 29 2015, 5:34 AM
slthakur edited edge metadata.

Addressed review comments

slthakur updated this revision to Diff 30918.Jul 29 2015, 9:44 AM
slthakur edited edge metadata.
  • Removed Scalar::GetMaxByteSize() function.
  • Removed SetType() from Scalar as it is not needed there.
  • Fixed some code in NativeRegisterContextLinux_mips64.cpp.
clayborg accepted this revision.Jul 29 2015, 10:36 AM
clayborg edited edge metadata.

Thanks for bearing with me and fixing everything. Looks good.

This revision is now accepted and ready to land.Jul 29 2015, 10:36 AM
slthakur closed this revision.Aug 6 2015, 11:41 PM

Committed in revision 244308

Hi,

This CL caused bunch of tests to fail - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/4949
For example, TestLldbGdbServer.py is failing on Linux x86_64 - it seems, GPR value is sent as 8-byte value regardless of register's size - for example:

0000000000000000 - 32, {'set': 'General Purpose Registers', 'name': 'eax', 'format': 'hex', 'bitsize': '32', 'encoding': 'uint', 'container-regs': '0', 'offset': '80'}
0000000000000000 - 8, {'set': 'General Purpose Registers', 'name': 'ah', 'format': 'hex', 'bitsize': '8', 'encoding': 'uint', 'container-regs': '0', 'offset': '81'}

It sued to be:
00000000 - 32, {'set': 'General Purpose Registers', 'name': 'eax', 'format': 'hex', 'bitsize': '32', 'encoding': 'uint', 'container-regs': '0', 'offset': '80'}
00 - 8, {'set': 'General Purpose Registers', 'name': 'ah', 'format': 'hex', 'bitsize': '8', 'encoding': 'uint', 'container-regs': '0', 'offset': '81'}

Could you take a look into this failures or revert CL for now?

Hi,

This CL caused bunch of tests to fail - http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/4949
For example, TestLldbGdbServer.py is failing on Linux x86_64 - it seems, GPR value is sent as 8-byte value regardless of register's size - for example:

0000000000000000 - 32, {'set': 'General Purpose Registers', 'name': 'eax', 'format': 'hex', 'bitsize': '32', 'encoding': 'uint', 'container-regs': '0', 'offset': '80'}
0000000000000000 - 8, {'set': 'General Purpose Registers', 'name': 'ah', 'format': 'hex', 'bitsize': '8', 'encoding': 'uint', 'container-regs': '0', 'offset': '81'}

It sued to be:
00000000 - 32, {'set': 'General Purpose Registers', 'name': 'eax', 'format': 'hex', 'bitsize': '32', 'encoding': 'uint', 'container-regs': '0', 'offset': '80'}
00 - 8, {'set': 'General Purpose Registers', 'name': 'ah', 'format': 'hex', 'bitsize': '8', 'encoding': 'uint', 'container-regs': '0', 'offset': '81'}

Could you take a look into this failures or revert CL for now?

I reverted the CL with http://reviews.llvm.org/rL244514 since it was causing multiple test regressions - feel free to re-submit it with fixed Linux tests.

slthakur updated this revision to Diff 31910.Aug 12 2015, 2:25 AM
slthakur edited edge metadata.

Fixed failures reported by Oleksiy

Hi Oleksiy,

Revert r244308 since it's introducing test regressions on Linux:

  • TestLldbGdbServer.py both clang & gcc, i386 and x86_64
  • TestConstVariables.py gcc, i386 and x86_64
  • 112 failures clang, i386
  • TestLldbGdbServer.py both clang & gcc, i386 and x86_64

    This test was failing because Scalar objects type was not set properly when we set the type of RegisterValue. I have added the SetType function in Scalar now which fixes this failure.
  • TestConstVariables.py gcc, i386 and x86_64

    This test was failing only with gcc because gcc produces DW_OP_const2u which causes it to create a Scalar of short type for variable index.
  1684         case DW_OP_const1u             :    stack.push_back(Scalar(( uint8_t)opcodes.GetU8 (&offset))); break;
  1685         case DW_OP_const1s             :    stack.push_back(Scalar((  int8_t)opcodes.GetU8 (&offset))); break;
->1686         case DW_OP_const2u             :    stack.push_back(Scalar((uint16_t)opcodes.GetU16 (&offset))); break;
  1687         case DW_OP_const2s             :    stack.push_back(Scalar(( int16_t)opcodes.GetU16 (&offset))); break;
  1688         case DW_OP_const4u             :    stack.push_back(Scalar((uint32_t)opcodes.GetU32 (&offset))); break;

Because of this the size of ValueObject is less than the size of variable index. I have now updated Scalar class to use int for all 1 byte, 2 byte and 4 byte integers. This fixes TestConstVariables.py with gcc.

  • 112 failures clang, i386

    I couldn’t see any failures with clang, i386. I am using the following command for testing i386 arch : python ./dosep.py -o "-A i386 --executable /home/sagar/LLDB_x86/build/bin/lldb -t -v" Is there anything wrong in the way I am testing ?
  • 112 failures clang, i386

    I couldn’t see any failures with clang, i386. I am using the following command for testing i386 arch : python ./dosep.py -o "-A i386 --executable /home/sagar/LLDB_x86/build/bin/lldb -t -v" Is there anything wrong in the way I am testing ?

I haven't tried it out, but based on your command I guess you are using a 32bit LLDB with 32bit inferiors and the failures Oleksiy mentioned are happening when you run 64bit LLDB with 32bit inferiors (this is the configuration what is tested by the build bot).

emaste edited edge metadata.Aug 12 2015, 8:21 AM

Applied to my tree and testing now.

Is it possible/reasonable to submit this in two parts, the value changes first, and then the MIPS changes?

as an aside git apply complained about whitespace errors, but I think many/all of these already existed prior to your change:

<stdin>:68: trailing whitespace.
        RegisterValue (llvm::APInt inst) : 
<stdin>:161: trailing whitespace.
            m_scalar = llvm::APInt(uint); 
<stdin>:292: trailing whitespace.
        struct 
<stdin>:348: trailing whitespace.
    Scalar(long double v, bool ieee_quad) 
<stdin>:356: trailing whitespace.
    Scalar(llvm::APInt v) : 
warning: squelched 15 whitespace errors
warning: 20 lines add whitespace errors.
include/lldb/Core/RegisterValue.h
83

Example line with reported whitespace issue

Test suite passes on FreeBSD with this change.

I did see a failure in a couple of runs, but these have a long history of very occasional intermittent failures for me, and I can't really make any claim about them with respect to this patch.

Ran 388 test suites (0 failed) (0.000000%)
Ran 288 test cases (0 failed) (0.000000%)
      145.36 real       621.10 user       181.65 sys
Failing Tests (1)
FAIL: LLDB (suite) :: TestMultithreaded.py (FreeBSD feynman 10.1-STABLE FreeBSD 10.1-STABLE #28 r280427+86df2de(stable-10): Thu Mar 26 16:07:47 EDT 2015     emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC amd64 amd64)
Failing Tests (1)
FAIL: LLDB (suite) :: TestEvents.py (FreeBSD feynman 10.1-STABLE FreeBSD 10.1-STABLE #28 r280427+86df2de(stable-10): Thu Mar 26 16:07:47 EDT 2015     emaste@feynman:/tank/emaste/obj/tank/emaste/src/git-stable-10/sys/GENERIC amd64 amd64)

Hi,

@tberghammer: The LLDB I am using is 64-bit. And the tests are getting compiled with the -m32 option with the command I gave, so the inferiors are 32-bit.

@emaste: Thanks for the review. It is reasonable to commit the generic part and the MIPS part separately. I will also clear the white spaces before committing.

I tried out your change with i386 and "Patch #7" have a lot of failure, but "Patch #8" fixes all of them

Hi,

Since all tests are passing with Patch #8 and if everyone is okay with it, may I commit it in two parts:

  1. Scalar and RegisterValue changes.
  2. MIPS MSA register context part.

Hi,

Since all tests are passing with Patch #8 and if everyone is okay with it, may I commit it in two parts:

  1. Scalar and RegisterValue changes.
  2. MIPS MSA register context part.

Yes please go for it.

Committed scalar and register value part in r245216

Committed MIPS MSA register context part in r245217

source/Plugins/Process/Utility/lldb-mips-linux-register-enums.h