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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
