[Lldb-commits] [PATCH] D153657: Don't allow ValueObject::Cast from a smaller type to a larger

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

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. :(



================
Comment at: lldb/include/lldb/Core/ValueObject.h:617-619
+  lldb::ValueObjectSP Cast(const CompilerType &compiler_type);
+
+  virtual lldb::ValueObjectSP DoCast(const CompilerType &compiler_type);
----------------
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`? 


================
Comment at: lldb/source/Core/ValueObject.cpp:2792
+  CompilerType my_type = GetCompilerType();
+  bool unused;
+
----------------
What is the purpose of this `bool unused`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153657/new/

https://reviews.llvm.org/D153657