Page MenuHomePhabricator

shafik (Shafik Yaghmour)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 12 2018, 2:31 PM (79 w, 1 d)

Recent Activity

Yesterday

shafik created D72953: Fix the handling of unnamed bit-fields when parsing DWARF.
Fri, Jan 17, 1:17 PM
shafik accepted D72694: [lldb] Mark the implicit copy constructor as deleted when a move constructor is provided..

LGTM

Fri, Jan 17, 10:51 AM · Restricted Project

Mon, Jan 13

shafik added a reviewer for D72597: [lldb][DWARF] Added support for new forms in DWARFv5 macro.: shafik.
Mon, Jan 13, 12:56 PM · Restricted Project

Fri, Jan 10

shafik accepted D72495: [lldb] Make CompleteTagDeclsScope completion order deterministic.

This is a good change, we do see bugs that are sometimes not totally deterministic so this should hopefully reduce those.

Fri, Jan 10, 10:59 AM · Restricted Project
shafik accepted D72507: [lldb] Remove FieldDecl stealing hack by rerouting indirect imports to the original AST.

LGTM

Fri, Jan 10, 10:03 AM · Restricted Project

Thu, Jan 9

shafik added a comment to rG61bd19206f61: [libc++] Explicitly enumerate std::string external instantiations..

Eric, this is breaking the green dragon build bots for LLDB, see the first log here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/5581/

Thu, Jan 9, 5:25 PM
shafik added a comment to D72391: [lldb] Add a display name to ClangASTContext instances.

I don't know if this needs a unit test where we call the constructor and explicitly check the name is the one we passed in. Let me know if you think this would make sense.

Thu, Jan 9, 10:52 AM · Restricted Project
shafik added a comment to D72391: [lldb] Add a display name to ClangASTContext instances.

@clayborg We can easily append the ptr value to the display name. All names should always be unique as long as there is one target, but in the off-chance that one isn't unique it might be useful.

Thu, Jan 9, 10:52 AM · Restricted Project
shafik accepted D72391: [lldb] Add a display name to ClangASTContext instances.
Thu, Jan 9, 10:52 AM · Restricted Project
shafik added a comment to D72391: [lldb] Add a display name to ClangASTContext instances.

Perhaps it makes sense to modify the dump() method to also display the new name?

Thu, Jan 9, 10:42 AM · Restricted Project
shafik accepted D72359: [lldb] Fix TestClangASTContext.TestFunctionTemplateInRecordConstruction in Debug builds.

LGTM

Thu, Jan 9, 10:23 AM · Restricted Project
shafik added a comment to D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions.

This looks fine to me. @shafik, @teemperor, do you have any more comments?

Thu, Jan 9, 9:17 AM · Restricted Project

Wed, Jan 8

shafik added a reviewer for D72413: Add missing nullptr checks.: shafik.

I am curious what prompted this set of changes, they look sensible but I don't see how we can do anything useful if we end up in this state.

Wed, Jan 8, 4:33 PM · Restricted Project

Fri, Jan 3

shafik created D72190: Removing C-style casts in favor of explict C++ style casts.
Fri, Jan 3, 4:47 PM
shafik added inline comments to D72161: [lldb][NFC] Use static_cast instead of reinterpret_cast where possible.
Fri, Jan 3, 11:21 AM · Restricted Project
shafik added a comment to D72161: [lldb][NFC] Use static_cast instead of reinterpret_cast where possible.

This is a great change! Moving to stricter casts clarifies intent and reduces the chance that later changes can introduce bugs accidentally.

Fri, Jan 3, 11:21 AM · Restricted Project
shafik added inline comments to D71909: [lldb] Fix crash in AccessDeclContextSanity when copying FunctionTemplateDecl inside a record..
Fri, Jan 3, 10:53 AM · Restricted Project

Thu, Jan 2

shafik added inline comments to D71909: [lldb] Fix crash in AccessDeclContextSanity when copying FunctionTemplateDecl inside a record..
Thu, Jan 2, 1:40 PM · Restricted Project
shafik added inline comments to D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end..
Thu, Jan 2, 1:08 PM · Restricted Project

