[Lldb-commits] [PATCH] D86436: [lldb] Fix Type::GetByteSize for pointer types

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

In D86436#2234110 <https://reviews.llvm.org/D86436#2234110>, @davide wrote:

> @labath something I noticed when finding this (and related bugs) is that `frame var` carries a decent diagnostic
>
>   (int *) l_125 = <empty constant data>
>
> and the expression parser returns something not particularly useful:
>
>   (lldb) p l_125 
>   error: <lldb wrapper prefix>:43:31: no member named 'l_125' in namespace '$__lldb_local_vars'
>       using $__lldb_local_vars::l_125;
>             ~~~~~~~~~~~~~~~~~~~~^
>   error: <user expression 0>:1:1: use of undeclared identifier 'l_125'
>   l_125
>
> From my testing infrastructure/fuzzing perspective the two are indistinguishable, as the script I've written chokes on both, but it would be better from an ergonomics point of view if `p` would return something meaningful, if possible (even if there's a bug in lldb). Do you think it's worth filing a PR? (also, cc: @teemperor for ideas as he spent a fair amount of time working on the expression parser)

For this particular case, I think the best diagnostic would be one of those module-level warnings we print to stderr, as I think that a DW_AT_const_value attribute with no data means the debug info is broken.

Speaking more generally, it may be interesting to try to present variables with no value (e.g. optimized out) to clang as undefined (`extern`) variables. That way we could get past the "compile" stage, and handle this stuff in the "link" stage. Here, I believe we have more control so it may be easier to print something like "Cannot evaluate expression because variable 'foo' is optimized out. But I am not really an expert here so I am just guessing...
I'm not really an expert here, and we are sort of limited by what error messages we can get clang to produce. The current error message is not completely unreasonable because it basically says "this variable does not exist"



================
Comment at: lldb/source/Symbol/Type.cpp:378
         m_byte_size_has_value = true;
+        return m_byte_size;
       }
----------------
davide wrote:
> shafik wrote:
> > labath wrote:
> > > davide wrote:
> > > > shafik wrote:
> > > > > Wouldn't it be better to turn `m_byte_size` into an `Optional`? As this fix shows this having this additional state we carry around is error prone.
> > > > This is a much larger project, probably worth considering.
> > > > With that in mind, it wouldn't have prevented this bug from happening, as this falls through and returns an `Optional<T>` anyway. 
> > > > Yay fuzz testing (that found this), I would say.
> > > IIRC, this was done this way to reduce sizeof(Type). Note that the external interface is Optional<uint64_t> and the only place that knows (should know?) about this encoding is this function.
> > It would have prevented this bug as you just return `m_byte_size` no matter what.
> If you think it's an improvement, please consider submitting a review, I would be eager to look at it!
FWIW, there is a way to make this more robust without the memory size increase due to llvm::Optional:
```
if (!m_byte_size_has_value) {
  if (Optional<uint64_t> byte_size = ComputeByteSize()) {
    m_byte_size_has_value = true;
    m_byte_size = *byte_size;
  }
}
if (m_byte_size_has_value)
  return m_byte_size;
return None;
```
That way the optional-to-flat conversion is fully contained in 9 lines of code, which can be easily audited for correctness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86436