Don't allow ValueObject::Cast from a smaller type to a larger

[jingham]

Summary

SBValue::Cast actually allows casting from a struct type to another struct type. That's a little odd, C-family languages don't allow this, but we have done forever, so I don't want to remove the ability altogether. However, we can't support casting from a small structure to a larger one because in some cases - e.g. all the ConstResult types used both for expression results and for many synthetic children - we don't know where we should fetch the extra bits from. Just zero-filling them seems odd, and error seems like a better response.

This fixes a bug where when casting an expression result from a smaller type to a larger, lldb would present the memory in lldb after the ValueObject's data buffer as the value of the cast type. Again, I could have fixed that bug by expanding the data buffer to match the larger size, but I wouldn't know what to fill it with.

There were two places in the C++ formatters that were using this cast from struct to struct type to change a C++ std typedef (e.g. std::shared_ptr<Foo>::element type *) to a type that is more useful to the user (pointer to the first template argument's type). The cast from struct to struct in that case wasn't necessary, and looked weird since this really isn't an allowed C++ operation. So I also changed those to case the pointer first, then dereference the cast value.

[JDevlieghere]

LGTM

[bulbazord]

I think this patch is probably okay to do, but it does break a supported use case that I'm aware of: One way you can do "object oriented programming" in C is to do exactly what you're trying to prevent. Using the test, you could say that "MyStruct" is the base class and "MyBiggerStruct" would be a derived class. You might have a pointer to a "MyStruct" object, even though you called malloc(sizeof(MyBiggerStruct)), and maybe you can perform that cast if you're sure that you actually have a MyBiggerStruct object.

I know there's not necessarily a good way to support that without also supporting the bug you're trying to fix though. :(

lldb/include/lldb/Core/ValueObject.h

617–620

I'm a little concerned about the API surface here. Maybe this is just my misunderstandings about ValueObject, but...

Given a ValueObjectSP (which may contain a ValueObject, ValueObjectConstResult, etc), which one of these are you supposed to call? If you call Cast, you'll get what you want. If you call DoCast, you might circumvent the safety checks that Cast is providing... but if you have something like a ValueObjectConstResult, this actually calls ValueObject::Cast which does the right thing. Am I understanding this correctly?

I also have a little confusion about which one I would call based on the names... Maybe it would make more sense to call DoCast something like CastImpl?

lldb/source/Core/ValueObject.cpp

2792

What is the purpose of this bool unused?

[jingham]

In D153657#4445292, @bulbazord wrote:

I think this patch is probably okay to do, but it does break a supported use case that I'm aware of: One way you can do "object oriented programming" in C is to do exactly what you're trying to prevent. Using the test, you could say that "MyStruct" is the base class and "MyBiggerStruct" would be a derived class. You might have a pointer to a "MyStruct" object, even though you called malloc(sizeof(MyBiggerStruct)), and maybe you can perform that cast if you're sure that you actually have a MyBiggerStruct object.

I know there's not necessarily a good way to support that without also supporting the bug you're trying to fix though. :(

You can't actually cast structures to structures in C. You can only cast pointers. So in your underlying code you always have to have pointers around if you are playing these games, and you can cast the pointer to the derived type and then dereference it, which is what you would have had to do in the source language anyway.

lldb/include/lldb/Core/ValueObject.h

617–620

We use this naming convention pretty consistently in lldb when there's an overridden method that we need to do some generic preparation for. We use Action for the public call, and DoAction as the overridden one. I could put in some words in the header, but this is a pretty well-established convention: in lldb, you should never call the DoAction version unless you are the Action wrapper.

lldb/source/Core/ValueObject.cpp

2792

That's leftover from a previous overly-complex solution.

[bulbazord]

In D153657#4449608, @jingham wrote:

In D153657#4445292, @bulbazord wrote:

I think this patch is probably okay to do, but it does break a supported use case that I'm aware of: One way you can do "object oriented programming" in C is to do exactly what you're trying to prevent. Using the test, you could say that "MyStruct" is the base class and "MyBiggerStruct" would be a derived class. You might have a pointer to a "MyStruct" object, even though you called malloc(sizeof(MyBiggerStruct)), and maybe you can perform that cast if you're sure that you actually have a MyBiggerStruct object.

I know there's not necessarily a good way to support that without also supporting the bug you're trying to fix though. :(

You can't actually cast structures to structures in C. You can only cast pointers. So in your underlying code you always have to have pointers around if you are playing these games, and you can cast the pointer to the derived type and then dereference it, which is what you would have had to do in the source language anyway.

Right, this makes sense, that was my disconnect. Thanks for clarifying and helping me understand!

lldb/include/lldb/Core/ValueObject.h

public:
 
   virtual lldb::ValueObjectSP AddressOf(Status &error);
 
   virtual lldb::addr_t GetLiveAddress() { return LLDB_INVALID_ADDRESS; }
 
   virtual void SetLiveAddress(lldb::addr_t addr = LLDB_INVALID_ADDRESS,
                               AddressType address_type = eAddressTypeLoad) {}
 
-  virtual lldb::ValueObjectSP Cast(const CompilerType &compiler_type);
+  lldb::ValueObjectSP Cast(const CompilerType &compiler_type);
+
+  virtual lldb::ValueObjectSP DoCast(const CompilerType &compiler_type);
 
   virtual lldb::ValueObjectSP CastPointerType(const char *name,
                                               CompilerType &ast_type);
 
   virtual lldb::ValueObjectSP CastPointerType(const char *name,
                                               lldb::TypeSP &type_sp);
 
   // The backing bits of this value object were updated, clear any descriptive
   // string, so we know we have to refetch them.

lldb/include/lldb/Core/ValueObjectConstResult.h

void SetLiveAddress(lldb::addr_t addr = LLDB_INVALID_ADDRESS,
     m_impl.SetLiveAddress(addr, address_type);
   }
 
   lldb::ValueObjectSP
   GetDynamicValue(lldb::DynamicValueType valueType) override;
 
   lldb::LanguageType GetPreferredDisplayLanguage() override;
 
-  lldb::ValueObjectSP Cast(const CompilerType &compiler_type) override;
+  lldb::ValueObjectSP DoCast(const CompilerType &compiler_type) override;
 
 protected:
   bool UpdateValue() override;
 
   CompilerType GetCompilerTypeImpl() override;
 
   ConstString m_type_name;
   std::optional<uint64_t> m_byte_size;

lldb/include/lldb/Core/ValueObjectConstResultCast.h

lldb::ValueObjectSP GetSyntheticChildAtOffset(
       uint32_t offset, const CompilerType &type, bool can_create,
       ConstString name_const_str = ConstString()) override;
 
   lldb::ValueObjectSP AddressOf(Status &error) override;
 
   size_t GetPointeeData(DataExtractor &data, uint32_t item_idx = 0,
                         uint32_t item_count = 1) override;
 
-  lldb::ValueObjectSP Cast(const CompilerType &compiler_type) override;
+  lldb::ValueObjectSP DoCast(const CompilerType &compiler_type) override;
 
 protected:
   ValueObjectConstResultImpl m_impl;
 
 private:
   friend class ValueObject;
   friend class ValueObjectConstResult;
   friend class ValueObjectConstResultImpl;

lldb/include/lldb/Core/ValueObjectConstResultChild.h

public:
   lldb::ValueObjectSP AddressOf(Status &error) override;
 
   lldb::addr_t GetAddressOf(bool scalar_is_load_address = true,
                             AddressType *address_type = nullptr) override;
 
   size_t GetPointeeData(DataExtractor &data, uint32_t item_idx = 0,
                         uint32_t item_count = 1) override;
 
-  lldb::ValueObjectSP Cast(const CompilerType &compiler_type) override;
+  lldb::ValueObjectSP DoCast(const CompilerType &compiler_type) override;
 
 protected:
   ValueObjectConstResultImpl m_impl;
 
 private:
   friend class ValueObject;
   friend class ValueObjectConstResult;
   friend class ValueObjectConstResultImpl;

lldb/packages/Python/lldbsuite/test/lldbtest.py

):
         return eval_result
 
     """Assert that an lldb.SBError is in the "success" state."""
 
     def assertSuccess(self, obj, msg=None):
         if not obj.Success():
             error = obj.GetCString()
             self.fail(self._formatMessage(msg, "'{}' is not success".format(error)))
+    """Assert that an lldb.SBError is in the "failure" state."""
+
+    def assertFailure(self, obj, error_str = None, msg=None):
+        if obj.Success():
+            self.fail(self._formatMessage(msg, "Error not in a fail state"))
+
+        if error_str == None:
+            return
+                      
+        error = obj.GetCString()
+        self.assertEqual(error, error_str, msg)
 
     """Assert that a command return object is successful"""
 
     def assertCommandReturn(self, obj, msg=None):
         if not obj.Succeeded():
             error = obj.GetError()
             self.fail(self._formatMessage(msg, "'{}' is not success".format(error)))
 

lldb/source/Core/ValueObject.cpp

if (addr != LLDB_INVALID_ADDRESS && address_type != eAddressTypeHost) {
     GetExpressionPath(expr_path_strm);
     error.SetErrorStringWithFormat("'%s' doesn't have a valid address",
                                    expr_path_strm.GetData());
   }
 
   return m_addr_of_valobj_sp;
 }
 
-ValueObjectSP ValueObject::Cast(const CompilerType &compiler_type) {
+ValueObjectSP ValueObject::DoCast(const CompilerType &compiler_type) {
     return ValueObjectCast::Create(*this, GetName(), compiler_type);
 }
 
+ValueObjectSP ValueObject::Cast(const CompilerType &compiler_type) {
+  // Only allow casts if the original type is equal or larger than the cast
+  // type.  We don't know how to fetch more data for all the ConstResult types, 
+  // so we can't guarantee this will work:
+  Status error;
+  CompilerType my_type = GetCompilerType();
+
+  ExecutionContextScope *exe_scope 
+      = ExecutionContext(GetExecutionContextRef())
+          .GetBestExecutionContextScope();
+  if (compiler_type.GetByteSize(exe_scope) 
+      <= GetCompilerType().GetByteSize(exe_scope)) {
+        return DoCast(compiler_type);
+  }
+  error.SetErrorString("Can only cast to a type that is equal to or smaller "
+                       "than the orignal type.");
+
+  return ValueObjectConstResult::Create(
+      ExecutionContext(GetExecutionContextRef()).GetBestExecutionContextScope(),
+                       error);
+}
+
 lldb::ValueObjectSP ValueObject::Clone(ConstString new_name) {
   return ValueObjectCast::Create(*this, new_name, GetCompilerType());
 }
 
 ValueObjectSP ValueObject::CastPointerType(const char *name,
                                            CompilerType &compiler_type) {
   ValueObjectSP valobj_sp;
   AddressType address_type;

lldb/source/Core/ValueObjectConstResult.cpp

if (use_dynamic != eNoDynamicValues) {
     }
     if (m_dynamic_value && m_dynamic_value->GetError().Success())
       return m_dynamic_value->GetSP();
   }
   return ValueObjectSP();
 }
 
 lldb::ValueObjectSP
-ValueObjectConstResult::Cast(const CompilerType &compiler_type) {
+ValueObjectConstResult::DoCast(const CompilerType &compiler_type) {
   return m_impl.Cast(compiler_type);
 }
 
 lldb::LanguageType ValueObjectConstResult::GetPreferredDisplayLanguage() {
   if (m_preferred_display_language != lldb::eLanguageTypeUnknown)
     return m_preferred_display_language;
   return GetCompilerTypeImpl().GetMinimumLanguage();
 }

lldb/source/Core/ValueObjectConstResultCast.cpp

 
 size_t ValueObjectConstResultCast::GetPointeeData(DataExtractor &data,
                                                   uint32_t item_idx,
                                                   uint32_t item_count) {
   return m_impl.GetPointeeData(data, item_idx, item_count);
 }
 
 lldb::ValueObjectSP
-ValueObjectConstResultCast::Cast(const CompilerType &compiler_type) {
+ValueObjectConstResultCast::DoCast(const CompilerType &compiler_type) {
   return m_impl.Cast(compiler_type);
 }

lldb/source/Core/ValueObjectConstResultChild.cpp

 
 size_t ValueObjectConstResultChild::GetPointeeData(DataExtractor &data,
                                                    uint32_t item_idx,
                                                    uint32_t item_count) {
   return m_impl.GetPointeeData(data, item_idx, item_count);
 }
 
 lldb::ValueObjectSP
-ValueObjectConstResultChild::Cast(const CompilerType &compiler_type) {
+ValueObjectConstResultChild::DoCast(const CompilerType &compiler_type) {
   return m_impl.Cast(compiler_type);
 }

lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp

if (!valobj_sp)
     return lldb::ValueObjectSP();
 
   if (idx == 0)
     return valobj_sp->GetChildMemberWithName("__ptr_");
 
   if (idx == 1) {
     if (auto ptr_sp = valobj_sp->GetChildMemberWithName("__ptr_")) {
       Status status;
-      auto value_sp = ptr_sp->Dereference(status);
-      if (status.Success()) {
-        auto value_type_sp =
-            valobj_sp->GetCompilerType().GetTypeTemplateArgument(0);
-        return value_sp->Cast(value_type_sp);
+            valobj_sp->GetCompilerType()
+              .GetTypeTemplateArgument(0).GetPointerType();
+      ValueObjectSP cast_ptr_sp = ptr_sp->Cast(value_type_sp);
+      ValueObjectSP value_sp = cast_ptr_sp->Dereference(status);
+      if (status.Success()) {
+        return value_sp;
       }
     }
   }
 
   return lldb::ValueObjectSP();
 }
 
 bool lldb_private::formatters::LibcxxSharedPtrSyntheticFrontEnd::Update() {

lldb/source/Plugins/Language/CPlusPlus/LibCxxUnorderedMap.cpp

if (!hash_sp || !value_sp) {
               0, name, nullptr, nullptr, nullptr);
           CompilerType actual_type = field_type.GetTypedefedType();
           if (isStdTemplate(actual_type.GetTypeName(), "pair"))
             m_element_type = actual_type;
         }
       }
       if (!m_node_type)
         return nullptr;
-      node_sp = node_sp->Cast(m_node_type);
+      node_sp = m_next_element->Cast(m_node_type.GetPointerType())
+              ->Dereference(error);
+      if (!node_sp || error.Fail())
+          return nullptr;
+
       value_sp = node_sp->GetChildMemberWithName("__value_");
       hash_sp = node_sp->GetChildMemberWithName("__hash_");
       if (!value_sp || !hash_sp)
         return nullptr;
     }
     m_elements_cache.push_back(
         {value_sp.get(), hash_sp->GetValueAsUnsigned(0)});
     m_next_element = node_sp->GetChildMemberWithName("__next_").get();

lldb/test/API/python_api/value/TestValueAPI.py

def test(self):
         val_s = target.EvaluateExpression("s")
         val_a = target.EvaluateExpression("a")
         self.assertTrue(
             val_s.GetChildMemberWithName("a").GetAddress().IsValid(), VALID_VARIABLE
         )
         self.assertTrue(val_s.GetChildMemberWithName("a").AddressOf(), VALID_VARIABLE)
         self.assertTrue(val_a.Cast(val_i.GetType()).AddressOf(), VALID_VARIABLE)
 
+        # Test some other cases of the Cast API.  We allow casts from one struct type
+        # to another, which is a little weird, but we don't support casting from a
+        # smaller type to a larger as we often wouldn't know how to get the extra data:
+        val_f = target.EvaluateExpression("f")
+        bad_cast = val_s.Cast(val_f.GetType())
+        self.assertFailure(bad_cast.GetError(),
+                           "Can only cast to a type that is equal to or smaller than the orignal type.")
+        weird_cast = val_f.Cast(val_s.GetType())
+        self.assertSuccess(weird_cast.GetError(),
+                        "Can cast from a larger to a smaller")
+        self.assertEqual(weird_cast.GetChildMemberWithName("a").GetValueAsSigned(0), 33,
+                         "Got the right value")
+
         # Check that lldb.value implements truth testing.
         self.assertFalse(lldb.value(frame0.FindVariable("bogus")))
         self.assertTrue(lldb.value(frame0.FindVariable("uinthex")))
 
         self.assertEqual(
             int(lldb.value(frame0.FindVariable("uinthex"))),
             3768803088,
             "uinthex == 3768803088",

lldb/test/API/python_api/value/main.c

 typedef int MyInt;
 
 struct MyStruct
 {
   int a;
   int b;
 };
 
+struct MyBiggerStruct
+{
+  int a;
+  int b;
+  int c;
+};
+
 int main (int argc, char const *argv[])
 {
     uint32_t uinthex = 0xE0A35F10;
     int32_t  sinthex = 0xE0A35F10;
 
     int i;
     MyInt a = 12345;
     struct MyStruct s = { 11, 22 };
+    struct MyBiggerStruct f = { 33, 44, 55 }; 
     int *my_int_ptr = &g_my_int;
     printf("my_int_ptr points to location %p\n", my_int_ptr);
     const char **str_ptr = days_of_week;
     for (i = 0; i < 7; ++i)
         printf("%s\n", str_ptr[i]); // Break at this line
                                     // and do str_ptr_val.GetChildAtIndex(5, lldb.eNoDynamicValues, True).
     
     return 0;
 }