Thu, Dec 19

shafik committed rG6a7df3a3f940: [ASTImporter][LLDB] Modifying ImportDeclContext(...) to ensure that we complete… (authored by shafik).
[ASTImporter][LLDB] Modifying ImportDeclContext(...) to ensure that we complete…
Thu, Dec 19, 11:23 AM
shafik closed D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton.
Thu, Dec 19, 11:22 AM · Restricted Project, Restricted Project
shafik added inline comments to D71630: [lldb] Add a SubsystemRAII that takes care of calling Initialize and Terminate in the unit tests.
Thu, Dec 19, 9:53 AM · Restricted Project

Dec 18 2019

shafik updated the diff for D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton.
  • Fix typo
  • Adjust error handing
  • clang-format test
Dec 18 2019, 2:10 PM · Restricted Project, Restricted Project

Dec 12 2019

shafik added a comment to D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton.

I wonder if we have a way to fix this from with LLDB. Having Clang code that is only tested in LLDB is always a bit weird.

Otherwise the idea itself LGTM, thanks for working on this (and reducing the test case to that!)

Dec 12 2019, 10:23 AM · Restricted Project, Restricted Project

Dec 11 2019

shafik created D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton.
Dec 11 2019, 3:51 PM · Restricted Project, Restricted Project
shafik accepted D71336: [lldb] Remove ClangASTMetrics.

It is not clear to me we could use these in a test because I don't see a reason for the numbers to stay stable and therefore a useful measure.

Dec 11 2019, 9:17 AM · Restricted Project
shafik added a comment to D71306: [RFC] Change how we deal with optional dependencies.

Besides lldb developers, another category we should consider are llvm (or clang or lld, ...) developers who know nothing about lldb, but they ended up building it because they specified LLVM_ENABLE_PROJECTS=all. These people probably don't want to figure out how to install libedit, or how to disable it, but if the default config "just works" for them, they'd be happy to build lldb and make sure their patch doesn't break it. Optimizing for this experience may be even more important than core lldb developers, since there's fewer of the latter and they usually just configure lldb once.

Dec 11 2019, 9:05 AM · Restricted Project

Dec 10 2019

shafik accepted D70819: [ASTImporter] Support functions with placeholder return types ....

Besides my comments about different scenarios this LGTM

Dec 10 2019, 11:04 AM · Restricted Project
shafik added a comment to D70819: [ASTImporter] Support functions with placeholder return types ....

Apologies for wacky C++ code that follows but will this also work for the following cases:

Dec 10 2019, 10:09 AM · Restricted Project

Dec 9 2019

shafik accepted D71196: [lldb] Add support for calling objc_direct methods from LLDB's expression evaluator..

LGTM

Dec 9 2019, 2:08 PM · Restricted Project

Dec 5 2019

shafik added a comment to D70524: Support DebugInfo generation for auto return type for C++ functions..

@shafik Can you speak about whether this feature ("auto" return type in DWARF v5 section 5.2) would be useful for LLDB?

Dec 5 2019, 11:58 AM · debug-info, Restricted Project, Restricted Project
shafik committed rGfffd70291e12: [LLDB] Replacing use of ul suffix in GetMaxU64Bitfield since it not guarenteed… (authored by shafik).
[LLDB] Replacing use of ul suffix in GetMaxU64Bitfield since it not guarenteed…
Dec 5 2019, 10:05 AM
shafik closed D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit.
Dec 5 2019, 10:05 AM · Restricted Project

Dec 4 2019

shafik accepted D60499: [ASTImporter] Various source location and range import fixes..

LGTM

Dec 4 2019, 5:10 PM · Restricted Project
shafik added inline comments to D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit.
Dec 4 2019, 3:08 PM · Restricted Project
shafik updated the diff for D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit.

Changing -static_cast<uint64_t>(1) to std::numeric_limits<uint64_t>::max()

Dec 4 2019, 3:08 PM · Restricted Project
shafik added a comment to D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions.

