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

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D153657#4445292 <https://reviews.llvm.org/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.



================
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);
----------------
bulbazord wrote:
> 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`? 
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.


================
Comment at: lldb/source/Core/ValueObject.cpp:2792
+  CompilerType my_type = GetCompilerType();
+  bool unused;
+
----------------
bulbazord wrote:
> What is the purpose of this `bool unused`?
That's leftover from a previous overly-complex solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153657