[ValueObject] Do not return address of eValueTypeHostAddress values.

[sivachandra]

Summary

This fixes TestRegisterVariables for clang and hence it is enabled in this commit.

[sivachandra]

This change is not really neat. My intention is to start a discussion by putting out a change which "works" (by fixing TestRegisterVariables).

The issue with that test is that clang puts out the DWARF for a frame var as follows:

< 2><0x00000040>      DW_TAG_formal_parameter
                      DW_AT_location              <loclist with 1 entries follows>
                      [ 0]<lowpc=0x00000000><highpc=0x00000002>DW_OP_reg5 DW_OP_piece 4
                      DW_AT_name                  "a"
                      ...

AIUI, since pieces are accumulated in a data buffer, the resulting value is not treated as a scalar but to be of eValueTypeHostAddress type. The effective change suggested here is to build a scalar value out of the eValueTypeHostAddress pieces instead of using the raw pieces.

[clayborg]

This is an incorrect fix. The variable doesn't necessarily need to be a scalar. You might be describing where a uint128 or larger value is constructed from multiple registers. In your case this works, but this isn't the right fix. We should be able to describe a variable's value by saying "here is the buffer that contains it". This is what the pieces variable contains: all the bytes needed for the variable. In this case we have a "host" buffer of bytes and the value lldb_private::Value should say that its address is a host address and the bytes are contained in the value itself. Whatever is failing to read the value correctly out of this host buffer is what actually needs to be fixed, not this code.

[sivachandra]

On Thu, Apr 30, 2015 at 3:41 PM, Greg Clayton <[e-mail]> wrote:

This is an incorrect fix. The variable doesn't necessarily need to be a scalar.
You might be describing where a uint128 or larger value is constructed from
multiple registers. In your case this works, but this isn't the right fix. We
should be able to describe a variable's value by saying "here is the buffer that
contains it".

This wording helps understand the code and its meaning better and I now agree that the change is in the wrong place.

I would like to use this as an opportunity to ask about few other pieces of code which don't seem to be in line [per my current understanding that is] with the above meaning.

Look at my comments inline with in code below from ValueObject.cpp:

1804 addr_t
1805 ValueObject::GetAddressOf (bool scalar_is_load_address, AddressType *address_type)
1806 {
1807     if (!UpdateValueIfNeeded(false))
1808         return LLDB_INVALID_ADDRESS;
1809
1810     switch (m_value.GetValueType())
1811     {
1812     case Value::eValueTypeScalar:
1813     case Value::eValueTypeVector:
1814         if (scalar_is_load_address)
1815         {
1816             if(address_type)
1817                 *address_type = eAddressTypeLoad;
1818             return m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
1819         }
1820         break;
1821
1822     case Value::eValueTypeLoadAddress:
1823     case Value::eValueTypeFileAddress:
1824     case Value::eValueTypeHostAddress:

    Can a value of type eValueTypeHostAddress have a valid address in the inferior process space? Even if it does, why is a scalar value being returned on line 1828.

1825         {
1826             if(address_type)
1827                 *address_type = m_value.GetValueAddressType ();
1828             return m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
1829         }
1830         break;
1831     }
1832     if (address_type)
1833         *address_type = eAddressTypeInvalid;
1834     return LLDB_INVALID_ADDRESS;
1835 }

Another one from ValueObject.cpp:

3802         case eAddressTypeHost:
3803             {
3804                 ClangASTType clang_type = GetClangType();
3805                 if (clang_type)
3806                 {
3807                     std::string name (1, '&');
3808                     name.append (m_name.AsCString(""));
3809                     ExecutionContext exe_ctx (GetExecutionContextRef());
3810                     m_addr_of_valobj_sp = ValueObjectConstResult::Create (exe_ctx.GetBestExecutionContextScope(),
3811                                                                           clang_type.GetPointerType(),
3812                                                                           ConstString (name.c_str()),
3813                                                                           addr,

    Why is the host address being treated as the value's address when the address type is eAddressTypeHost.

3814                                                                           eAddressTypeInvalid,
3815                                                                           m_data.GetAddressByteSize());
3816                 }

[sivachandra]

My problem is solved if I make ValueObject::AddressOf and
ValueObject::GetAddressOf return LLDB_INVALID_ADDRESS for values of
type eValueTypeHostAddress. I am not sure Greg was actually implying
the same while answering my questions, but I am putting this out so
that he can suggest other options if the fix is not at the right
place again. I could not locate a better place anyway.

I have run the test suite and do not see any regressions.

[clayborg]

Much better and safer. I would prefer to not allow taking the address of a host scalar pointer and pass out the address of that scalar object, so this should be safe and what we want.

source/Core/ValueObject.cpp

case Value::eValueTypeVector:
             if(address_type)
                 *address_type = eAddressTypeLoad;
             return m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
         }
         break;
 
     case Value::eValueTypeLoadAddress: 
     case Value::eValueTypeFileAddress:
-    case Value::eValueTypeHostAddress:
         {
             if(address_type)
                 *address_type = m_value.GetValueAddressType ();
             return m_value.GetScalar().ULongLong(LLDB_INVALID_ADDRESS);
         }
         break;
+    case Value::eValueTypeHostAddress:
+        break;
     }
     if (address_type)
         *address_type = eAddressTypeInvalid;
     return LLDB_INVALID_ADDRESS;
 }
 
 addr_t
 ValueObject::GetPointerValue (AddressType *address_type)

 {
     if (m_addr_of_valobj_sp)
         return m_addr_of_valobj_sp;
         
     AddressType address_type = eAddressTypeInvalid;
     const bool scalar_is_load_address = false;
     addr_t addr = GetAddressOf (scalar_is_load_address, &address_type);
     error.Clear();
-    if (addr != LLDB_INVALID_ADDRESS)
+    if (addr != LLDB_INVALID_ADDRESS && address_type != eAddressTypeHost)
     {
         switch (address_type)
         {
         case eAddressTypeInvalid:
             {
                 StreamString expr_path_strm;
                 GetExpressionPath(expr_path_strm, true);
                 error.SetErrorStringWithFormat("'%s' is not in memory", expr_path_strm.GetString().c_str());
             }
             break;
 
         case eAddressTypeFile:
         case eAddressTypeLoad:
-        case eAddressTypeHost:
             {
                 ClangASTType clang_type = GetClangType();
                 if (clang_type)
                 {
                     std::string name (1, '&');
                     name.append (m_name.AsCString(""));
                     ExecutionContext exe_ctx (GetExecutionContextRef());
                     m_addr_of_valobj_sp = ValueObjectConstResult::Create (exe_ctx.GetBestExecutionContextScope(),
                                                                           clang_type.GetPointerType(),
                                                                           ConstString (name.c_str()),
                                                                           addr, 
                                                                           eAddressTypeInvalid,
                                                                           m_data.GetAddressByteSize());
                 }
             }
             break;
+        default:
+            break;
         }
     }
     else
     {
         StreamString expr_path_strm;
         GetExpressionPath(expr_path_strm, true);
         error.SetErrorStringWithFormat("'%s' doesn't have a valid address", expr_path_strm.GetString().c_str());
     }

test/lang/c/register_variables/TestRegisterVariables.py

class RegisterVariableTestCase(TestBase):
 
     @dsym_test
     @expectedFailureDarwin("llvm.org/pr20266")
     def test_with_dsym_and_run_command(self):
         """Test expressions on register values."""
         self.buildDsym()
         self.const_variable()
 
-    @expectedFailureClang
     @dwarf_test
     @expectedFailureGcc #xfail to get buildbot green, test failed with gcc4.8.2
     def test_with_dwarf_and_run_command(self):
         """Test expressions on register values."""
         self.buildDwarf()
         self.const_variable()
 
     def setUp(self):