I guess you were building darwin binaries, right? If that's the case, then you weren't using this code at all, as apple uses AppleIndex by default. The test in question uses all three indexes (it forces their generation by altering compile flags) and the failures you were seeing were most likely coming the "manual" case...

Dec 4 2019, 2:00 PM · Restricted Project
shafik accepted D70721: [lldb/cpluspluslanguage] Add constructor substitutor.
Dec 4 2019, 9:32 AM · Restricted Project

Dec 3 2019

shafik created D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit.
Dec 3 2019, 5:05 PM · Restricted Project
shafik added a comment to D70663: [lldb] Remove lldb's own ASTDumper.

Yeah, there was indeed some ugly code there, nice work!

Dec 3 2019, 4:46 PM · Restricted Project
shafik added inline comments to D70721: [lldb/cpluspluslanguage] Add constructor substitutor.
Dec 3 2019, 11:01 AM · Restricted Project
shafik added a comment to D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions.

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

So with this change for the find-basic-function.cpp test I no longer see any results for the full case so we should at least generate a case that has results for the full case.

There's an additional check in that test which does a search by a mangled name (search for FULL-MANGLED), and this one does return some result. If this patch lands, I'm not sure if there's any other kind of a "full" lookup that we could perform. eFunctionNameTypeFull is documented as: ... For C this is the same as just the name of the function For C++ this is the mangled or demangled version of the mangled name..., which appears like we should support searching by *de*mangled names. However, I'm not sure if that is actually a good idea. Implementing that for the manual index would be simple enough, but that is something that the apple index could never support (in fact, I think I remember that the manual index once supported searching by demangled names, but then I removed this ability for consistency when adding debug_names support).

That said, I think it may be interesting to add a test searching for an extern "C" symbol (which has no "mangled" name), as right now it's not clear if it will show up because of function_fullnames.Find or function_basenames.Find...

Dec 3 2019, 9:56 AM · Restricted Project

Dec 2 2019

shafik added a comment to D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions.

There's lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp, which probably didn't get run for you as you didn't have lld enabled (cmake -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to update that test to match the new behavior, but other than that, I think this is a good change.

Dec 2 2019, 4:21 PM · Restricted Project
shafik updated subscribers of D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions.

This LGTM, but the TODO directly above the change in ClangExpressionDeclMap.cpp worries me a bit. I am not sure how good our test coverage is for calling functions when there are instance methods in scope (e.g., we are in instance method and try to call another instance method).

Dec 2 2019, 1:39 PM · Restricted Project

Nov 19 2019

shafik added inline comments to D70448: [LLDB] Fix wrong argument in CommandObjectThreadStepWithTypeAndScope.
Nov 19 2019, 9:29 AM · Restricted Project
shafik added inline comments to D70449: Add a "Using LLDB" section to the welcome page of the website.
Nov 19 2019, 9:20 AM · Restricted Project
shafik added inline comments to D70449: Add a "Using LLDB" section to the welcome page of the website.
Nov 19 2019, 9:20 AM · Restricted Project

Nov 12 2019

shafik committed rG91e94a7015f1: [LLDB][Formatters] Re-enable std::function formatter with fixes to improve non… (authored by shafik).
[LLDB][Formatters] Re-enable std::function formatter with fixes to improve non…
Nov 12 2019, 11:33 AM
shafik closed D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.
Nov 12 2019, 11:33 AM · Restricted Project
shafik updated the diff for D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.

Applying clang-format

Nov 12 2019, 11:05 AM · Restricted Project
shafik added inline comments to D69704: [lldb] Add IsDebugInfoCompatible method to SBModule to allow checking compatibility between language versions.
Nov 12 2019, 10:37 AM · Restricted Project, Restricted Project

Nov 11 2019

shafik updated the diff for D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.
  • Adjust comment
  • matching_lambda argument is now FunctionSP to match ForeachFunction signature.
Nov 11 2019, 3:14 PM · Restricted Project
shafik added a comment to D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.

