jankratochvil added inline comments.
Comment at:
lldb/trunk/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3476
+ if (DWARFDIE var_die = die.GetReferencedDIE(DW_AT_count)) {
+if (var_die.Tag() == DW_TAG_variable)
+ if
jankratochvil added subscribers: dblaikie, jankratochvil.
jankratochvil added a comment.
There is a regression in trunk clang+lldb. Using either 7.0 clang or 7.0 lldb
makes the testcases PASS but both trunk clang and trunk lldb makes the testcase
FAIL.
This commit (rL346165
For dotest style tests, the debug format to test is chosen by the test driver.
All the tests should run with any format, but sometimes there are bugs in one
reader or another (or in one version of DWARF or another) so you can skip or
xfail a test based on format. Sounds like this test should
stella.stamenova added a comment.
In https://reviews.llvm.org/D53530#1290680, @aprantl wrote:
> In https://reviews.llvm.org/D53530#1290664, @stella.stamenova wrote:
>
> > TestVla fails on Windows:
> > http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1129/steps/test/logs/stdio
> >
aprantl added a comment.
In https://reviews.llvm.org/D53530#1290664, @stella.stamenova wrote:
> TestVla fails on Windows:
> http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1129/steps/test/logs/stdio
>
> AssertionError: False is not True : 'frame var vla' returns expected
>
stella.stamenova added a comment.
TestVla fails on Windows:
http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1129/steps/test/logs/stdio
AssertionError: False is not True : 'frame var vla' returns expected result,
got '(int []) vla = {}'
I will have time to look in more detail
aprantl added a comment.
Thanks for your patience!
Repository:
rL LLVM
https://reviews.llvm.org/D53530
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL346165: Fix (and improve) the support for C99 variable
length array types (authored by adrian, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.
Thanks for making the changes!
https://reviews.llvm.org/D53530
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
aprantl updated this revision to Diff 172390.
aprantl added a comment.
Fix a bug in the testcase.
https://reviews.llvm.org/D53530
Files:
include/lldb/Symbol/ClangASTContext.h
include/lldb/Symbol/CompilerType.h
include/lldb/Symbol/GoASTContext.h
include/lldb/Symbol/JavaASTContext.h
aprantl added a comment.
Fair point. So here's a version that only overrides GetNumChildren(). I'll
leave the type summary for a follow-up patch.
https://reviews.llvm.org/D53530
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
aprantl updated this revision to Diff 172388.
aprantl added a comment.
Herald added subscribers: kbarton, nemanjai.
Version that only overrides GetNumChildren to avoid creating dynamic clang
types.
https://reviews.llvm.org/D53530
Files:
include/lldb/Symbol/ClangASTContext.h
clayborg added a comment.
In https://reviews.llvm.org/D53530#1284267, @aprantl wrote:
> > I didn't realize that the string int [] is produced by ValueObject itself;
> > I think this makes this option more palatable to me. I'll give it a try.
>
> So, it turns out it isn't. The only way to get
aprantl added a comment.
> I didn't realize that the string int [] is produced by ValueObject itself; I
> think this makes this option more palatable to me. I'll give it a try.
So, it turns out it isn't. The only way to get the length into the typename is
to hack ClangASTContext::GetTypeName
aprantl added a comment.
To me a VLA fulfills all properties of a dynamic type so not modeling it as a
dynamic type feels backwards to me. But not having to deal with temporary clang
types might be worth the trade-off.
> The only other thing you would need to change to get the usability back
clayborg added a comment.
The only other thing you would need to change to get the usability back in
check when doing things in GetNumChildren() would be to have the function that
gets the typename take on optional execution context for dynamic types. The
ValueObject can easily pass its
aprantl added a comment.
I now spent (way too much :-) time experimenting with alternative
implementations of this patch. In https://reviews.llvm.org/D53961 there is a
version that changes GetNumChildren() to take an execution context. I don't
think that doing it this way is a good solution
davide added inline comments.
Comment at:
source/Plugins/LanguageRuntime/CPlusPlus/ItaniumABI/ItaniumABILanguageRuntime.cpp:213-229
+ // Handle variables with incomplete array types.
+ auto *type = in_value.GetCompilerType().GetOpaqueQualType();
+ auto qual_type =
clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.
Parsing a type shouldn't need an execution context and we shouldn't be
re-parsing a type over and over for each frame. We should be encoding the array
expression somewhere that
aprantl updated this revision to Diff 170526.
aprantl added a comment.
Factor out caching of types as suggested. Thanks!
https://reviews.llvm.org/D53530
Files:
include/lldb/Symbol/SymbolFile.h
include/lldb/Symbol/Variable.h
packages/Python/lldbsuite/test/lang/c/vla/Makefile
jingham added a comment.
This seems good to me, but Greg is more expert than I so I'd wait on his okay.
Comment at: source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1901-1907
+dwarf->GetTypeList()->Insert(type_sp);
+// Cache the type if it isn't
aprantl created this revision.
aprantl added reviewers: jingham, clayborg.
aprantl added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Clang recently improved its DWARF support for C VLA types. The DWARF now looks
like this:
0x0051: DW_TAG_variable [4]
22 matches
Mail list logo