Why not have the FindFunctions lambda take a FunctionSP? It would be easy to get the Function name out of the function in the lambda, and that would make the function more general (and also match what the Foreach does...

Nov 11 2019, 3:14 PM · Restricted Project

Nov 8 2019

shafik added a comment to D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.

@aprantl you will want to take a second look at this, I did some major refactoring.

Nov 8 2019, 3:38 PM · Restricted Project
shafik updated the diff for D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.

After updating branch the stepping test started failing. It was failing because the __invoke case was subtly broken in this patch. Fixing that case opened up a lot of refactoring opportunities. So this patch ends up being a large refactor and in many ways a simplification now.

Nov 8 2019, 3:38 PM · Restricted Project

Nov 6 2019

shafik committed rGe18f4db208ba: [LLDB] Adding caching to libc++ std::function formatter for lookups that… (authored by shafik).
[LLDB] Adding caching to libc++ std::function formatter for lookups that…
Nov 6 2019, 4:06 PM
shafik closed D67111: Adding caching to libc++ std::function formatter for lookups that require scanning symbols.
Nov 6 2019, 4:06 PM · Restricted Project
shafik committed rG83393d27af66: [LLDB] Fix handling for the clang name mangling extension for block invocations (authored by shafik).
[LLDB] Fix handling for the clang name mangling extension for block invocations
Nov 6 2019, 2:21 PM
shafik closed D69738: Fix handling for the clang name mangling extension for block invocations.
Nov 6 2019, 2:21 PM · Restricted Project
shafik edited reviewers for D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance, added: aprantl; removed: aam.
Nov 6 2019, 11:04 AM · Restricted Project
shafik added a child revision for D67111: Adding caching to libc++ std::function formatter for lookups that require scanning symbols: D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.
Nov 6 2019, 10:39 AM · Restricted Project
shafik created D69913: Re-enable std::function formatter with fixes to improve non-cached lookup performance.
Nov 6 2019, 10:39 AM · Restricted Project

Nov 5 2019

shafik added inline comments to D69738: Fix handling for the clang name mangling extension for block invocations.
Nov 5 2019, 10:01 AM · Restricted Project

Nov 4 2019

shafik updated the diff for D69738: Fix handling for the clang name mangling extension for block invocations.

Updating based on comments

  • Adding documentation
  • using startswith(...)
  • Updating initialization to use = instead of {}
Nov 4 2019, 2:53 PM · Restricted Project
shafik added a comment to D69820: [Symbol] Add TypeSystem::GetClassName.

As far as I can tell we don't have coverage for GetBaseClassPath it would be good to add it while we are refactoring this code.

Nov 4 2019, 1:49 PM · Restricted Project

Nov 1 2019

shafik created D69738: Fix handling for the clang name mangling extension for block invocations.
Nov 1 2019, 2:04 PM · Restricted Project

Oct 29 2019

shafik committed rGe6581783f767: [LLDB] Fix for windows bots broken by unsupported tests (authored by shafik).
[LLDB] Fix for windows bots broken by unsupported tests
Oct 29 2019, 11:41 AM
shafik accepted D69566: [ASTImporter] Add support for BuiltinTemplateDecl.
Oct 29 2019, 10:07 AM · Restricted Project

Oct 28 2019

shafik committed rGde2c7cab715e: Add support for DW_AT_export_symbols for anonymous structs (authored by shafik).
Add support for DW_AT_export_symbols for anonymous structs
Oct 28 2019, 3:28 PM
shafik closed D68961: Add support for DW_AT_export_symbols for anonymous structs .
Oct 28 2019, 3:28 PM · Restricted Project

Oct 25 2019

shafik updated the diff for D68961: Add support for DW_AT_export_symbols for anonymous structs .

Using llvm-dwarfdump instead of dwarfdump

Oct 25 2019, 9:57 AM · Restricted Project

Oct 24 2019

shafik updated the diff for D68961: Add support for DW_AT_export_symbols for anonymous structs .
  • Updated test to add dwarfdump test
Oct 24 2019, 5:46 PM · Restricted Project
shafik updated the diff for D68961: Add support for DW_AT_export_symbols for anonymous structs .

-Add test to clarify what we depending wrt to debug info

Oct 24 2019, 4:00 PM · Restricted Project

Oct 21 2019

shafik added a comment to D68961: Add support for DW_AT_export_symbols for anonymous structs .

We also have the lldb-cmake-matrix bot which runs the test suite using older versions of clang which is there for exactly this purpose to catch regressions due to features not supported by older compiler versions. Which would catch any regressions here.

I'm afraid I have to disagree with that. Like I said the last time <D66370> when this topic came up, I don't think that this "matrix" testing is the way to test dwarf features. The "matrix" bot is very useful to catch regressions in parsing old debug info (because there will always be some quirk that you forget to think of), but I don't think there should be any piece of code that is _only_ tested this way, particularly when it is very easy to do so (like it is here).

As you're probably aware, there's an "end-to-end testing" discussion going on on the mailing lists right now. One of the main questions being debated is whether it is ok to have a test which checks the assembly generated from some c(++) source code. And there's a lot of pushback saying that this is testing too much. However, even such a test is peanuts compared to clang-ast-from-dwarf-unamed-and-anon-structs.cpp. Even though it's one of the simplest lldb test we have, it runs the compiler front end, back end *and* assembler, and then reads the output hoping that the pipeline has generated the thing it wants to test. (And btw, it will fail on windows.) It doesn't even check that the compiler has generated the thing it wants to check, so if the compiler output later changes to not include DW_AT_export_symbols, it will start to test something completely different, leaving the code added here uncovered, unless someone remembers to add a new row to the matrix bot.

Not that this is very likely to happen in this particular case, but these kinds of changes happen all the time. Just last week, llvm started generating a very different style of location lists. Both lldb <rL374769> and dsymutil <D69005> had to be fixed to handle that. Both patches had tests, which were not based on running clang to generate the debug info (the dsymutil test had a checked in binary, which I am not a fan of, but that's a different story). Even so, it's very likely that this has regressed our coverage of the previous forms of location lists because we had no tests (except some basic ones I've added two or three months ago) which tested this code directly. And we only know about this because the initial implementation was wrong/incomplete, so we noticed the breakage.

So, overall, I still think we should have an even more targeted test here. Maybe it's not fair to ask you to add this kind of a test for the "old" style structs because that is past and not really "on you". However, I think we should be trying to add these kinds of tests for new features whenever is possible (another thing -- I imagine debugging failures from the matrix bot can be hard, though I've fortunately had not had to do that yet). I've been trying to do that for the past year, and while it takes a while to get used to, after some time I've gotten fairly efficient at generating/reducing "raw" dwarf. Besides being able to generate "stable" tests, this also enables one to generate tests for "incorrect" dwarf, which is one of the things which is impossible with the "clang -g" approach.

Oct 21 2019, 12:59 PM · Restricted Project
shafik added a comment to D68961: Add support for DW_AT_export_symbols for anonymous structs .
Oct 21 2019, 12:58 PM · Restricted Project

Oct 17 2019

shafik updated the diff for D68961: Add support for DW_AT_export_symbols for anonymous structs .

Minor fixes.

Oct 17 2019, 1:32 PM · Restricted Project
shafik added a comment to D68961: Add support for DW_AT_export_symbols for anonymous structs .

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

We don't except any regressions for code compiled with older compilers. We are fixing the case that unnamed classes are identified as anonymous. The anonymous classes cases should be caught in older revisions in ClangASTContext::AddFieldToRecordType which does a Rec->setAnonymousStructOrUnion(true) for those cases.

Would it make sense to write a test in asm then? That way it would be obvious exactly what kind of debug info is being tested, and you could ensure both old&new compiler behavior is covered.

Oct 17 2019, 1:31 PM · Restricted Project

Oct 16 2019

shafik added a comment to D68961: Add support for DW_AT_export_symbols for anonymous structs .

Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in order to fix an issue when debugging in lldb?

Oct 16 2019, 10:37 AM · Restricted Project

Oct 14 2019

shafik created D68961: Add support for DW_AT_export_symbols for anonymous structs .
Oct 14 2019, 4:40 PM · Restricted Project

Oct 11 2019

shafik committed rG5f46982b452b: [lldb-test] Modify lldb-test to print out ASTs from symbol file (authored by shafik).
[lldb-test] Modify lldb-test to print out ASTs from symbol file
Oct 11 2019, 9:38 AM
shafik closed D67994: Modify lldb-test to print out ASTs from symbol file.
Oct 11 2019, 9:38 AM · Restricted Project
shafik committed rL374570: [lldb-test] Modify lldb-test to print out ASTs from symbol file.
[lldb-test] Modify lldb-test to print out ASTs from symbol file
Oct 11 2019, 9:38 AM

Oct 10 2019

shafik updated the diff for D67994: Modify lldb-test to print out ASTs from symbol file.
  • Simplified DumpFromSymbolFile() based on comments
  • Reused -name option in lldb-test and to replace -symbol-name
Oct 10 2019, 2:34 PM · Restricted Project
shafik added inline comments to D67994: Modify lldb-test to print out ASTs from symbol file.
Oct 10 2019, 2:34 PM · Restricted Project
shafik added inline comments to D68010: [lldb] Fix string summary of an empty NSPathStore2.
Oct 10 2019, 9:36 AM · Restricted Project

Oct 9 2019

shafik added a comment to D68641: [LLDB] Fix for synthetic children memory leak.

This change broek the`TestDataFormatterInvalidStdUniquePtr.py` test, see: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2411/testReport/

Oct 9 2019, 1:37 PM · Restricted Project
shafik created D68722: Add an example showing the alternative to nested designators.
Oct 9 2019, 12:42 PM
shafik added a comment to D68533: Explicitly set entry point arch when it's thumb [Second Try].

SymbolFile/Breakpad.symtab.test is failing on green dragon cmake build see: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2378/

Oct 9 2019, 10:09 AM · Restricted Project

Oct 8 2019

shafik added inline comments to D68671: Add the ability to pass extra args to a Python breakpoint command function.
Oct 8 2019, 4:55 PM · Restricted Project
shafik added a comment to D67994: Modify lldb-test to print out ASTs from symbol file.

Maybe this is my fault since I'm the one who introduced the first bunch of arguments here IIRC, but anyway, I have a feeling all of these dumping options are getting out of hand. Looking at the argument list, you'd expect that -dump-ast, and -dump-clang-ast do something completely different, but they actually print the exact same AST, only one allows you to print a subset of it, while the other one doesn't.

Given that the ast dumping is currently incompatible with all/most of the other dumping options anyway, maybe we should turn over a clean slate, and implement a new subcommand specifically dedicated to dumping clang ast (as opposed to the internal lldb representations of types/functions/..., which is what the "symbols" subcommand started out as, and which is what most of its options are dedicated to).

OR, and this would-be super cool, if it was actually possible, we could try to make ast-dumping compatible with the existing searching options. Currently, if you type lldb-test symbols -find=type -name=foo it will search for types named "foo", and print some summary of the lldb object representing that type. What if we made it so that adding -dump-ast to that command caused the tool to dump the *AST* of the types it has found (e.g., in addition to the previous summary)? I think that would make the behavior of the tool most natural to a new user, but I am not sure whether that would actually fit in with your goals here...

The output is actually quite different for example given:

struct A {
  struct {
      int x;
  };
} a;

the output of lldb-test symbols -dump-ast anon.o is:

Module: anon.o
struct A;

while the output of lldb-test symbols -dump-clang-ast anon.o is:

Module: anon.o
A
CXXRecordDecl 0x7ff3698262c8 <<invalid sloc>> <invalid sloc> struct A definition
|-DefinitionData pass_in_registers aggregate standard_layout trivially_copyable pod trivial literal
...

Given they are both working with the Symbol File I believe they both belong under the symbol command. I feel like -dump-ast is a little misleading but -dump-clang-ast feels good.

You're right about. I'm sorry, I should have checked what -dump-ast does. It definitely is confusing that it does something completely different than the "dump ast" lldb command.

But what about the second part of my comment? I think it would still be nice if the lldb-test symbols options could be somehow factored into two groups:

  • a group which selects *what* to dump: this would be -find, -name, etc.
  • a group which selects *how* to dump it: -dump-clang-ast, -dump-ast :/, the default mode

    would it be somehow possible to hook in this logic into the findTypes function in lldb-test, so that instead of calling TypeMap::Dump, it does something else if -dump-clang-ast flag is present (this "else" resuling in the printing of clang ast)?
Oct 8 2019, 4:08 PM · Restricted Project
shafik added inline comments to D68634: [ASTImporter] Imported FunctionDecls inherit __attribute__((noreturn)) and analyzer_noreturn attrs from existing functions.
Oct 8 2019, 12:12 PM · Restricted Project
shafik accepted D68641: [LLDB] Fix for synthetic children memory leak.

LGTM

Oct 8 2019, 11:07 AM · Restricted Project
shafik added a comment to D67520: Add pretty printing of Clang "bitfield" enums.

See the same break on green dragon as well: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2360/testReport/LLDB/SymbolFile_NativePDB/s_constant_cpp/

Oct 8 2019, 10:06 AM · Restricted Project
shafik added a reviewer for D68641: [LLDB] Fix for synthetic children memory leak: shafik.
Oct 8 2019, 9:47 AM · Restricted Project
shafik committed rG02376077be55: Revert "[platform process list] add a flag for showing the processes of all… (authored by shafik).
Revert "[platform process list] add a flag for showing the processes of all…
Oct 8 2019, 9:25 AM
shafik added a comment to D68354: [platform process list] add a flag for showing the processes of all users.

I reverted this change b/c the green dragon bots have been red for too long.

Oct 8 2019, 9:25 AM · Restricted Project
shafik committed rL374077: Revert "[platform process list] add a flag for showing the processes of all….
Revert "[platform process list] add a flag for showing the processes of all…
Oct 8 2019, 9:24 AM

Oct 7 2019

shafik added a comment to D68354: [platform process list] add a flag for showing the processes of all users.

@wallace it looks like TestPlatformClient.py is failing on osx see green dragon log here: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2317/testReport/junit/lldb-Suite/functionalities_gdb_remote_client/TestPlatformClient_py/

Oct 7 2019, 2:33 PM · Restricted Project

Oct 4 2019

shafik added a comment to D67994: Modify lldb-test to print out ASTs from symbol file.

Maybe this is my fault since I'm the one who introduced the first bunch of arguments here IIRC, but anyway, I have a feeling all of these dumping options are getting out of hand. Looking at the argument list, you'd expect that -dump-ast, and -dump-clang-ast do something completely different, but they actually print the exact same AST, only one allows you to print a subset of it, while the other one doesn't.

Given that the ast dumping is currently incompatible with all/most of the other dumping options anyway, maybe we should turn over a clean slate, and implement a new subcommand specifically dedicated to dumping clang ast (as opposed to the internal lldb representations of types/functions/..., which is what the "symbols" subcommand started out as, and which is what most of its options are dedicated to).

OR, and this would-be super cool, if it was actually possible, we could try to make ast-dumping compatible with the existing searching options. Currently, if you type lldb-test symbols -find=type -name=foo it will search for types named "foo", and print some summary of the lldb object representing that type. What if we made it so that adding -dump-ast to that command caused the tool to dump the *AST* of the types it has found (e.g., in addition to the previous summary)? I think that would make the behavior of the tool most natural to a new user, but I am not sure whether that would actually fit in with your goals here...

Oct 4 2019, 1:08 PM · Restricted Project

Oct 3 2019

shafik added a comment to D68422: [DWARFASTParserClang] Factor out structure-like type parsing, NFC.

Thank you for doing this, this looks like an excellent start from a quick review.

Oct 3 2019, 3:47 PM · Restricted Project