[Lldb-commits] [PATCH] D52572: Replace pointer to C-array of PropertyDefinition with llvm::ArrayRef

2018-09-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: tatyana-krasnukha.
zturner added a comment.

It would be nice if You could replace the logic that iterates these arrays.
We no longer need to terminate on a sentinel nullptr entry and can now use
range based for loop


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52572



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52572: Replace pointer to C-array of PropertyDefinition with llvm::ArrayRef

2018-09-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

You might be right, I’m on mobile so hard for me to review. I saw a bunch
of nullptr so assumed it was still using sentinel values for iterating. If
not, ignore my suggestion


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52572



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52572: Replace pointer to C-array of PropertyDefinition with llvm::ArrayRef

2018-09-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

No, separate revision is fine. Thanks!


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52572



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-09-27 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: aleksandr.urakov.
zturner added a comment.

It requires a lot of work (nobody has started porting it). lldb-server
exists on other platforms but it basically needs a full port to Windows. It
doesn’t use the same process plugin, but it would instead use a different
plugin that would copy much of the low level code. Once it works well
enough the existing process plugin would then be deleted. It would be great
if someone had time to work on it, but I understand if for practical
reasons other work has higher priority


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52468: [PDB] Treat `char`, `signed char` and `unsigned char` as three different types

2018-09-27 Thread Zachary Turner via Phabricator via lldb-commits
zturner accepted this revision.
zturner added a comment.

Can you change the description of the patch before submitting it?  It's hard to 
understand why the change does what the description says it does, because the 
description mentions 3 types of chars but the patch only handles 1.  I would 
just make the description say "handle char as a builtin type" or something


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52468



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52627: [lldb] Start a new line for the next output if there are no symbols in the current symtab

2018-09-27 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Couple of options:

1. Can you give an example of before/after output?
2. Is the `size_t` change related?
3. Can you use -U99 in the future when generating patches?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52627



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52627: [lldb] Start a new line for the next output if there are no symbols in the current symtab

2018-09-27 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

s/options/comments/


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52627



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52626: [lldb] Remove an assertion in RichManglingContext::GetBufferRef() hit when debugging a native x86 Windows process

2018-09-27 Thread Zachary Turner via Phabricator via lldb-commits
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

BTW, I wrote a demangler for Windows ABI that should be 100% complete modulo 
bugs and can generate an AST that RichManglingContext should use.  So it would 
be interesting if someone decided to start using that to make this actually 
work.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52626



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-09-28 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

I think it’s fine. Eventually when you are ready to support remote
debugging hopefully we can convert it over to using lldb-server


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-10-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

One idea would be to define some lit substitutions like %debuginfo. It’s
true you can produce a gcc style command line that will be equivalent to a
clang-cl invocation but it won’t be easy. eg you’ll needing to pass
-fms-compatibility as well as various -I for includes.

It may be easier to have substitutions instead


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-10-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D52618#1251076, @stella.stamenova wrote:

> In https://reviews.llvm.org/D52618#1250909, @zturner wrote:
>
> > One idea would be to define some lit substitutions like %debuginfo. It’s
> >  true you can produce a gcc style command line that will be equivalent to a
> >  clang-cl invocation but it won’t be easy. eg you’ll needing to pass
> >  -fms-compatibility as well as various -I for includes.
> >
> > It may be easier to have substitutions instead
>
>
> Another option would be to define a way in lit to specify a command to run 
> based on requirements - similar how we can use "windows" or "linux" in the 
> "requires" command.


Yea that would work too.  `REQUIRES` isn't quite the right thing because that 
just makes the infrastructure decide whether to run or skip your test.  It 
would need to be something different, like `COMPILATION_SETTINGS: debug, opt, 
noexcept`.  But I think that would be quite a bit of work and probably not fit 
nicely with the existing `ShTest`.  You might need a subclass of `ShTest` like 
`LLDBShTest` that can extend its functionality a bit.


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-10-02 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D52618#1252372, @labath wrote:

> In https://reviews.llvm.org/D52618#1250909, @zturner wrote:
>
> > One idea would be to define some lit substitutions like %debuginfo. It’s
> >  true you can produce a gcc style command line that will be equivalent to a
> >  clang-cl invocation but it won’t be easy. eg you’ll needing to pass
> >  -fms-compatibility as well as various -I for includes.
> >
> > It may be easier to have substitutions instead
>
>
> Using substitutions SGTM.
>
> I am not sure if this is a good idea, but it had occured to me that we could 
> put `-fms-compatibility` and friends into a substitution of it's own, which 
> would be computed by lit (through some equivalent of `clang++ -###` ?). That 
> way, the tests could still use g++ syntax, only the command lines would 
> contain an extra `%cflags` argument. This has the advantage of extra 
> flexibility over a predefined set of compile commands (%compile_with_debug, 
> %compile_without_debug, ...), and it might be sufficient to make 
> cross-compiling work, if we ever need it.


Another idea I just thought of, which would basically be the heaviest hammer 
possible and give the most flexibility is to modify the lit infrastructure 
(just for lldb, not all of llvm) to look for a `compiler_config.py` file.  If 
it finds it, it can run the file.  This file could define whatever 
substitutions it wanted.  In the top-level LLDB lit configuration, we could 
provide some basic common infrastructure to easily support common use cases.  I 
don't have specifics in mind for how the implementation would look like, but 
from a user point of view (i.e. what the `compiler_config.py` would look like), 
I'm imagining you could write something like this:

  # compiler_config.py
  global compiler_config
  compiler_config.create_configuration(
  "fooconfig",   # user can reference this config as 'fooconfig' from a 
test file.
  driver=best,# use clang-cl on Windows, clang++ otherwise
  debug=True,   # pass /Z7 on Windows, -g otherwise
  optlevel=1, # pass /O2 on Windows, -O2 otherwise
  output_type=sharedlib,   # pass -fPIC on Linux and /D_DLL on Windows
  exceptions=False, # pass -fno-exceptions on Linux, /EHs-c- on Windows
  rtti=False,   # pass -fno-rtti on Linux, /GR- on Windows
  mode=compile-only)   # Don't run the linker
  
  compiler_config.create_configuration(
  "barconfig", # user can reference this config as 'barconfig' from a test 
file.
  driver=g++, # this config always invokes clang++, never anything else.
  debug=False, optlevel=3, output_type=exe)  # etc

Then, in your test file, you could have:

  ; RUN: %fooconfig %p/foo.cpp
  ; RUN: %barconfig %p/bar.cpp

This is a very rough outline of the idea, but I think this could actually be 
really cool if done properly.


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

2018-10-10 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

> By the way, what do you think, how can we make LLDB support aligned stacks? 
> As far as I know, similar alignment problems are reproducible on non-Windows 
> too.

When you see VFRAME, you need to look in FPO data.  As you might have guessed, 
VFRAME only occurs in X86.

I compiled your sample program with a few modifications, and I'll show some 
output of `llvm-pdbutil` to illustrate how analyzing FPO data works (which 
would also give you some insight into how this will eventually be implemented 
without DIA)

Here is the source code I compiled:

  int g_var = ;
  
  void __fastcall with_double(short arg_0, float arg_1) { char loc_0 = 'x'; 
double dvar = 0.5678; }
  
  void __fastcall with_float(short arg_0, float arg_1) { char loc_0 = 'x'; 
float fvar = 0.5678f; }
  
  int main(int argc, char *argv[]) {
bool loc_0 = true;
int loc_1 = ;
  
with_double(, 0.1234);
with_float(, 0.1234);
  
return 0;
  }

Then I ran this command.

  $ llvm-pdbutil.exe dump -symbols -modi=0 vlt.pdb
  
  
Symbols
  
Mod  | `D:\src\llvmbuild\cl\Debug\x64\vlt.obj`:

52 | S_GPROC32 [size = 52] `with_double`
 parent = 0, end = 268, addr = 0001:, code size = 50
 type = `0x1001 (void (short, float))`, debug start = 0, debug end 
= 0, flags = none
   104 | S_FRAMEPROC [size = 32]
 size = 28, padding size = 0, offset to padding = 0
 bytes of callee saved registers = 0, exception handler addr = 
:
 local fp reg = VFRAME, param fp reg = EBP
 flags =
   
   236 | S_LOCAL [size = 16] `dvar`
 type=0x0041 (double), flags = none
   252 | S_DEFRANGE_FRAMEPOINTER_REL [size = 16]
 offset = -16, range = [0001:0027,+23)
 gaps = 2
   268 | S_END [size = 4]
   272 | S_GPROC32 [size = 52] `with_float`
 parent = 0, end = 484, addr = 0001:0064, code size = 44
 type = `0x1001 (void (short, float))`, debug start = 0, debug end 
= 0, flags = none
   324 | S_FRAMEPROC [size = 32]
 size = 16, padding size = 0, offset to padding = 0
 bytes of callee saved registers = 0, exception handler addr = 
:
 local fp reg = EBP, param fp reg = EBP
 flags =
   
   452 | S_LOCAL [size = 16] `fvar`
 type=0x0040 (float), flags = none
   468 | S_DEFRANGE_FRAMEPOINTER_REL [size = 16]
 offset = -8, range = [0001:0087,+21)
 gaps = 2
   484 | S_END [size = 4]

the `S_GPROC32` and `S_END` records form a pair, so all relevant data for this 
function is inside of the matching pair.

Both `dvar` and `fvar` are of type `S_DEFRANGE_FRAMEPOINTER_REL`, which means 
they're relative to the framepointer.  So we need to search for the 
`S_FRAMEPROC` record inside of this function.  It's immediately after the 
`S_GPROC32` record in both cases.  In the case of `with_float` we find that it 
says "local fp reg = EBP".  This means it's easy, nothing special to do which 
is why it fixed the issue for you changing to float.  On the other hand, as you 
noticed the other one says `VFRAME`.

This means we need to go to the FPO data.  But first we need to find the 
address of this function.  The `S_GPROC32` of `with_double` says it's at 
address `0001:`.  So we check the section headers to find out what is 
section 1.

  $ llvm-pdbutil.exe dump -section-headers vlt.pdb
  
  
Section Headers
  
  
SECTION HEADER #1
   .text name
   3E3FD virtual size
1000 virtual address
   3E400 size of raw data
 600 file pointer to raw data
   0 file pointer to relocation table
   0 file pointer to line numbers
   0 number of relocations
   0 number of line numbers
6020 flags
 IMAGE_SCN_CNT_CODE
 IMAGE_SCN_MEM_EXECUTE
 IMAGE_SCN_MEM_READ

So now we know section 1 starts at virtual address `0x1000`, and this 
particular function is at offset ``, so it is also at virtual address 
`0x1000`.  The function has size 50, so we are looking for an FPO record in the 
range of `[0x1000,0x1032)`

Now let's look at the FPO data in the PDB.

  Old FPO Data
  
RVA| Code | Locals | Params | Prolog | Saved Regs | Use BP | Has SEH | 
Frame Type
  131A |   20 |  0 |  0 |  0 |  0 |  false |   false |  
 FPO
  1483 |   19 |  0 |  0 |  0 |  0 |  false |   false |  
 FPO
  
  
  New FPO Data
  
RVA| Code | Locals | Params | Stack | Prolog | Saved Regs | Has

[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

2018-10-10 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

You didn't include it here, but I notice the test file just writes `clang-cl 
/Zi`.  Should we be passing `-m32` or `-m64`?  Currently, the test just runs 
with whatever architecture happens to be set via the VS command prompt.  The 
behavior here is different on x86 and x64 so perhaps it requires separate tests.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53086



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53090: [ProcessWindows] Fix a bug that causes lldb to freeze

2018-10-10 Thread Zachary Turner via Phabricator via lldb-commits
zturner accepted this revision.
zturner added a comment.
This revision is now accepted and ready to land.

Nice find, thanks


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53090



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

2018-10-10 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

If you plan to invest in more substantial changes in `ObjectFilePECOFF`, it 
might worth considering a complete re-write in terms of `llvm::object::coff`.  
It has pretty comprehensive support for the PE/COFF spec, so it's just a matter 
of implementing `ObjectFilePECOFF` on top of it.




Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:810
+
+  std::lock_guard guard(module_sp->GetMutex());
+

Does this actually need to be a `recursive_mutex`?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53094



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53166: [lldbsuite] Fix the filecheck functionality to work with Python 3

2018-10-11 Thread Zachary Turner via Phabricator via lldb-commits
zturner added inline comments.



Comment at: packages/Python/lldbsuite/test/lldbtest.py:2230-2233
+# In Python 2, communicate sends byte strings. In Python 3, 
communicate sends bytes.
+# If we got a string (and not a byte string), encode it before sending.
+if isinstance(output, str) and not isinstance(output, bytes):
+output = output.encode()

Ok I had to stare at this for a long time to figure out what was going on.  I 
think you need to update the comment here, because it makes it sounds like 
`output` is the result of a `Popen.communicate`.  But `output` is the result of 
a `debugger.HandleCommand()`.  

I think we should actually just leave this the way it was and fix it in the 
call to `Popen` (see below)



Comment at: packages/Python/lldbsuite/test/lldbtest.py:2247-2249
 subproc = Popen(filecheck_args, stdin=PIPE, stdout=PIPE, stderr=PIPE)
 cmd_stdout, cmd_stderr = subproc.communicate(input=output)
 cmd_status = subproc.returncode

If I'm not mistaken, Python 3 can accept `stdin` as `bytes` or `string`, and 
either will work, it depends on the value of `universal_newlines`, and `stdout` 
will be in whatever format `stdin` was in.  If we pass 
`universal_newlines=True`, then it will expect a `string` and output a 
`string`, otherwise it expects a `bytes` and output a `bytes`.  Given that 
`FileCheck` is supposed to operate on textual data and not binary data, perhaps 
we should be doing that here?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53166



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

2018-10-11 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: vsk.
zturner added a comment.

See the other email thread.  The bots have been broken since September, all
of them complain about missing FileCheck executable


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50478



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D50478: Add support for artificial tail call frames

2018-10-11 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: aprantl.
zturner added a comment.

https://reviews.llvm.org/D53002

Is the thread I'm referring to.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50478



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53175: [dotest] Make a missing FileCheck binary a warning, not an error

2018-10-11 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: vsk.
zturner added a comment.

Why is FileCheck missing in the first place?


https://reviews.llvm.org/D53175



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

2018-10-11 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53086#1261697, @aleksandr.urakov wrote:

> Thanks a lot for so detailed answer, it helps!
>
> So we need to parse a FPO program and to convert it in a DWARF expression 
> too. The problem here (in the DIA case) is that I don't know how to retrieve 
> the required FPO range (we have a symbol context when creating a variable, 
> but it seems that it doesn't contain enough information).


I think the `SymbolContext` should have enough information.  As long as you can 
find the `PDBSymbolFunction` for the current frame, that gives you the range of 
the function, and the `IDiaSymbol` for the variable should give you the 
important info like register, offset, etc.

> As for the raw PDB case, can the same `S_LOCAL` have several 
> `S_DEFRANGE_FRAMEPOINTER_REL` records for several ranges?  If so, then the 
> problem exists for the raw PDB case too, but there's a solution: we can 
> combine processing of all `S_DEFRANGE_FRAMEPOINTER_REL` records in the single 
> DWARF expression (and call the required FPO program depending on current 
> `eip`).

Not as far as I know.  There should be only 1.  But note that is not the only 
type of record that can appear, there are several others.  But basically there 
will be `S_LOCAL` followed by 1 record describing the location.

> The interesting moment here is that MSVC doesn't emit correct information for 
> locals. For the program `aaa.cpp`:
> 
>   void bar(char a, short b, int c) { }
>   
>   void __fastcall foo(short arg_0, float arg_1) {
> char loc_0 = 'x';
> double __declspec(align(8)) loc_1 = 0.5678;
> bar(1, 2, 3);
>   }
>   
>   int main(int argc, char *argv[]) {
> foo(, 0.1234);
> return 0;
>   }
> 
> 
> compiled with `cl /Zi /Oy aaa.cpp` we have the next disassembly of `foo`:
> 
>   pushebp
>   mov ebp, esp
>   and esp, 0FFF8h
>   sub esp, 10h
>   mov [esp+4], cx ; arg_0
>   mov byte ptr [esp+3], 'x' ; loc_0
>   movsd   xmm0, ds:__real@3fe22b6ae7d566cf
>   movsd   qword ptr [esp+8], xmm0 ; loc_1
>   push3   ; c
>   push2   ; b
>   push1   ; a
>   callj_?bar@@YAXDFH@Z ; bar(char,short,int)
>   add esp, 0Ch
>   mov esp, ebp
>   pop ebp
>   retn4
> 
> 
> but MSVC emits the next info about locals:
> 
>   296 | S_GPROC32 [size = 44] `foo`
> parent = 0, end = 452, addr = 0001:23616, code size = 53
> type = `0x1003 (void (short, float))`, debug start = 14, debug end = 
> 47, flags = has fp
>   340 | S_FRAMEPROC [size = 32]
> size = 16, padding size = 0, offset to padding = 0
> bytes of callee saved registers = 0, exception handler addr = 
> :
> local fp reg = VFRAME, param fp reg = EBP
> flags = has async eh | opt speed
>   372 | S_REGREL32 [size = 20] `arg_0`
> type = 0x0011 (short), register = ESP, offset = 4294967284
>   392 | S_REGREL32 [size = 20] `arg_1`
> type = 0x0040 (float), register = EBP, offset = 8
>   412 | S_REGREL32 [size = 20] `loc_1`
> type = 0x0041 (double), register = ESP, offset = 4294967288
>   432 | S_REGREL32 [size = 20] `loc_0`
> type = 0x0070 (char), register = ESP, offset = 4294967283
>   452 | S_END [size = 4]
>
> 
> so neither LLDB nor Visual Studio show valid values (except for `arg_1`).

Generally speaking, if you're using `/Oy` all bets are off and good luck :)

Interestingly, clang actually gets this right but we use a totally different 
CodeView record.

  $ llvm-pdbutil.exe dump -symbols -modi=0 foo-clang.pdb | grep -A 4 loc_1
   392 | S_LOCAL [size = 16] `loc_1`
 type=0x0041 (double), flags = none
   408 | S_DEFRANGE_FRAMEPOINTER_REL [size = 16]
 offset = -16, range = [0001:0075,+51)
 gaps = 2
  
  D:\src\llvmbuild\ninja-release-x64>

If you try to debug the same program in VS built with clang with the exact same 
command line, it will work.

I need to study the disassembly and FPO program of the cl.exe version some more 
to decide if the bug is in the compiler or the debugger, but I think the bug is 
in the compiler.

It's funny because our optimized debug info is not very good, so I'm surprised 
there's a case where we're better than MSVC here :)


https://reviews.llvm.org/D53086



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53086: [PDB] Fix flaky `variables-locations.test` after PR38857

2018-10-11 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53086#1263001, @zturner wrote:

> In https://reviews.llvm.org/D53086#1261697, @aleksandr.urakov wrote:
>
> > Thanks a lot for so detailed answer, it helps!
> >
> > So we need to parse a FPO program and to convert it in a DWARF expression 
> > too. The problem here (in the DIA case) is that I don't know how to 
> > retrieve the required FPO range (we have a symbol context when creating a 
> > variable, but it seems that it doesn't contain enough information).
>
>
> I think the `SymbolContext` should have enough information.  As long as you 
> can find the `PDBSymbolFunction` for the current frame, that gives you the 
> range of the function, and the `IDiaSymbol` for the variable should give you 
> the important info like register, offset, etc.


Do we have access to the current instruction pointer?  That's what you need to 
find the correct FPO record.


https://reviews.llvm.org/D53086



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52461: [PDB] Introduce `PDBNameParser`

2018-10-15 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D52461#1265335, @aleksandr.urakov wrote:

> Hello!
>
> I just have tried to patch `CPlusPlusNameParser` in the way to support MSVC 
> demangled names, but there is a problem. `CPlusPlusNameParser` splits an 
> incoming name in tokens with `clang::Lexer`. I've lexed the next name:
>
>   `anonymous namespace'::foo
>
>
> The lexer treats the first character (a grave accent) as an unknown token, 
> and it's ok for our purposes. Then it sees an identifier (`anonymous`), a 
> keyword (`namespace`), and it's ok too. But the problem is with the last part 
> of the string. The lexer sees an apostrophe and supposes that it's a 
> character constant, it looks for a closing apostrophe, don't find it and 
> treats all the line ending (`'::foo`) as a single unknown token.
>
> It is possible to somehow make `clang::Lexer` lex MSVC demangled names 
> correctly, but I'm not sure if it is the right place to do it. And it may 
> have then some side effects during lexing a real code.
>
> Another option is to somehow preprocess the name before lexing and replace 
> all //paired// apostrophes with grave accents, and after lexing replace with 
> apostrophes back, and make `CPlusPlusNameParser` understand unknown grave 
> accent tokens. But it's a bit tricky, may be you can suggest some better 
> solution?


Just handle the `anonymous namespace' thing specially before passing to 
`CPlusPlusNameParser`.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D52461



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53402: [SymbolFileNativePDB] Fix missing linkage to DebugInfoCodeView

2018-10-18 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: mgorny.
zturner added a comment.

Lgtm


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53402



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53506: [ClangASTContext] Extract VTable pointers from C++ objects

2018-10-22 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

> PDB has no enough info to restore VBase offsets properly, so we have to read 
> real VTable instead.

What's missing that you're unable to restore the VBase offset properly?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53506



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53506: [ClangASTContext] Extract VTable pointers from C++ objects

2018-10-22 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53506#1270919, @aleksandr.urakov wrote:

> In https://reviews.llvm.org/D53506#1270893, @zturner wrote:
>
> > What's missing that you're unable to restore the VBase offset properly?
>
>
> If I understand correctly, in the PDB there is only info about offset to 
> VTablePtr and index in VTable, so there is enough info to retrieve VBase 
> offset fairly, and we do it in that way. But there's no info in PDB about 
> offset to VBase directly from object. This info is used when the "fair" 
> doesn't work (e.g. at line 6640). This patch just makes the "fair" way to 
> work in more cases.


My understanding of record layout with virtual bases is still sketchy (it's 
very confusing), and it's even worse with DIA because the API is so general and 
poorly documented, so let's go to the low-level CodeView records.

  typedef struct lfVBClass {
  unsigned short  leaf;   // LF_VBCLASS (virtual base) | 
LV_IVBCLASS (indirect virtual base)
  CV_fldattr_tattr;   // attribute
  CV_typ_tindex;  // type index of direct virtual base class
  CV_typ_tvbptr;  // type index of virtual base pointer
  unsigned char   vbpoff[CV_ZEROLEN];   // virtual base pointer offset 
from address point
  // followed by virtual base offset from 
vbtable
  } lfVBClass;

This is what we have access to reading directly from the raw pdb file, which is 
sometimes more information than what we have access to using DIA.  Of course, 
we also have to interpret whether this actually means what we think it means by 
inspecting the bytes of a C++ object in a debugger and comparing the layout to 
what the debug info tells us.

So, the point is, just because we don't have access to the info via DIA doesn't 
mean we won't have access to the info once the native pdb plugin is complete.  
Just something to think about.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53506



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

2018-10-22 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: labath, lemo, aleksandr.urakov, stella.stamenova, 
asmith.
Herald added subscribers: arphaman, mgorny.

This is the minimal set of functionality necessary to support type lookup by 
name in the native PDB plugin.

For the purposes of testing I decided to bypass `lldb-test` and go with a 
scripted lldbinit file.  I'm sticking with this approach wherever possible to 
prove that this is "true" cross-platform debugger functionality.  However, 
there are definite limitations.  For example, the output format of `type 
lookup` has some limitations.  It doesn't print the layout of the type in any 
kind of detail (e.g. field offsets)`, it doesn't support lookup by regex, it 
doesn't print the underlying type of an enumeration, it doesn't support 
limiting the scope of the search to specific kinds of types, so there is 
definitely room for `lldb-test` to come into the picture here to expand the 
coverage since we have full control over the output format.

I tried to think of some interesting test cases to exercise some edge cases 
here, but I welcome some ideas for other interesting cases.  I consciously 
avoided things like bit fields, member functions, pointers to members, and 
virtual bases because there was a balancing act between implementing a useful 
piece of functionality and keeping the patch small enough that it can be 
understandable enough to review meaningfully.

Welcome any feedback.  Hopefully this is getting to the point that maybe it's 
hackable by others as well, although I'm definitely going to continue working 
on it.


https://reviews.llvm.org/D53511

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/tag-types.lldbinit
  lldb/lit/SymbolFile/NativePDB/tag-types.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
===
--- /dev/null
+++ lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
@@ -0,0 +1,68 @@
+//===-- SymbolFileNativePDB.h ---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLDB_PLUGINS_SYMBOLFILE_NATIVEPDB_UDTRECORDCOMPLETER_H
+#define LLDB_PLUGINS_SYMBOLFILE_NATIVEPDB_UDTRECORDCOMPLETER_H
+
+#include "lldb/Symbol/ClangASTImporter.h"
+#include "llvm/DebugInfo/CodeView/CVRecord.h"
+#include "llvm/DebugInfo/CodeView/TypeRecord.h"
+#include "llvm/DebugInfo/CodeView/TypeVisitorCallbacks.h"
+
+#include "PdbSymUid.h"
+
+namespace clang {
+class CXXBaseSpecifier;
+class TagDecl;
+} // namespace clang
+
+namespace lldb_private {
+class Type;
+class CompilerType;
+namespace npdb {
+class SymbolFileNativePDB;
+
+class UdtRecordCompleter : public llvm::codeview::TypeVisitorCallbacks {
+  union UdtTagRecord {
+UdtTagRecord() {}
+llvm::codeview::UnionRecord ur;
+llvm::codeview::ClassRecord cr;
+llvm::codeview::EnumRecord er;
+  } m_cvr;
+
+  PdbSymUid m_uid;
+  CompilerType &m_derived_ct;
+  clang::TagDecl &m_tag_decl;
+  SymbolFileNativePDB &m_symbol_file;
+  std::vector m_bases;
+  ClangASTImporter::LayoutInfo m_layout;
+
+public:
+  UdtRecordCompleter(PdbSymUid uid, CompilerType &derived_ct,
+ clang::TagDecl &tag_decl,
+ SymbolFileNativePDB &symbol_file);
+
+#define MEMBER_RECORD(EnumName, EnumVal, Name) \
+  llvm::Error visitKnownMember(llvm::codeview::CVMemberRecord &CVR,\
+   llvm::codeview::Name##Record &Record) override;
+#define MEMBER_RECORD_ALIAS(EnumName, EnumVal, Name, AliasName)
+#include "llvm/DebugInfo/CodeView/CodeViewTypes.def"
+
+  void complete();
+
+private:
+  lldb::opaque_compiler_type_t
+  AddBaseClassForTypeIndex(llvm::codeview::TypeIndex ti,
+   llvm::codeview::MemberAccess access);
+};
+
+} // namespace npdb
+} // namespace lldb_private
+
+#endif // LLDB_PLUGINS_SYMBOLFILE_NATIVEPDB_UDTRECORDCOMPLETER_H
Index: lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
===
--- /dev/null
+++ lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
@@ -0,0 +1,188 @@
+#include "UdtRecordCompleter.h"
+
+#include "PdbIndex.h"
+#include "PdbSymUid.h"
+#include "PdbUtil.h"
+#include "SymbolFileNativePDB.h"
+
+#include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Symbol

[Lldb-commits] [PATCH] D53368: [Symbol] Search symbols with name and type in a symbol file

2018-10-22 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: aleksandr.urakov.
zturner added a comment.

To answer your question, PE/COFF executable symbol tables are basically
empty


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53368



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

2018-10-22 Thread Zachary Turner via Phabricator via lldb-commits
zturner added inline comments.



Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:87
+// Test single inheritance.
+class Derived : public Class {
+public:

lemo wrote:
> - at least one virtual function + override?
> - at least one method returning void?
The reason I didn't add any tests for methods is because I didn't add anything 
in the implementation to parse method records either.  As I said in the patch 
description, it was a balance between providing some piece of useful 
functionality while keeping the patch size down.  So I tried to limit the scope 
to just fields minus bitfields, since there's some extra complexity there that 
I wanted to punt on for now.  The reason I have the constructor is because it 
was required in order to initialize the reference member, otherwise it wouldn't 
compile.



Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:110
+
+// Test multiple inheritance, as well as protected and private inheritance.
+class Derived2 : protected Class, private Struct {

lemo wrote:
> just an idea - add a virtual inheritance variation?
Virtual inheritance is a good followup, but if you look at 
`UdtRecordCompleter.cpp` and Ctrl+F for `VirtualBaseClassRecord`, I basically 
treat them as regular base classes and I put a fixme to handle the virtual 
aspects of the base.  So that's a known problem right now.



Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:131
+
+int main(int argc, char **argv) {
+  Struct S;

lemo wrote:
> a few suggestions for additional things to cover:
> - local classes
> - lambdas
> - instantiating class and function templates
>- explicit specializations
>- for classes, partial specializations
> - namespaces
> - anonymous namespace
Some of these I could probably handle right now.  I tried to keep the scope of 
the patch to "types which would be named", because it makes it easy to write 
tests (the test can just do lookup by name, basically a WinDbg `dt` test.).  
That makes local classes, lambdas, anonymous namespace, and anything to do with 
functions out of scope.


https://reviews.llvm.org/D53511



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB345047: [NativePDB] Add basic support for tag types to 
the native pdb plugin. (authored by zturner, committed by ).
Herald added subscribers: teemperor, abidh.

Changed prior to commit:
  https://reviews.llvm.org/D53511?vs=170447&id=170683#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53511

Files:
  lit/SymbolFile/NativePDB/Inputs/tag-types.lldbinit
  lit/SymbolFile/NativePDB/tag-types.cpp
  source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h

Index: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -10,6 +10,7 @@
 #ifndef LLDB_PLUGINS_SYMBOLFILE_NATIVEPDB_SYMBOLFILENATIVEPDB_H
 #define LLDB_PLUGINS_SYMBOLFILE_NATIVEPDB_SYMBOLFILENATIVEPDB_H
 
+#include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Symbol/SymbolFile.h"
 
 #include "llvm/ADT/DenseMap.h"
@@ -144,22 +145,63 @@
   llvm::pdb::PDBFile &GetPDBFile() { return m_index->pdb(); }
   const llvm::pdb::PDBFile &GetPDBFile() const { return m_index->pdb(); }
 
+  ClangASTContext &GetASTContext() { return *m_clang; }
+  ClangASTImporter &GetASTImporter() { return *m_importer; }
+
 private:
+  void AddBaseClassesToLayout(CompilerType &derived_ct,
+  ClangASTImporter::LayoutInfo &layout,
+  const llvm::codeview::ClassRecord &record);
+  void AddMembersToLayout(ClangASTImporter::LayoutInfo &layout,
+  const llvm::codeview::TagRecord &record);
+  void AddMethodsToLayout(ClangASTImporter::LayoutInfo &layout,
+  const llvm::codeview::TagRecord &record);
+
+  size_t FindTypesByName(llvm::StringRef name, uint32_t max_matches,
+ TypeMap &types);
+
+  lldb::TypeSP CreateModifierType(PdbSymUid type_uid,
+  const llvm::codeview::ModifierRecord &mr);
+  lldb::TypeSP CreatePointerType(PdbSymUid type_uid,
+ const llvm::codeview::PointerRecord &pr);
+  lldb::TypeSP CreateSimpleType(llvm::codeview::TypeIndex ti);
+  lldb::TypeSP CreateTagType(PdbSymUid type_uid,
+ const llvm::codeview::ClassRecord &cr);
+  lldb::TypeSP CreateTagType(PdbSymUid type_uid,
+ const llvm::codeview::EnumRecord &er);
+  lldb::TypeSP CreateTagType(PdbSymUid type_uid,
+ const llvm::codeview::UnionRecord &ur);
+  lldb::TypeSP
+  CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name, size_t size,
+ clang::TagTypeKind ttk,
+ clang::MSInheritanceAttr::Spelling inheritance);
+
   lldb::FunctionSP GetOrCreateFunction(PdbSymUid func_uid,
const SymbolContext &sc);
   lldb::CompUnitSP GetOrCreateCompileUnit(const CompilandIndexItem &cci);
+  lldb::TypeSP GetOrCreateType(PdbSymUid type_uid);
+  lldb::TypeSP GetOrCreateType(llvm::codeview::TypeIndex ti);
 
   lldb::FunctionSP CreateFunction(PdbSymUid func_uid, const SymbolContext &sc);
   lldb::CompUnitSP CreateCompileUnit(const CompilandIndexItem &cci);
+  lldb::TypeSP CreateType(PdbSymUid type_uid);
+  lldb::TypeSP CreateAndCacheType(PdbSymUid type_uid);
 
   llvm::BumpPtrAllocator m_allocator;
 
   lldb::addr_t m_obj_load_address = 0;
 
   std::unique_ptr m_index;
+  std::unique_ptr m_importer;
+  ClangASTContext *m_clang = nullptr;
+
+  llvm::DenseMap m_decl_to_status;
+
+  llvm::DenseMap m_uid_to_decl;
 
   llvm::DenseMap m_functions;
   llvm::DenseMap m_compilands;
+  llvm::DenseMap m_types;
 };
 
 } // namespace npdb
Index: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
===
--- source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
+++ source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
@@ -0,0 +1,188 @@
+#include "UdtRecordCompleter.h"
+
+#include "PdbIndex.h"
+#include "PdbSymUid.h"
+#include "PdbUtil.h"
+#include "SymbolFileNativePDB.h"
+
+#include "lldb/Symbol/ClangASTContext.h"
+#include "lldb/Symbol/Type.h"
+#include "lldb/Utility/LLDBAssert.h"
+#include "lldb/lldb-enumerations.h"
+#include "lldb/lldb-forward.h"
+
+#include "llvm/DebugInfo/CodeView/TypeDeserializer.h"
+#include "llvm/DebugInfo/CodeView/TypeIndex.h"
+#include "llvm/DebugInfo/PDB/Native/TpiStream.h"
+#include "llvm/DebugInfo/PDB/PDBTypes.h"
+
+using namespace llvm::codeview;
+using namespace llvm::pdb;
+using namespace lldb;
+using nam

[Lldb-commits] [PATCH] D53590: Refactor SetBaseClassesForType and DeleteBaseClasses to be more C++'y

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: labath, davide, clayborg.
Herald added a subscriber: JDevlieghere.

  [NFC] Refactor SetBaseClasses and DeleteBaseClasses.
  
  We currently had a 2-step process where we had to call
  SetBaseClassesForType and DeleteBaseClasses.  Every single caller
  followed this exact 2-step process, and there was manual memory
  management going on with raw pointers.  We can do better than this
  by storing a vector of unique_ptrs and passing this around.
  This makes for a cleaner API, and we only need to call one method
  so there is no possibility of a user forgetting to call
  DeleteBaseClassSpecifiers.
  
  In addition to this, it also makes for a *simpler* API.  Part of
  why I wanted to do this is because when I was implementing the native
  PDB interface I had to spend some time understanding exactly what I
  was deleting and why.  ClangAST has significant mental overhead
  associated with it, and reducing the API surface can go along
  way to making it simpler for people to understand.


https://reviews.llvm.org/D53590

Files:
  lldb/include/lldb/Symbol/ClangASTContext.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
  lldb/source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  lldb/source/Symbol/ClangASTContext.cpp
  lldb/unittests/Symbol/TestClangASTContext.cpp

Index: lldb/unittests/Symbol/TestClangASTContext.cpp
===
--- lldb/unittests/Symbol/TestClangASTContext.cpp
+++ lldb/unittests/Symbol/TestClangASTContext.cpp
@@ -337,15 +337,19 @@
   EXPECT_NE(nullptr, non_empty_base_field_decl);
   EXPECT_TRUE(ClangASTContext::RecordHasFields(non_empty_base_decl));
 
+  std::vector> bases;
+
   // Test that a record with no direct fields, but fields in a base returns true
   CompilerType empty_derived = m_ast->CreateRecordType(
   nullptr, lldb::eAccessPublic, "EmptyDerived", clang::TTK_Struct,
   lldb::eLanguageTypeC_plus_plus, nullptr);
   ClangASTContext::StartTagDeclarationDefinition(empty_derived);
-  CXXBaseSpecifier *non_empty_base_spec = m_ast->CreateBaseClassSpecifier(
-  non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, false, false);
-  bool result = m_ast->SetBaseClassesForClassType(
-  empty_derived.GetOpaqueQualType(), &non_empty_base_spec, 1);
+  std::unique_ptr non_empty_base_spec =
+  m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(),
+  lldb::eAccessPublic, false, false);
+  bases.push_back(std::move(non_empty_base_spec));
+  bool result = m_ast->TransferBaseClasses(empty_derived.GetOpaqueQualType(),
+   std::move(bases));
   ClangASTContext::CompleteTagDeclarationDefinition(empty_derived);
   EXPECT_TRUE(result);
   CXXRecordDecl *empty_derived_non_empty_base_cxx_decl =
@@ -363,10 +367,12 @@
   nullptr, lldb::eAccessPublic, "EmptyDerived2", clang::TTK_Struct,
   lldb::eLanguageTypeC_plus_plus, nullptr);
   ClangASTContext::StartTagDeclarationDefinition(empty_derived2);
-  CXXBaseSpecifier *non_empty_vbase_spec = m_ast->CreateBaseClassSpecifier(
-  non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, true, false);
-  result = m_ast->SetBaseClassesForClassType(empty_derived2.GetOpaqueQualType(),
- &non_empty_vbase_spec, 1);
+  std::unique_ptr non_empty_vbase_spec =
+  m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(),
+  lldb::eAccessPublic, true, false);
+  bases.push_back(std::move(non_empty_vbase_spec));
+  result = m_ast->TransferBaseClasses(empty_derived2.GetOpaqueQualType(),
+  std::move(bases));
   ClangASTContext::CompleteTagDeclarationDefinition(empty_derived2);
   EXPECT_TRUE(result);
   CXXRecordDecl *empty_derived_non_empty_vbase_cxx_decl =
@@ -377,9 +383,6 @@
empty_derived_non_empty_vbase_cxx_decl, false));
   EXPECT_TRUE(
   ClangASTContext::RecordHasFields(empty_derived_non_empty_vbase_decl));
-
-  delete non_empty_base_spec;
-  delete non_empty_vbase_spec;
 }
 
 TEST_F(TestClangASTContext, TemplateArguments) {
Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -8239,39 +8239,37 @@
 
 #pragma mark C++ Base Classes
 
-clang::CXXBaseSpecifier *
+std::unique_ptr
 ClangASTContext::CreateBaseClassSpecifier(lldb::opaque_compiler_type_t type,
   AccessType access, bool is_virtual,
   bool base_of_class) {
-  if (type)
-return new clang::CXXBaseSpecifier(

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: clayborg, jingham, labath.
Herald added subscribers: atanasyan, JDevlieghere, sdardis.

When we get the `resolve_scope` parameter from the SB API, it's a `uint32_t`.  
We then pass it through all of LLDB this way, as a uint32.  This is 
unfortunate, because it means the user of an API never actually knows what 
they're dealing with.  We can call it something like `resolve_scope` and have 
comments saying "this is a value from the `SymbolContextItem` enumeration, but 
it makes more sense to just have it actually *be* the correct type in the 
actual C++ type system to begin with.  This way the person reading the code 
just knows what it is.

There is one thing here which I'm a little uncertain about, which is that I 
included a file from llvm `llvm/ADT/BitmaskEnum.h` from 
`lldb/lldb-enumerations.h`, which is a public header.`  This is only done for 
convenience, and has two effects.

1. It allows us to use bitwise operations on enums so we don't have to write 
things like `Foo x = Foo(eFoo1 | eFoo2)`
2. More subtly, it inserts an enumerator into the enum.  But (!) it doesn't 
change the value of any existing enumerators and it does so with a name that 
won't cause any clashes, so the important thing is that it shouldn't cause any 
name clashes.

Putting this all together, I think this should be an ABI-compatible change as 
far as SWIG is concerned, and I can't see any differences on my end.  But if 
anyone can see any reason why we should be wary of this, speak up.

Assuming all goes well with this patch, I plan to repeat the same thing with 
`SymbolFile::GetTypes` which currently has a `uint32_t types_mask`


https://reviews.llvm.org/D53597

Files:
  lldb/include/lldb/Core/Address.h
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Symbol/CompileUnit.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/include/lldb/Target/StackFrame.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/SBAddress.cpp
  lldb/source/API/SBFrame.cpp
  lldb/source/API/SBModule.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/Address.cpp
  lldb/source/Core/Disassembler.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp
  lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp
  lldb/source/Plugins/Process/Utility/UnwindMacOSXFrameBackchain.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Symbol/SymbolVendor.cpp
  lldb/source/Target/StackFrame.cpp

Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -35,6 +35,8 @@
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/RegisterValue.h"
 
+#include "lldb/lldb-enumerations.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -262,7 +264,8 @@
 // StackFrame object, everyone will have as much information as possible and no
 // one will ever have to look things up manually.
 //--
-const SymbolContext &StackFrame::GetSymbolContext(uint32_t resolve_scope) {
+const SymbolContext &
+StackFrame::GetSymbolContext(SymbolContextItem resolve_scope) {
   std::lock_guard guard(m_mutex);
   // Copy our internal symbol context into "sc".
   if ((m_flags.Get() & resolve_scope) != resolve_scope) {
@@ -314,7 +317,7 @@
   // haven't already tried to lookup one of those things. If we haven't
   // then we will do the query.
 
-  uint32_t actual_resolve_scope = 0;
+  SymbolContextItem actual_resolve_scope = SymbolContextItem(0);
 
   if (resolve_scope & eSymbolContextCompUnit) {
 if (m_flags.IsClear(eSymbolContextCompUnit)) {
Index: lldb/source/Symbol/SymbolVendor.cpp
===
--- lldb/source/Symbol/SymbolVendor.cpp
+++ lldb/source/Symbol/SymbolVendor.cpp
@@ -235,7 +235,7 @@
 }
 
 uint32_t SymbolVendor::Res

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53597#1273087, @jingham wrote:

> It would be great not to have to use comments to express what these values 
> mean.  OTOH, having to put in static casts whenever you want to or values 
> together would be a pain, so if there's no way to do it without magic, I'm a 
> little less enthused...
>
> But on the magic: It looks like BitMaskEnum.h pulls in MathTypes.h which 
> pulls in Compiler.h which then pulls in llvm-config.h.  You are supposed to 
> be able to build tools on top of lldb with just the headers that go in 
> LLDB.framework, you are not required to have the full source tree for an lldb 
> build.  I don't want to impose that restriction without very good reason, and 
> fixing this wart doesn't rise to that level.  Anyway, so if we want to 
> include BitMaskEnum.h we would be required to ship a bunch of llvm headers 
> (including some build produced ones IIUC).  I don't think that's a very good 
> idea.
>
> It also looks to me like the value of the largest item + 1 gets baked into 
> the operator overloading code.  What would happen if we decided to add an 
> element to the enum?


It's not the largest item +1, it's actually just that the largest item gets a 
second internal name.  If you add a new element to the enum you need to just 
make sure you update the `LLVM_MARK_AS_BITMASK_ENUM()` to contain the new 
largest value.

Anyway, your point about `llvm-config.h` is a good one, so what I can perhaps 
do instead is make something called `LLDB_DEFINE_BITMASK_ENUM(EnumName)` which 
basically just defines the overloads for a particular enum, then we can have it 
be a two step process.  1. Make the enum, 2. Call 
`LLDB_DEFINE_BITMASK_ENUM(Enum)`.  This way there's no external header 
dependencies.  I'll upload a new patch shortly.


https://reviews.llvm.org/D53597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53590: Refactor SetBaseClassesForType and DeleteBaseClasses to be more C++'y

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner added inline comments.



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3273-3274
+  is_virtual, is_base_of_class);
+  if (!result)
+break;
+

If the result is a `nullptr`, then there is no base to begin with.  This 
indicates an error, so continuing doesn't make sense.  In fact, I think this 
was a bug with the previous implementation.  We were pushing back a null base 
onto the list of base classes.  I suspect it never actually happened in 
practice, though.


https://reviews.llvm.org/D53590



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53590: Refactor SetBaseClassesForType and DeleteBaseClasses to be more C++'y

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner added inline comments.



Comment at: 
lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3273-3274
+  is_virtual, is_base_of_class);
+  if (!result)
+break;
+

zturner wrote:
> If the result is a `nullptr`, then there is no base to begin with.  This 
> indicates an error, so continuing doesn't make sense.  In fact, I think this 
> was a bug with the previous implementation.  We were pushing back a null base 
> onto the list of base classes.  I suspect it never actually happened in 
> practice, though.
One alternative would be to assert that it is *not* null, then continue as the 
previous code did.  WDYT?


https://reviews.llvm.org/D53590



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner updated this revision to Diff 170737.
zturner added a comment.

Remove the reference to `llvm/ADT/BitmaskEnu.h` and define the operators 
ourselves.  Also, removed a few casts that got left in.


https://reviews.llvm.org/D53597

Files:
  lldb/include/lldb/Core/Address.h
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Symbol/CompileUnit.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/include/lldb/Target/StackFrame.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/SBAddress.cpp
  lldb/source/API/SBFrame.cpp
  lldb/source/API/SBModule.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/Commands/CommandObjectSource.cpp
  lldb/source/Core/Address.cpp
  lldb/source/Core/Disassembler.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Core/SourceManager.cpp
  lldb/source/Plugins/Architecture/Mips/ArchitectureMips.cpp
  lldb/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  
lldb/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/source/Plugins/Process/Utility/RegisterContextLLDB.cpp
  lldb/source/Plugins/Process/Utility/UnwindMacOSXFrameBackchain.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/source/Symbol/CompileUnit.cpp
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Symbol/SymbolVendor.cpp
  lldb/source/Target/StackFrame.cpp

Index: lldb/source/Target/StackFrame.cpp
===
--- lldb/source/Target/StackFrame.cpp
+++ lldb/source/Target/StackFrame.cpp
@@ -35,6 +35,8 @@
 #include "lldb/Target/Thread.h"
 #include "lldb/Utility/RegisterValue.h"
 
+#include "lldb/lldb-enumerations.h"
+
 using namespace lldb;
 using namespace lldb_private;
 
@@ -262,7 +264,8 @@
 // StackFrame object, everyone will have as much information as possible and no
 // one will ever have to look things up manually.
 //--
-const SymbolContext &StackFrame::GetSymbolContext(uint32_t resolve_scope) {
+const SymbolContext &
+StackFrame::GetSymbolContext(SymbolContextItem resolve_scope) {
   std::lock_guard guard(m_mutex);
   // Copy our internal symbol context into "sc".
   if ((m_flags.Get() & resolve_scope) != resolve_scope) {
@@ -314,7 +317,7 @@
   // haven't already tried to lookup one of those things. If we haven't
   // then we will do the query.
 
-  uint32_t actual_resolve_scope = 0;
+  SymbolContextItem actual_resolve_scope = SymbolContextItem(0);
 
   if (resolve_scope & eSymbolContextCompUnit) {
 if (m_flags.IsClear(eSymbolContextCompUnit)) {
Index: lldb/source/Symbol/SymbolVendor.cpp
===
--- lldb/source/Symbol/SymbolVendor.cpp
+++ lldb/source/Symbol/SymbolVendor.cpp
@@ -235,7 +235,7 @@
 }
 
 uint32_t SymbolVendor::ResolveSymbolContext(const Address &so_addr,
-uint32_t resolve_scope,
+SymbolContextItem resolve_scope,
 SymbolContext &sc) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
@@ -248,7 +248,7 @@
 
 uint32_t SymbolVendor::ResolveSymbolContext(const FileSpec &file_spec,
 uint32_t line, bool check_inlines,
-uint32_t resolve_scope,
+SymbolContextItem resolve_scope,
 SymbolContextList &sc_list) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -97,7 +97,7 @@
 
 uint32_t SymbolFile::ResolveSymbolContext(const FileSpec &file_spec,
   uint32_t line, bool check_inlines,
-  uint32_t resolve_scope,
+  lldb::SymbolContextItem resolve_scope,
   SymbolContextList &sc_list) {
   return 0;
 }
Index: lldb/source/Symbol/CompileUnit.cpp
===
--- lldb/source/Symbol/Co

[Lldb-commits] [PATCH] D53616: Don't type-erase the FunctionNameType or TypeClass enums

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: davide, jingham, clayborg.
Herald added a subscriber: JDevlieghere.

This is a followup to https://reviews.llvm.org/D53597, with 2 more enums.  
Assuming that patch is good, I see no reason why this wasn't isn't good as 
well, but I'm throwing it out here for review anyway if nothing else as an FYI. 
 I'll wait for a final LGTM on https://reviews.llvm.org/D53597, but assuming 
that's good, I'll probably submit both of these unless someone has a comment or 
comments on this one first.

I think this is the last of the type-erased enums, so after this, all of our 
flags enums should be strongly typed through the system.


https://reviews.llvm.org/D53616

Files:
  lldb/include/lldb/Breakpoint/BreakpointResolverName.h
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Core/ModuleList.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/include/lldb/Target/Target.h
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/API/SBCompileUnit.cpp
  lldb/source/API/SBModule.cpp
  lldb/source/API/SBTarget.cpp
  lldb/source/Breakpoint/BreakpointResolverName.cpp
  lldb/source/Commands/CommandObjectBreakpoint.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Core/ModuleList.cpp
  lldb/source/Expression/IRExecutionUnit.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Symbol/SymbolVendor.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -415,12 +415,11 @@
   false);
 }
 
-BreakpointSP
-Target::CreateBreakpoint(const FileSpecList *containingModules,
- const FileSpecList *containingSourceFiles,
- const char *func_name, uint32_t func_name_type_mask,
- LanguageType language, lldb::addr_t offset,
- LazyBool skip_prologue, bool internal, bool hardware) {
+BreakpointSP Target::CreateBreakpoint(
+const FileSpecList *containingModules,
+const FileSpecList *containingSourceFiles, const char *func_name,
+FunctionNameType func_name_type_mask, LanguageType language,
+lldb::addr_t offset, LazyBool skip_prologue, bool internal, bool hardware) {
   BreakpointSP bp_sp;
   if (func_name) {
 SearchFilterSP filter_sp(GetSearchFilterForModuleAndCUList(
@@ -443,9 +442,9 @@
 Target::CreateBreakpoint(const FileSpecList *containingModules,
  const FileSpecList *containingSourceFiles,
  const std::vector &func_names,
- uint32_t func_name_type_mask, LanguageType language,
- lldb::addr_t offset, LazyBool skip_prologue,
- bool internal, bool hardware) {
+ FunctionNameType func_name_type_mask,
+ LanguageType language, lldb::addr_t offset,
+ LazyBool skip_prologue, bool internal, bool hardware) {
   BreakpointSP bp_sp;
   size_t num_names = func_names.size();
   if (num_names > 0) {
@@ -465,11 +464,13 @@
   return bp_sp;
 }
 
-BreakpointSP Target::CreateBreakpoint(
-const FileSpecList *containingModules,
-const FileSpecList *containingSourceFiles, const char *func_names[],
-size_t num_names, uint32_t func_name_type_mask, LanguageType language,
-lldb::addr_t offset, LazyBool skip_prologue, bool internal, bool hardware) {
+BreakpointSP
+Target::CreateBreakpoint(const FileSpecList *containingModules,
+ const FileSpecList *containingSourceFiles,
+ const char *func_names[], size_t num_names,
+ FunctionNameType func_name_type_mask,
+ LanguageType language, lldb::addr_t offset,
+ LazyBool skip_prologue, bool internal, bool hardware) {
   BreakpointSP bp_sp;
   if (num_names > 0) {
 SearchFilterSP filter_sp(GetSearchFilterForModuleAndCUList(
Index: lldb/source/Symbol/SymbolVendor.cpp
===
--- lldb/source/Symbol/SymbolVendor.cpp
+++ lldb/source/Symbol/SymbolVendor.cpp
@@ -288,7 +288,7 @@
 
 size_t SymbolVendor::FindFunctions(const ConstString &name,
c

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

2018-10-23 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53094#1273556, @asmith wrote:

> I think this addresses all the previous comments.


Still didn't get a clear answer if the mutex being used needs to be recursive.  
If it doesn't, perhaps `std::mutex` can be used instead of 
`std::recursive_mutex`?  Not everyone agrees with me, but I often prefer to be 
too strict rather than too relaxed.


https://reviews.llvm.org/D53094



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-24 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: clayborg, labath, jingham.
Herald added subscribers: JDevlieghere, aprantl.

Previously the Module would parse a type name into scope and base name during a 
lookup and only give the SymbolFile plugin the base name, then it would filter 
the results down into the match set based on whether it was supposed to be an 
exact match.

However, if this is supposed to be an exact match, a SymbolFile can do this 
lookup in O(1) time by using its internal accelerator tables, but it can't do 
this unless it actually receives the full name as well as the information that 
it is supposed to be an exact match.  Rather than being too smart, we should 
just let the symbol file plugin be the arbiter to decide how it wants to do 
lookups, so we should pass it the full name.

I'm trying to keep this as NFC for the DWARF plugin, so I took the code that 
was in Module.cpp and tried to rewrite it to be equivalent at the top of 
`SymbolFileDWARF::FindTypes`


https://reviews.llvm.org/D53662

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/lit/SymbolFile/NativePDB/Inputs/tag-types.lldbinit
  lldb/lit/SymbolFile/NativePDB/tag-types.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Symbol/SymbolVendor.cpp
  lldb/tools/lldb-test/lldb-test.cpp

Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -456,8 +456,8 @@
   SymbolContext SC;
   DenseSet SearchedFiles;
   TypeMap Map;
-  Vendor.FindTypes(SC, ConstString(Name), ContextPtr, true, UINT32_MAX,
-   SearchedFiles, Map);
+  Vendor.FindTypes(SC, Name, ContextPtr, false, true, UINT32_MAX, SearchedFiles,
+   Map);
 
   outs() << formatv("Found {0} types:\n", Map.GetSize());
   StreamString Stream;
Index: lldb/source/Symbol/SymbolVendor.cpp
===
--- lldb/source/Symbol/SymbolVendor.cpp
+++ lldb/source/Symbol/SymbolVendor.cpp
@@ -315,17 +315,18 @@
 }
 
 size_t SymbolVendor::FindTypes(
-const SymbolContext &sc, const ConstString &name,
-const CompilerDeclContext *parent_decl_ctx, bool append, size_t max_matches,
+const SymbolContext &sc, llvm::StringRef name,
+const CompilerDeclContext *parent_decl_ctx, bool exact_match, bool append,
+size_t max_matches,
 llvm::DenseSet &searched_symbol_files,
 TypeMap &types) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
 std::lock_guard guard(module_sp->GetMutex());
 if (m_sym_file_ap.get())
-  return m_sym_file_ap->FindTypes(sc, name, parent_decl_ctx, append,
-  max_matches, searched_symbol_files,
-  types);
+  return m_sym_file_ap->FindTypes(sc, name, parent_decl_ctx, exact_match,
+  append, max_matches,
+  searched_symbol_files, types);
   }
   if (!append)
 types.Clear();
Index: lldb/source/Symbol/SymbolFile.cpp
===
--- lldb/source/Symbol/SymbolFile.cpp
+++ lldb/source/Symbol/SymbolFile.cpp
@@ -140,8 +140,8 @@
 }
 
 uint32_t SymbolFile::FindTypes(
-const SymbolContext &sc, const ConstString &name,
-const CompilerDeclContext *parent_decl_ctx, bool append,
+const SymbolContext &sc, llvm::StringRef name,
+const CompilerDeclContext *parent_decl_ctx, bool exact_match, bool append,
 uint32_t max_matches,
 llvm::DenseSet &searched_symbol_files,
 TypeMap &types) {
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
@@ -134,10 +134,9 @@
   std::vector &mangled_names) override;
 
   uint32_t
-  FindTypes(const lldb_private::SymbolContext &sc,
-const lldb_private::ConstString &name,
+  FindTypes(const lldb_private::SymbolContext &sc, llvm::StringRef name,
 const lldb_private::CompilerDeclContext *parent_decl_ctx,
-bool append, uint32_t max_matches,
+bool exact_match, bool append, uint32_t max_matches,
 llvm::DenseSet &searched_symbol_files,
   

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-24 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Note that AFAICT, native PDB plugin is now the only plugin where you can do 
`type lookup -- NS::Struct::Local` and have it return instantly.  On the 
regular PDB plugin it doesn't work at all (returns no results).  On the DWARF 
plugin, I haven't tested, but it will either not work at all, or take a 
potentially long time if you have a lot of debug info).


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-24 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Hi Greg, any thoughts on this?


https://reviews.llvm.org/D53597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-24 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53662#1275238, @jingham wrote:

> I worry that your patch changes the behavior when you add the type_class 
> explicitly in the lookup (i.e. look up "struct Struct" not "Struct".  That 
> should work...
>
> Note, this doesn't currently work in type lookup:
>
>   (lldb) type lookup "struct Foo"
>   no type was found matching '"struct Foo"'
>


Ahh, nice catch.  Luckily it should be easy to add a couple of tests for those 
alongside the other ones I added, so I'll try that and upload a new version.


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support to FileSpec convenience methods.

2018-10-24 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: JDevlieghere.
zturner added a comment.

It seems like FileSpec was moved out of Utility and into Host. I’m not a
fan of this change, because it took a lot of effort to get it into Utility,
which helped break a lot of bad dependencies. Can we invert this dependency
and move FileSpec back into Utility?


https://reviews.llvm.org/D53532



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner updated this revision to Diff 171134.
zturner added a comment.

Fixed issues pointed out by @jingham and added some test coverage for this.


https://reviews.llvm.org/D53662

Files:
  lldb/include/lldb/Core/Module.h
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/include/lldb/Symbol/SymbolVendor.h
  lldb/include/lldb/Symbol/Type.h
  lldb/include/lldb/Symbol/TypeMap.h
  lldb/lit/SymbolFile/NativePDB/Inputs/tag-types.lldbinit
  lldb/lit/SymbolFile/NativePDB/tag-types.cpp
  lldb/source/Core/Module.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/source/Symbol/SymbolFile.cpp
  lldb/source/Symbol/SymbolVendor.cpp
  lldb/source/Symbol/Type.cpp
  lldb/source/Symbol/TypeList.cpp
  lldb/source/Symbol/TypeMap.cpp
  lldb/tools/lldb-test/lldb-test.cpp
  lldb/unittests/Symbol/TestType.cpp

Index: lldb/unittests/Symbol/TestType.cpp
===
--- lldb/unittests/Symbol/TestType.cpp
+++ lldb/unittests/Symbol/TestType.cpp
@@ -21,9 +21,7 @@
const char *expected_scope,
const char *expected_name) {
   llvm::StringRef scope, name;
-  lldb::TypeClass type_class;
-  bool is_scoped =
-  Type::GetTypeScopeAndBasename(full_type, scope, name, type_class);
+  bool is_scoped = Type::GetTypeScopeAndBasename(full_type, scope, name);
   EXPECT_EQ(is_scoped, expected_is_scoped);
   if (expected_is_scoped) {
 EXPECT_EQ(scope, expected_scope);
Index: lldb/tools/lldb-test/lldb-test.cpp
===
--- lldb/tools/lldb-test/lldb-test.cpp
+++ lldb/tools/lldb-test/lldb-test.cpp
@@ -456,8 +456,8 @@
   SymbolContext SC;
   DenseSet SearchedFiles;
   TypeMap Map;
-  Vendor.FindTypes(SC, ConstString(Name), ContextPtr, true, UINT32_MAX,
-   SearchedFiles, Map);
+  Vendor.FindTypes(SC, Name, ContextPtr, false, true, UINT32_MAX, SearchedFiles,
+   Map);
 
   outs() << formatv("Found {0} types:\n", Map.GetSize());
   StreamString Stream;
Index: lldb/source/Symbol/TypeMap.cpp
===
--- lldb/source/Symbol/TypeMap.cpp
+++ lldb/source/Symbol/TypeMap.cpp
@@ -153,18 +153,35 @@
 
 void TypeMap::RemoveMismatchedTypes(const char *qualified_typename,
 bool exact_match) {
+  llvm::StringRef name(qualified_typename);
   llvm::StringRef type_scope;
   llvm::StringRef type_basename;
-  TypeClass type_class = eTypeClassAny;
+  TypeClass type_class = Type::ConsumeTypeClass(name);
   if (!Type::GetTypeScopeAndBasename(qualified_typename, type_scope,
- type_basename, type_class)) {
+ type_basename)) {
 type_basename = qualified_typename;
 type_scope = "";
   }
   return RemoveMismatchedTypes(type_scope, type_basename, type_class,
exact_match);
 }
 
+void TypeMap::OnlyKeepTypeClass(lldb::TypeClass type_class) {
+  if (type_class == eTypeClassAny)
+return;
+
+  collection matching_types;
+
+  iterator pos, end = m_types.end();
+
+  for (const auto &t : m_types) {
+TypeClass tc = t.second->GetForwardCompilerType().GetTypeClass();
+if (tc & type_class)
+  matching_types.insert(t);
+  }
+  m_types.swap(matching_types);
+}
+
 void TypeMap::RemoveMismatchedTypes(const std::string &type_scope,
 const std::string &type_basename,
 TypeClass type_class, bool exact_match) {
@@ -193,8 +210,7 @@
   llvm::StringRef match_type_scope;
   llvm::StringRef match_type_basename;
   if (Type::GetTypeScopeAndBasename(match_type_name, match_type_scope,
-match_type_basename,
-match_type_class)) {
+match_type_basename)) {
 if (match_type_basename == type_basename) {
   const size_t type_scope_size = type_scope.size();
   const size_t match_type_scope_size = match_type_scope.size();
Index: lldb/source/Symbol/TypeList.cpp
===
--- lldb/source/Symbol/TypeList.cpp
+++ lldb/source/Symbol/TypeList.cpp
@@ -112,9 +112,10 @@
  bool exact_match) {
   llvm::StringRef type_scope;
   llvm::StringRef type_basename;
-  TypeClass type_class = eTypeClassAny;
+  llvm::StringRef name(qualified_typ

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53597#1276086, @clayborg wrote:

> As long as Swig is happy and the ABI doesn't change I am ok with this. Will 
> we see the variables better when debugging? Or is this solely so the 
> SymbolContextItem type doesn't disappear from the debug info?


We will see variables better when debugging too.


https://reviews.llvm.org/D53597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53597#1276110, @zturner wrote:

> In https://reviews.llvm.org/D53597#1276086, @clayborg wrote:
>
> > As long as Swig is happy and the ABI doesn't change I am ok with this. Will 
> > we see the variables better when debugging? Or is this solely so the 
> > SymbolContextItem type doesn't disappear from the debug info?
>
>
> We will see variables better when debugging too.


(That was actually my primary motivation)


https://reviews.llvm.org/D53597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53597#1276145, @jingham wrote:

> So far as I can tell, this patch will make lookup of exact types faster for 
> PDB, but because of the way DWARF debug_names tables are constructed, I don't 
> think there's any way we can do the same thing for DWARF.
>
> But unless I'm misunderstanding the patch, this doesn't change correctness of 
> the lookup (except for fixing "type lookup 'struct Foo'").  Did I miss 
> something?
>
> Jim


That's the other patch.  This patch is NFC and just makes debugging nicer 
because you can see enum values in your debugger as rich enumerators.  But for 
the other patch, if what you said is correct, then I suppose that's correct.  I 
asked Eric Christopher and he said he thought (but wasn't 100% sure) that types 
were hashed in the accelerator tables by their full name and not their base 
name.  If that is true then it could make exact matches faster.  But if it's 
incorrect then yes, it wouldn't be able to make exact matches faster in DWARF.


https://reviews.llvm.org/D53597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: jingham.
zturner added a comment.

I guess the question is, How is that hash and the bucket computed?  If it's
based on the full name, then you should be able to get fast exact lookup.
If it's based on the based name, then it will indeed be slow.


https://reviews.llvm.org/D53597



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support to FileSpec convenience methods.

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

I envision `FileSpec` as being more of a `PathSpec`.  It should be able 
manipulate paths as strings and nothing more.  There is indeed some logic that 
still remains that resolves paths, but it manages to do this using LLVM's file 
system APIs and the only reason it's still there is because it was baked in and 
a bit hard to remove.

One idea for removing it would be to have `FileSpec FileSystem::Resolve(const 
FileSpec &)`.  Then instead of saying `FileSpec foo(path, /*resolve = */ 
true);`, they could say `FileSpec foo = FileSystem::Resolve(FileSpec(path));` 
or something.


https://reviews.llvm.org/D53532



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53532: [FileSpec] Add VFS support to FileSpec convenience methods.

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53532#1276463, @JDevlieghere wrote:

> Thanks for the feedback! I totally agree it's a good solution and it was one 
> of the things I considered. It didn't make it to the top of the list because 
> it is very intrusive and changes the semantics of FileSpec quite a bit (i.e. 
> it becomes a PathSpec as Zachary noted). Anyway, I'm happy to do this and 
> I'll split this up in two patches as Pavel suggested. I'll extend the 
> FileSystem first, so that we can do the VFS stuff straight away instead of 
> implementing these methods twice.


It does change them, but that's always been the sort of "end goal" of 
`FileSpec` anyway, it just hasn't been reached yet.  Hopefully we can get there 
with your help :)


https://reviews.llvm.org/D53532



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53590: Refactor SetBaseClassesForType and DeleteBaseClasses to be more C++'y

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB345312: [NFC] Refactor SetBaseClasses and 
DeleteBaseClasses. (authored by zturner, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D53590?vs=170700&id=171185#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53590

Files:
  include/lldb/Symbol/ClangASTContext.h
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h
  source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
  source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.h
  source/Plugins/SymbolFile/PDB/PDBASTParser.cpp
  source/Symbol/ClangASTContext.cpp
  unittests/Symbol/TestClangASTContext.cpp

Index: unittests/Symbol/TestClangASTContext.cpp
===
--- unittests/Symbol/TestClangASTContext.cpp
+++ unittests/Symbol/TestClangASTContext.cpp
@@ -337,15 +337,19 @@
   EXPECT_NE(nullptr, non_empty_base_field_decl);
   EXPECT_TRUE(ClangASTContext::RecordHasFields(non_empty_base_decl));
 
+  std::vector> bases;
+
   // Test that a record with no direct fields, but fields in a base returns true
   CompilerType empty_derived = m_ast->CreateRecordType(
   nullptr, lldb::eAccessPublic, "EmptyDerived", clang::TTK_Struct,
   lldb::eLanguageTypeC_plus_plus, nullptr);
   ClangASTContext::StartTagDeclarationDefinition(empty_derived);
-  CXXBaseSpecifier *non_empty_base_spec = m_ast->CreateBaseClassSpecifier(
-  non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, false, false);
-  bool result = m_ast->SetBaseClassesForClassType(
-  empty_derived.GetOpaqueQualType(), &non_empty_base_spec, 1);
+  std::unique_ptr non_empty_base_spec =
+  m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(),
+  lldb::eAccessPublic, false, false);
+  bases.push_back(std::move(non_empty_base_spec));
+  bool result = m_ast->TransferBaseClasses(empty_derived.GetOpaqueQualType(),
+   std::move(bases));
   ClangASTContext::CompleteTagDeclarationDefinition(empty_derived);
   EXPECT_TRUE(result);
   CXXRecordDecl *empty_derived_non_empty_base_cxx_decl =
@@ -363,10 +367,12 @@
   nullptr, lldb::eAccessPublic, "EmptyDerived2", clang::TTK_Struct,
   lldb::eLanguageTypeC_plus_plus, nullptr);
   ClangASTContext::StartTagDeclarationDefinition(empty_derived2);
-  CXXBaseSpecifier *non_empty_vbase_spec = m_ast->CreateBaseClassSpecifier(
-  non_empty_base.GetOpaqueQualType(), lldb::eAccessPublic, true, false);
-  result = m_ast->SetBaseClassesForClassType(empty_derived2.GetOpaqueQualType(),
- &non_empty_vbase_spec, 1);
+  std::unique_ptr non_empty_vbase_spec =
+  m_ast->CreateBaseClassSpecifier(non_empty_base.GetOpaqueQualType(),
+  lldb::eAccessPublic, true, false);
+  bases.push_back(std::move(non_empty_vbase_spec));
+  result = m_ast->TransferBaseClasses(empty_derived2.GetOpaqueQualType(),
+  std::move(bases));
   ClangASTContext::CompleteTagDeclarationDefinition(empty_derived2);
   EXPECT_TRUE(result);
   CXXRecordDecl *empty_derived_non_empty_vbase_cxx_decl =
@@ -377,9 +383,6 @@
empty_derived_non_empty_vbase_cxx_decl, false));
   EXPECT_TRUE(
   ClangASTContext::RecordHasFields(empty_derived_non_empty_vbase_decl));
-
-  delete non_empty_base_spec;
-  delete non_empty_vbase_spec;
 }
 
 TEST_F(TestClangASTContext, TemplateArguments) {
Index: source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
===
--- source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
+++ source/Plugins/SymbolFile/NativePDB/UdtRecordCompleter.cpp
@@ -59,12 +59,12 @@
   CVType udt_cvt = m_symbol_file.m_index->tpi().getType(ti);
 
   lldb::opaque_compiler_type_t base_qt = base_ct.GetOpaqueQualType();
-  clang::CXXBaseSpecifier *base_spec =
+  std::unique_ptr base_spec =
   m_symbol_file.GetASTContext().CreateBaseClassSpecifier(
   base_qt, TranslateMemberAccess(access), false,
   udt_cvt.kind() == LF_CLASS);
-
-  m_bases.push_back(base_spec);
+  lldbassert(base_spec);
+  m_bases.push_back(std::move(base_spec));
   return base_qt;
 }
 
@@ -172,9 +172,8 @@
 
 void UdtRecordCompleter::complete() {
   ClangASTContext &clang = m_symbol_file.GetASTContext();
-  clang.SetBaseClassesForClassType(m_derived_ct.GetOpaqueQualType(),
-   m_bases.data(), m_bases.size());
-  ClangASTContext::DeleteBaseClassSpecifiers(m_bases.data(), m_bases.size());
+  clang.TransferBaseClasses(m_derived_ct.GetOpaqueQualType(),
+std::move(m_bases));
 
   clang.AddMethodOverridesForCXXRecordType(m_derived_ct.GetOpaqueQual

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB345313: Don't type-erase the SymbolContextItem 
enumeration. (authored by zturner, committed by ).
Herald added a subscriber: abidh.

Changed prior to commit:
  https://reviews.llvm.org/D53597?vs=170737&id=171187#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53597

Files:
  include/lldb/Core/Address.h
  include/lldb/Core/Module.h
  include/lldb/Core/ModuleList.h
  include/lldb/Symbol/CompileUnit.h
  include/lldb/Symbol/SymbolFile.h
  include/lldb/Symbol/SymbolVendor.h
  include/lldb/Target/StackFrame.h
  include/lldb/lldb-enumerations.h
  source/API/SBAddress.cpp
  source/API/SBFrame.cpp
  source/API/SBModule.cpp
  source/API/SBTarget.cpp
  source/Commands/CommandObjectSource.cpp
  source/Core/Address.cpp
  source/Core/Disassembler.cpp
  source/Core/Module.cpp
  source/Core/ModuleList.cpp
  source/Core/SourceManager.cpp
  source/Plugins/Architecture/Mips/ArchitectureMips.cpp
  source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  
source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  source/Plugins/Process/Utility/RegisterContextLLDB.cpp
  source/Plugins/Process/Utility/UnwindMacOSXFrameBackchain.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  source/Symbol/CompileUnit.cpp
  source/Symbol/SymbolFile.cpp
  source/Symbol/SymbolVendor.cpp
  source/Target/StackFrame.cpp

Index: source/API/SBModule.cpp
===
--- source/API/SBModule.cpp
+++ source/API/SBModule.cpp
@@ -217,9 +217,9 @@
  uint32_t resolve_scope) {
   SBSymbolContext sb_sc;
   ModuleSP module_sp(GetSP());
+  SymbolContextItem scope = static_cast(resolve_scope);
   if (module_sp && addr.IsValid())
-module_sp->ResolveSymbolContextForAddress(addr.ref(), resolve_scope,
-  *sb_sc);
+module_sp->ResolveSymbolContextForAddress(addr.ref(), scope, *sb_sc);
   return sb_sc;
 }
 
Index: source/API/SBTarget.cpp
===
--- source/API/SBTarget.cpp
+++ source/API/SBTarget.cpp
@@ -660,11 +660,12 @@
 SBTarget::ResolveSymbolContextForAddress(const SBAddress &addr,
  uint32_t resolve_scope) {
   SBSymbolContext sc;
+  SymbolContextItem scope = static_cast(resolve_scope);
   if (addr.IsValid()) {
 TargetSP target_sp(GetSP());
 if (target_sp)
-  target_sp->GetImages().ResolveSymbolContextForAddress(
-  addr.ref(), resolve_scope, sc.ref());
+  target_sp->GetImages().ResolveSymbolContextForAddress(addr.ref(), scope,
+sc.ref());
   }
   return sc;
 }
Index: source/API/SBAddress.cpp
===
--- source/API/SBAddress.cpp
+++ source/API/SBAddress.cpp
@@ -198,8 +198,9 @@
 
 SBSymbolContext SBAddress::GetSymbolContext(uint32_t resolve_scope) {
   SBSymbolContext sb_sc;
+  SymbolContextItem scope = static_cast(resolve_scope);
   if (m_opaque_ap->IsValid())
-m_opaque_ap->CalculateSymbolContext(&sb_sc.ref(), resolve_scope);
+m_opaque_ap->CalculateSymbolContext(&sb_sc.ref(), scope);
   return sb_sc;
 }
 
Index: source/API/SBFrame.cpp
===
--- source/API/SBFrame.cpp
+++ source/API/SBFrame.cpp
@@ -112,16 +112,16 @@
   SBSymbolContext sb_sym_ctx;
   std::unique_lock lock;
   ExecutionContext exe_ctx(m_opaque_sp.get(), lock);
-
+  SymbolContextItem scope = static_cast(resolve_scope);
   StackFrame *frame = nullptr;
   Target *target = exe_ctx.GetTargetPtr();
   Process *process = exe_ctx.GetProcessPtr();
   if (target && process) {
 Process::StopLocker stop_locker;
 if (stop_locker.TryLock(&process->GetRunLock())) {
   frame = exe_ctx.GetFramePtr();
   if (frame) {
-sb_sym_ctx.SetSymbolContext(&frame->GetSymbolContext(resolve_scope));
+sb_sym_ctx.SetSymbolContext(&frame->GetSymbolContext(scope));
   } else {
 if (log)
   log->Printf("SBFrame::GetVariables () => error: could not "
Index: source/Core/SourceManager.cpp
===
--- source/Core/SourceManager.cpp
+++ source/Core/SourceManager

[Lldb-commits] [PATCH] D53597: Don't type-erase the SymbolContextItem enum

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345313: Don't type-erase the SymbolContextItem 
enumeration. (authored by zturner, committed by ).
Herald added subscribers: llvm-commits, jrtc27.

Changed prior to commit:
  https://reviews.llvm.org/D53597?vs=170737&id=171186#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53597

Files:
  lldb/trunk/include/lldb/Core/Address.h
  lldb/trunk/include/lldb/Core/Module.h
  lldb/trunk/include/lldb/Core/ModuleList.h
  lldb/trunk/include/lldb/Symbol/CompileUnit.h
  lldb/trunk/include/lldb/Symbol/SymbolFile.h
  lldb/trunk/include/lldb/Symbol/SymbolVendor.h
  lldb/trunk/include/lldb/Target/StackFrame.h
  lldb/trunk/include/lldb/lldb-enumerations.h
  lldb/trunk/source/API/SBAddress.cpp
  lldb/trunk/source/API/SBFrame.cpp
  lldb/trunk/source/API/SBModule.cpp
  lldb/trunk/source/API/SBTarget.cpp
  lldb/trunk/source/Commands/CommandObjectSource.cpp
  lldb/trunk/source/Core/Address.cpp
  lldb/trunk/source/Core/Disassembler.cpp
  lldb/trunk/source/Core/Module.cpp
  lldb/trunk/source/Core/ModuleList.cpp
  lldb/trunk/source/Core/SourceManager.cpp
  lldb/trunk/source/Plugins/Architecture/Mips/ArchitectureMips.cpp
  lldb/trunk/source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp
  
lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.cpp
  lldb/trunk/source/Plugins/Process/Utility/RegisterContextLLDB.cpp
  lldb/trunk/source/Plugins/Process/Utility/UnwindMacOSXFrameBackchain.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/trunk/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/trunk/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/trunk/source/Symbol/CompileUnit.cpp
  lldb/trunk/source/Symbol/SymbolFile.cpp
  lldb/trunk/source/Symbol/SymbolVendor.cpp
  lldb/trunk/source/Target/StackFrame.cpp

Index: lldb/trunk/source/Symbol/CompileUnit.cpp
===
--- lldb/trunk/source/Symbol/CompileUnit.cpp
+++ lldb/trunk/source/Symbol/CompileUnit.cpp
@@ -278,7 +278,8 @@
 
 uint32_t CompileUnit::ResolveSymbolContext(const FileSpec &file_spec,
uint32_t line, bool check_inlines,
-   bool exact, uint32_t resolve_scope,
+   bool exact,
+   SymbolContextItem resolve_scope,
SymbolContextList &sc_list) {
   // First find all of the file indexes that match our "file_spec". If
   // "file_spec" has an empty directory, then only compare the basenames when
Index: lldb/trunk/source/Symbol/SymbolVendor.cpp
===
--- lldb/trunk/source/Symbol/SymbolVendor.cpp
+++ lldb/trunk/source/Symbol/SymbolVendor.cpp
@@ -235,7 +235,7 @@
 }
 
 uint32_t SymbolVendor::ResolveSymbolContext(const Address &so_addr,
-uint32_t resolve_scope,
+SymbolContextItem resolve_scope,
 SymbolContext &sc) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
@@ -248,7 +248,7 @@
 
 uint32_t SymbolVendor::ResolveSymbolContext(const FileSpec &file_spec,
 uint32_t line, bool check_inlines,
-uint32_t resolve_scope,
+SymbolContextItem resolve_scope,
 SymbolContextList &sc_list) {
   ModuleSP module_sp(GetModule());
   if (module_sp) {
Index: lldb/trunk/source/Symbol/SymbolFile.cpp
===
--- lldb/trunk/source/Symbol/SymbolFile.cpp
+++ lldb/trunk/source/Symbol/SymbolFile.cpp
@@ -97,7 +97,7 @@
 
 uint32_t SymbolFile::ResolveSymbolContext(const FileSpec &file_spec,
   uint32_t line, bool check_inlines,
-  uint32_t resolve_scope,
+  lldb::SymbolContextItem resolve_scope,
   SymbolContextList &sc_list) {
   return 0;
 }
Index: lldb/trunk/source/Plugins/LanguageRuntime/RenderScript/RenderScriptRuntime/RenderScriptRuntime.

[Lldb-commits] [PATCH] D53616: Don't type-erase the FunctionNameType or TypeClass enums

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345314: Don't type-erase the FunctionNameType or 
TypeClass enums. (authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53616?vs=170762&id=171188#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53616

Files:
  lldb/trunk/include/lldb/Breakpoint/BreakpointResolverName.h
  lldb/trunk/include/lldb/Core/Module.h
  lldb/trunk/include/lldb/Core/ModuleList.h
  lldb/trunk/include/lldb/Symbol/SymbolFile.h
  lldb/trunk/include/lldb/Symbol/SymbolVendor.h
  lldb/trunk/include/lldb/Target/Target.h
  lldb/trunk/include/lldb/lldb-enumerations.h
  lldb/trunk/source/API/SBCompileUnit.cpp
  lldb/trunk/source/API/SBModule.cpp
  lldb/trunk/source/API/SBTarget.cpp
  lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
  lldb/trunk/source/Commands/CommandObjectBreakpoint.cpp
  lldb/trunk/source/Core/Module.cpp
  lldb/trunk/source/Core/ModuleList.cpp
  lldb/trunk/source/Expression/IRExecutionUnit.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/trunk/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
  lldb/trunk/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.cpp
  lldb/trunk/source/Plugins/SymbolFile/Symtab/SymbolFileSymtab.h
  lldb/trunk/source/Symbol/SymbolFile.cpp
  lldb/trunk/source/Symbol/SymbolVendor.cpp
  lldb/trunk/source/Target/Target.cpp

Index: lldb/trunk/source/Expression/IRExecutionUnit.cpp
===
--- lldb/trunk/source/Expression/IRExecutionUnit.cpp
+++ lldb/trunk/source/Expression/IRExecutionUnit.cpp
@@ -709,9 +709,10 @@
 
 struct IRExecutionUnit::SearchSpec {
   ConstString name;
-  uint32_t mask;
+  lldb::FunctionNameType mask;
 
-  SearchSpec(ConstString n, uint32_t m = lldb::eFunctionNameTypeFull)
+  SearchSpec(ConstString n,
+ lldb::FunctionNameType m = lldb::eFunctionNameTypeFull)
   : name(n), mask(m) {}
 };
 
Index: lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
===
--- lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
+++ lldb/trunk/source/Breakpoint/BreakpointResolverName.cpp
@@ -30,7 +30,7 @@
 using namespace lldb_private;
 
 BreakpointResolverName::BreakpointResolverName(
-Breakpoint *bkpt, const char *name_cstr, uint32_t name_type_mask,
+Breakpoint *bkpt, const char *name_cstr, FunctionNameType name_type_mask,
 LanguageType language, Breakpoint::MatchType type, lldb::addr_t offset,
 bool skip_prologue)
 : BreakpointResolver(bkpt, BreakpointResolver::NameResolver, offset),
@@ -51,7 +51,7 @@
 
 BreakpointResolverName::BreakpointResolverName(
 Breakpoint *bkpt, const char *names[], size_t num_names,
-uint32_t name_type_mask, LanguageType language, lldb::addr_t offset,
+FunctionNameType name_type_mask, LanguageType language, lldb::addr_t offset,
 bool skip_prologue)
 : BreakpointResolver(bkpt, BreakpointResolver::NameResolver, offset),
   m_match_type(Breakpoint::Exact), m_language(language),
@@ -61,9 +61,12 @@
   }
 }
 
-BreakpointResolverName::BreakpointResolverName(
-Breakpoint *bkpt, std::vector names, uint32_t name_type_mask,
-LanguageType language, lldb::addr_t offset, bool skip_prologue)
+BreakpointResolverName::BreakpointResolverName(Breakpoint *bkpt,
+   std::vector names,
+   FunctionNameType name_type_mask,
+   LanguageType language,
+   lldb::addr_t offset,
+   bool skip_prologue)
 : BreakpointResolver(bkpt, BreakpointResolver::NameResolver, offset),
   m_match_type(Breakpoint::Exact), m_language(language),
   m_skip_prologue(skip_prologue) {
@@ -161,23 +164,23 @@
   return nullptr;
 }
 std::vector names;
-std::vector name_masks;
+std::vector name_masks;
 for (size_t i = 0; i < num_elem; i++) {
-  uint32_t name_mask;
   llvm::StringRef name;
 
   success = names_array->GetItemAtIndexAsString(i, name);
   if (!success) {
 error.SetErrorString("BRN::CFSD: name entry is not a string.");
 return nullptr;
   }
-  success = names_mask_array->GetItemAtIndexAsInteger(i, name_mask);
+  std::underlying_type::type fnt

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: lemo, aleksandr.urakov, stella.stamenova.

LLDB has the ability to display global variables, even without a running 
process, via the `target variable` command.  This is because global variables 
are linker initialized, so their values are embedded directly into the 
executables.  This gives us great power for testing native PDB functionality in 
a cross-platform manner, because we don't actually need a running process.  We 
can just create a target using an EXE file, and display global variables.  And 
global variables can have arbitrarily complex types, so in theory we can fully 
exercise the type system, record layout, and data formatters for native PDB 
files and PE/COFF executables on any host platform, as long as our type does 
not require a dynamic initializer.

This patch adds basic support for finding variables by name, and adds an 
exhaustive test for fundamental data types and pointers / references to 
fundamental data types.

Subsequent patches will extend this to typedefs, classes, pointers to 
functions, and other cases.


https://reviews.llvm.org/D53731

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/globals-fundamental.lldbinit
  lldb/lit/SymbolFile/NativePDB/globals-fundamental.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -103,6 +103,11 @@
 
   size_t ParseFunctionBlocks(const SymbolContext &sc) override;
 
+  uint32_t FindGlobalVariables(const ConstString &name,
+   const CompilerDeclContext *parent_decl_ctx,
+   uint32_t max_matches,
+   VariableList &variables) override;
+
   size_t ParseTypes(const SymbolContext &sc) override;
   size_t ParseVariablesForContext(const SymbolContext &sc) override {
 return 0;
@@ -175,11 +180,13 @@
   lldb::CompUnitSP GetOrCreateCompileUnit(const CompilandIndexItem &cci);
   lldb::TypeSP GetOrCreateType(PdbSymUid type_uid);
   lldb::TypeSP GetOrCreateType(llvm::codeview::TypeIndex ti);
+  lldb::VariableSP GetOrCreateGlobalVariable(PdbSymUid var_uid);
 
   lldb::FunctionSP CreateFunction(PdbSymUid func_uid, const SymbolContext &sc);
   lldb::CompUnitSP CreateCompileUnit(const CompilandIndexItem &cci);
   lldb::TypeSP CreateType(PdbSymUid type_uid);
   lldb::TypeSP CreateAndCacheType(PdbSymUid type_uid);
+  lldb::VariableSP CreateGlobalVariable(PdbSymUid var_uid);
 
   llvm::BumpPtrAllocator m_allocator;
 
@@ -193,6 +200,7 @@
 
   llvm::DenseMap m_uid_to_decl;
 
+  llvm::DenseMap m_global_vars;
   llvm::DenseMap m_functions;
   llvm::DenseMap m_compilands;
   llvm::DenseMap m_types;
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -16,14 +16,17 @@
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/StreamBuffer.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Symbol/ClangExternalASTSourceCommon.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolVendor.h"
+#include "lldb/Symbol/Variable.h"
+#include "lldb/Symbol/VariableList.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -886,6 +889,128 @@
   return CreateAndCacheType(type_uid);
 }
 
+static DWARFExpression MakeGlobalLocationExpression(uint16_t section,
+uint32_t offset,
+ModuleSP module) {
+  if (!module)
+return DWARFExpression(nullptr);
+
+  const ArchSpec &architecture = module->GetArchitecture();
+  ByteOrder byte_order = architecture.GetByteOrder();
+  uint32_t address_size = architecture.GetAddressByteSize();
+  uint32_t byte_size = architecture.GetDataByteSize();
+  if (byte_order == eByteOrderInvalid || address_size == 0)
+return DWARFExpression(nullptr);
+
+  RegisterKind register_kind = eRegisterKindDWARF;
+  StreamBuffer<32> stream(Stream::eBinary, address_size, byte_order);
+  stream.PutHex8(DW_OP_addr);
+
+  SectionList *section_list = module->GetSectionList();
+  if (!section_list)
+return DWARFExpression(nullptr);
+
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())
+return DWA

[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53662#1276253, @jingham wrote:

> So the current approach relies on the ability to sniff the name to determine 
> the context in which the user intends to find it.  It does (and always did) 
> use the presence of an initial "::" to tell whether a symbol is exact.  
> That's obviously also inappropriate for a generic Type method.  But OTOH, 
> there needs to be a funnel point where you take in a string you know nothing 
> about (from the user either in type lookup or in an expression) and find it 
> as best you can.  I don't think we want to force command line users to say 
> "type lookup --exact " so we've got to figure it out internally.
>
> Internally, it might be helpful to do some initial analysis of the input type 
> string, and then dispatch it to an exact or inexact search.  But given I 
> don't think you can get away without a FindTypes that takes a string that you 
> don't know whether it is exact or not, I don't feel strongly about that.
>
> We should abstract "is exact" and ask the various type systems to handle that 
> request, and we also need to abstract "remove type class" and again ask the 
> various type systems to handle that.  That seems to me the main ugliness that 
> this patch reveals.


Just to clarify, is the consensus then that I can submit this as is?  Or that 
it needs some modification?  Greg's suggestion of making a separate method 
called `FindExactType` could work, but it doesn't seem that much different than 
passing a boolean call `exact_match`, which is what I've done here.  On the 
extreme end, we could just make `Module::FindTypes` do absolutely nothing 
except call the SymbolFile plugin.I don't feel too strongly either way.  
The current patch seems NFC for DWARF and strictly better for PDB, but I'm 
willing to make changes if anyone feels like there's an architecturally more 
sound approach.


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

which generic SymbolFile test do you mean?  The lit ones are the only ones that 
are set up to run in this particular manner (run lines, check lines, etc), and 
currently we don't have a way to run different / multiple command line 
invocations.  I came up with this test in order to test the new changes I 
introduced in `SymbolFileNativePdb.cpp` that are also in this patch, so I 
definitely want to make sure all of that code still gets exercised with 
whatever test strategy I end up using.

I guess the way I see it, there's two interesting things this test could 
exercise.  1) the debug info, and 2) the formatters.  If we want to test the 
formatters, we could probably brainstorm a way to do it generically with 1 test 
(or set of tests) that omits the SymbolFile from the picture entirely and is 
generic enough that it should be applicable to any platform/compiler/host.  If 
we want to test the SymbolFile plugin though, then it may need to be specific 
to the debug info type.

I think what you're getting at though is that we probably need some notion of 
"debug info variants" for these lit tests like we have in the dotest suite.  I 
actually had an idea for something that might work like that a while back, 
which I described here: https://reviews.llvm.org/D52618#1252906

But the idea would basically be that instead of hardcoding a command line like 
I've done with `clang-cl /Z7 etc etc` we could write something more generic 
that would evaluate to different (or perhaps even multiple) things, so we could 
run it different ways.


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53731#1276633, @jingham wrote:

> Well, what's really going on is that I'm not familiar enough with lit to know 
> that it doesn't have the ability to run different commands to produce the 
> input file...  But as you guessed, my point is that you have written a bunch 
> of tests that would be valuable to test against any symfile format, so it is 
> a shame to only run them against one format.  OTOH, if that's not possible 
> right now, I'm content to wait for some enhancements to lit that make it 
> possible.


I can maybe hardcode multiple runlines, one for each possible target triple.  
But what's really going on is that I'm not familiar enough with other platforms 
to know how to generate their binaries :)

But I'll actually give it a try.  Just to explain what's going on here, I've 
got these two lines:

  // RUN: clang-cl /Z7 /GS- /GR- /c /Fo%t.obj -- %s 
  // RUN: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb -- 
%t.obj

Those two run the compiler and linker and write the output to a temporary file, 
but don't actually check anything yet.  The next two lines (which are actually 
one command broken with a continuation character)

  // RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
  // RUN: %p/Inputs/globals-fundamental.lldbinit | FileCheck %s

runs lldb and pipes the result to FileCheck, using this file as the check file.

So what I could *try* doing is having multiple compiler / linker invocations.  
One that produces a Windows executable / PDB file.  One that produces a Linux 
executable / DWARF.  One that produces some kind of OSX binary (I think lld 
doesn't work with MachO yet, so I'm not sure about this one), writing the 
linker outputs to different files each time.

Then we could maybe run lldb 3 times, once against each input, but using the 
same check file each time.  I'll give it a try and see what happens.


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added reviewers: vsk, labath.
zturner added a comment.

In https://reviews.llvm.org/D53731#1276660, @jingham wrote:

> Could you also use Vedant's new FileCheck dotest test class?  That should 
> allow you to write the tests exactly as you are, but use the dotest mechanism 
> to build and run the example.


Adding Vedant here.  My understanding is that the work he did there is like the 
inverse of what I'm doing.  It allows you to use FileCheck from inside of 
dotest tests, but I was trying to bypass dotest here.  I don't necessarily 
think "dotest for all things" should be an explicit goal (i actually think long 
term we should move away from it, but that's for another day).  Aside from that 
though, I don't think this particular test needs any of the functionality that 
dotest offers.  It offers building in multiple configurations, but we can get 
that from this test by just specifying mulitple run lines (I'm testing this out 
locally and it seems like I can get it to work).  Eventually, using some kind 
of configuration / builder type system like I brainstormed in the review thread 
I linked previously, I think we could have all the advantages of dotest's 
builder even with this style of test.

FWIW, I was also under the impression that Vedant's solution with FileCheck in 
dotest was only intended as sort of an immediate solution to a problem, but not 
necessary the long term desired end-state. (I could be wrong about this though).


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53731#1276743, @vsk wrote:

> In https://reviews.llvm.org/D53731#1276732, @zturner wrote:
>
> > In https://reviews.llvm.org/D53731#1276660, @jingham wrote:
> >
> > > Could you also use Vedant's new FileCheck dotest test class?  That should 
> > > allow you to write the tests exactly as you are, but use the dotest 
> > > mechanism to build and run the example.
> >
> >
> > Adding Vedant here.  My understanding is that the work he did there is like 
> > the inverse of what I'm doing.  It allows you to use FileCheck from inside 
> > of dotest tests, but I was trying to bypass dotest here.  I don't 
> > necessarily think "dotest for all things" should be an explicit goal (i 
> > actually think long term we should move away from it, but that's for 
> > another day).  Aside from that though, I don't think this particular test 
> > needs any of the functionality that dotest offers.  It offers building in 
> > multiple configurations, but we can get that from this test by just 
> > specifying mulitple run lines (I'm testing this out locally and it seems 
> > like I can get it to work).  Eventually, using some kind of configuration / 
> > builder type system like I brainstormed in the review thread I linked 
> > previously, I think we could have all the advantages of dotest's builder 
> > even with this style of test.
> >
> > FWIW, I was also under the impression that Vedant's solution with FileCheck 
> > in dotest was only intended as sort of an immediate solution to a problem, 
> > but not necessary the long term desired end-state. (I could be wrong about 
> > this though).
>
>
> The tests as-written seem to exercise the new functionality. I think it'd be 
> fine to use the FileCheck-in-{dotest,lldbinline} support if you need to set a 
> breakpoint, run a command, and then validate its output (though it looks like 
> you don't)


Yea, once I need to actually run a process I expect I'll need to start using 
it.  For the time being, I'm trying see how much ground I can cover without a 
process.  The reasoning is two-fold.

First, it means the tests can run anywhere.  If there's no process, my test 
coverage isn't limited to a specific host OS.

Second, from a layering perspective, if I know that there's a ton of test 
coverage for "everything that happens before a process is spawned", then once 
you get to the point that you are spawning process, it limits the scope of 
where you have to search when something does go wrong.

So that's my thinking.


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-25 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53731#1276818, @jingham wrote:

> dotest tests don't require a process.  Presumably dotest knows how to build 
> windows targeted PDB debug flavor files (to go along with dwarf/dsym/etc.).  
> So it would be straightforward to make a test that had your input sources, 
> built and made a target out of it and then poked at static variables and 
> their types.  That would straightaway run with all the different symbol file 
> formats we support.
>
> That was why using Vedant's FileCheck thing made sense to me.  You would use 
> that to specify the test cases, since you like that way of writing tests and 
> anyway already have them written out in that form, but use the dotest 
> machinery to build it for whatever symfile format and target architecture/s 
> the person who was running the test dialed up.  But if you are interesting in 
> also getting this to work with the straight FileCheck style test, your time 
> is your own...
>
> BTW, I would use dotest tests specifically for the kind of thing you are 
> doing here because then you can test against the SBType and SBTypeMembers 
> from the debug info you've ingested, which would give you bit and byte 
> offsets and sizes for free.  But if your differing tastes end up getting you 
> to add that info to "type lookup" - which we really should do - then I guess 
> we win either way...


Yea, so one of the commands I'm used to on the Windows command line debugger 
WinDbg is the `dt` (dump type) command.  It's basically the equivalent of `type 
lookup` - more powerful in some ways, less in others .  The output format looks 
like this:

  0:000> dt _EXCEPTION_RECORD
  ntdll!_EXCEPTION_RECORD
 +0x000 ExceptionCode: Int4B
 +0x004 ExceptionFlags   : Uint4B
 +0x008 ExceptionRecord  : Ptr64 _EXCEPTION_RECORD
 +0x010 ExceptionAddress : Ptr64 Void
 +0x018 NumberParameters : Uint4B
 +0x020 ExceptionInformation : [15] Uint8B

So you can see the field offsets and such.  (Less powerful because that's about 
it, no methods, for example).

But you can also pass it an address, which it then formats the block of memory 
as that type.  e.g.

  0:000> dt ntdll!_TEB32 007ed000
 +0x000 NtTib: _NT_TIB32
 +0x01c EnvironmentPointer : 0
 +0x020 ClientId : _CLIENT_ID32
 +0x028 ActiveRpcHandle  : 0
 +0x02c ThreadLocalStoragePointer : 0
 +0x030 ProcessEnvironmentBlock : 0x7ed000
 +0x034 LastErrorValue   : 0
 +0x038 CountOfOwnedCriticalSections : 0
 +0x03c CsrClientThread  : 0
 +0x040 Win32ThreadInfo  : 0x4d18
 +0x044 User32Reserved   : [26] 0
 +0x0ac UserReserved : [5] 0

So up on my list of things to implement is these two features.


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:46
+   // due to the debug magic at the beginning of the 
stream.
+  uint64_t global : 1; // True if this is from the globals stream.
+  uint64_t modi : 16;  // For non-global, this is the 0-based index of module.

lemo wrote:
> 30 + 1 != 32 - what's going on?
There's supposed to be a `uint64_t unused : 1;` here.  Thanks for noticing.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:913
+
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())

lemo wrote:
> comment explaining the - 1 ?
I'm going to be totally honest here.  I don't understand this code at all.  I 
copied it from the `SymbolFilePDB` implementation.  It seems to work :-/



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:936
+  lldb::ValueType scope = eValueTypeInvalid;
+  TypeIndex ti;
+  llvm::StringRef name;

lemo wrote:
> pls explicitly initialize all the locals
`TypeIndex` has a constructor, but I should definitely initialize the 
`lldb::addr_t`



Comment at: 
lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1406
+  TypeSP type_sp = CreateAndCacheType(uid);
+  return &*type_sp;
 }

lemo wrote:
> type_sp->get() seems cleaner / more readable
You know, I kept trying to write that, and I was like "what silly person on the 
C++ standards committee forgot to put a `get()` method on `std::shared_ptr<>`.  
Did they really forget this or am I just going crazy?"  Turns out it's a bug in 
MSVC intellisense where it doesn't show it for some reason.  So I took that to 
mean it didn't exist.  Shame on me.


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:913
+
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())

zturner wrote:
> lemo wrote:
> > comment explaining the - 1 ?
> I'm going to be totally honest here.  I don't understand this code at all.  I 
> copied it from the `SymbolFilePDB` implementation.  It seems to work :-/
Ok, I thought about this some, it's actually pretty simple.  I'll add a comment.


https://reviews.llvm.org/D53731



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345373: [NativePDB] Add the ability to dump dump global 
variables. (authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53731?vs=171189&id=171268#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53731

Files:
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/globals-fundamental.lldbinit
  lldb/trunk/lit/SymbolFile/NativePDB/globals-fundamental.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -16,14 +16,17 @@
 
 #include "lldb/Core/Module.h"
 #include "lldb/Core/PluginManager.h"
+#include "lldb/Core/StreamBuffer.h"
 #include "lldb/Symbol/ClangASTContext.h"
 #include "lldb/Symbol/ClangASTImporter.h"
 #include "lldb/Symbol/ClangExternalASTSourceCommon.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolVendor.h"
+#include "lldb/Symbol/Variable.h"
+#include "lldb/Symbol/VariableList.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
@@ -887,6 +890,131 @@
   return CreateAndCacheType(type_uid);
 }
 
+static DWARFExpression MakeGlobalLocationExpression(uint16_t section,
+uint32_t offset,
+ModuleSP module) {
+  assert(section > 0);
+  assert(module);
+
+  const ArchSpec &architecture = module->GetArchitecture();
+  ByteOrder byte_order = architecture.GetByteOrder();
+  uint32_t address_size = architecture.GetAddressByteSize();
+  uint32_t byte_size = architecture.GetDataByteSize();
+  assert(byte_order != eByteOrderInvalid && address_size != 0);
+
+  RegisterKind register_kind = eRegisterKindDWARF;
+  StreamBuffer<32> stream(Stream::eBinary, address_size, byte_order);
+  stream.PutHex8(DW_OP_addr);
+
+  SectionList *section_list = module->GetSectionList();
+  assert(section_list);
+
+  // Section indices in PDB are 1-based, but in DWARF they are 0-based, so we
+  // need to subtract 1.
+  uint32_t section_idx = section - 1;
+  if (section_idx >= section_list->GetSize())
+return DWARFExpression(nullptr);
+
+  auto section_ptr = section_list->GetSectionAtIndex(section_idx);
+  if (!section_ptr)
+return DWARFExpression(nullptr);
+
+  stream.PutMaxHex64(section_ptr->GetFileAddress() + offset, address_size,
+ byte_order);
+  DataBufferSP buffer =
+  std::make_shared(stream.GetData(), stream.GetSize());
+  DataExtractor extractor(buffer, byte_order, address_size, byte_size);
+  DWARFExpression result(module, extractor, nullptr, 0, buffer->GetByteSize());
+  result.SetRegisterKind(register_kind);
+  return result;
+}
+
+VariableSP SymbolFileNativePDB::CreateGlobalVariable(PdbSymUid var_uid) {
+  const PdbCuSymId &cu_sym = var_uid.asCuSym();
+  lldbassert(cu_sym.global);
+  CVSymbol sym = m_index->symrecords().readRecord(cu_sym.offset);
+  lldb::ValueType scope = eValueTypeInvalid;
+  TypeIndex ti;
+  llvm::StringRef name;
+  lldb::addr_t addr = 0;
+  uint16_t section = 0;
+  uint32_t offset = 0;
+  bool is_external = false;
+  switch (sym.kind()) {
+  case S_GDATA32:
+is_external = true;
+LLVM_FALLTHROUGH;
+  case S_LDATA32: {
+DataSym ds(sym.kind());
+llvm::cantFail(SymbolDeserializer::deserializeAs(sym, ds));
+ti = ds.Type;
+scope = (sym.kind() == S_GDATA32) ? eValueTypeVariableGlobal
+  : eValueTypeVariableStatic;
+name = ds.Name;
+section = ds.Segment;
+offset = ds.DataOffset;
+addr = m_index->MakeVirtualAddress(ds.Segment, ds.DataOffset);
+break;
+  }
+  case S_GTHREAD32:
+is_external = true;
+LLVM_FALLTHROUGH;
+  case S_LTHREAD32: {
+ThreadLocalDataSym tlds(sym.kind());
+llvm::cantFail(
+SymbolDeserializer::deserializeAs(sym, tlds));
+ti = tlds.Type;
+name = tlds.Name;
+section = tlds.Segment;
+offset = tlds.DataOffset;
+addr = m_index->MakeVirtualAddress(tlds.Segment, tlds.DataOffset);
+scope = eValueTypeVariableThreadLocal;
+break;
+  }
+  default:
+llvm_unreachable("unreachable!");
+  }
+
+  CompUnitSP comp_unit;
+  llvm::Optional modi = m_index->GetModuleIndexForVa(addr);
+  if (modi) {
+PdbSymUid cuid = PdbSymUid::makeCompilandId(*modi);
+CompilandIndexItem &cci = m_index->compilands().GetOrCreateCompiland(cuid);
+comp_unit = Ge

[Lldb-commits] [PATCH] D53749: [PDB] Fix `SymbolFilePDBTests` after r345313

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: aleksandr.urakov.
zturner added a comment.

Ahh, I meant to remind you, because I noticed this when I was looking
through the SymbolFilePDB code recently.  I think the LLVM guideline is not
to use so much auto.  The generally accepted rule is that we can use for
the result of make_shared, make_unique, and result of iterator types, but
otherwise we prefer to explicitly spell the types out.  You don't have to
go and submit another patch on top of this one to fix it again, it's just
something to keep in mind.

That said, thanks for fixing this for me.  Sorry for the break.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53749



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53749: [PDB] Fix `SymbolFilePDBTests` after r345313

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

For trivial changes it's ok to submit without review.  This is true for
cleanup and trivial refactor, but especially for build break like this
one., and even more so if you consider yourself code owner in the
corresponding code area.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53749



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53753: [Windows] Define generic arguments registers for Windows x64

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: aleksandr.urakov.
zturner added a comment.

Lgtm


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53753



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: clayborg.
zturner added a comment.

Ok, that reasoning makes sense, I’ll see what i can do


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53788: [FileSystem] Remove GetByteSize() from FileSpec

2018-10-26 Thread Zachary Turner via Phabricator via lldb-commits
zturner accepted this revision.
zturner added a comment.

I always wondered if we actually even need methods like this in `FileSystem` 
given that they already exist in `llvm::sys::fs`.  Is it possible to just call 
the llvm methods directly, or is it still helpful to call the ones in 
`FileSystem` so we can more transparently interoperate with the VFS layer 
somehow?


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53788



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-10-28 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Is this still blocked on anything?


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D52618: [Windows] A basic implementation of memory allocations in a debuggee process

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

I think we should try as hard as possible to have just one way of writing these 
tests.  So if we know there are going to be two different use cases, one where 
we have mutually exclusive variants (e.g. run a process on OSX, Linux, 
Windows), and platform agnostic variants (compile for multiple platform / debug 
info variants, run tests in every possible configuration), I think we should 
try to unify all of these under a single syntax.

One possible syntax that works today is to duplicate all the compile and test 
lines but delegate to an external check file.

  ; file 1
  ; REQUIRES: windows
  ; RUN: clang-cl ...
  ; RUN: lld-link ...
  ; RUN: lldb-test | FileCheck --check-file=%p/Inputs/some-test.check
  
  ; file 2
  ; UNSUPPORTED: windows
  ; RUN: clang++ ...
  ; RUN: lldb-test | FileCheck --check-file=%p/Inputs/some-test.check

This unblocks the patch, doesn't require any fancy logic that would be 
unfamiliar with people coming to LLDB, and is pretty simple.

I think we can do better than the above by removing the need to have multiple 
files, but the substitutions only get us halfway there because they don't 
handle the case where we actually want to repeat the same test multiple times 
with different command lines.  So since we're going to have to fall back to the 
above approach for that, let's just do it everywhere until we can come up with 
a solution that unifies everything under one syntax.

FWIW, I've been brainstorming some modifications to the core lit infrastructure 
that would support this kind of thing.  Here's some strawman syntax:

  VARIANTS: v1, v2
  
  REQUIRES-v1: windows
  UNSUPPORTED-v2: windows
  
  RUN-v1: %cxx %p/Inputs/call-function.cpp -g -o %t
  RUN-v2: clang-cl /Zi %p/Inputs/call-function.cpp -o %t
  
  RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-basic
  RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-basic
  
  RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-overlap1
  RUN: lldb-test ir-memory-map -host-only %t %S/Inputs/ir-memory-map-overlap1
  
  RUN: lldb-test ir-memory-map %t %S/Inputs/ir-memory-map-mix-malloc-free
  RUN: lldb-test ir-memory-map -host-only %t 
%S/Inputs/ir-memory-map-mix-malloc-free

Now in this case the two variants are mutually exclusive, but they need not be. 
 So, for example, in the case of my target variables test for builtin types, 
which I wrote as this:

  // Test that we can display tag types.
  // RUN: clang-cl /Z7 /GS- /GR- /c -Xclang -fkeep-static-consts /Fo%t.obj -- %s
  // RUN: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb -- 
%t.obj
  // RUN: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
  // RUN: %p/Inputs/globals-fundamental.lldbinit | FileCheck %s
  
  // bunch of source code

We could re-write it as:

  // VARIANTS: v1, v2
  
  // RUN-v1: clang-cl /Z7 /GS- /GR- /c -Xclang -fkeep-static-consts /Fo%t.obj 
-- %s
  // RUN-v1: lld-link /DEBUG /nodefaultlib /entry:main /OUT:%t.exe /PDB:%t.pdb 
-- %t.obj
  // RUN-v1: env LLDB_USE_NATIVE_PDB_READER=1 lldb -f %t.exe -s \
  // RUN-v1: %p/Inputs/globals-fundamental.lldbinit | FileCheck %s
  // RUN-v2: clang++ -g -m64 -Xclang -fkeep-static-consts 
--target=x86_x64-linux-gnu -o %t -- %s
  // RUN-v2: lldb -f %t -s %p/Inputs/globals-fundamental.lldbinit | FileCheck %s
  
  // bunch of source code

This will run variant 1, then run variant 2.

I think this fits nicely with the lit "philosophy" and handles all the use 
cases that we need.  But there's a lot of detail in implementing it correctly 
(and right now it's just in my head, I'm not actually working on it).  So I 
think we should go with the simple thing for now of just having 2 files and one 
shared check file so that we can unblock the patch and make forward progress


https://reviews.llvm.org/D52618



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53822: [NativePDB] Add tests for dumping global variables of class type

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: lemo, aleksandr.urakov.

Previous patches added support for dumpign global variables of builtin types, 
so this patch does the same for class types.

For the most part, everything just worked, there was only one minor bug fix 
needed, which was that when a variable is of a modified class type (const, 
volatile, etc), we can't resolve the forward decl in `CreateAndCacheType`, so 
when it comes time to call `CompleteType`, one of our asserts was firing 
because we expected a Class, Struct, Enum, or Union, but we were getting an 
`LF_MODIFIER`.  The other issue was that one of my tests added an array member, 
and we hadn't yet handled them, so I just went ahead and handled them since it 
was easy.

There's probably room for lots of interesting test cases here, but these are 
the ones I was able to come up with.


https://reviews.llvm.org/D53822

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/globals-classes.lldbinit
  lldb/lit/SymbolFile/NativePDB/global-classes.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -170,6 +170,8 @@
  const llvm::codeview::EnumRecord &er);
   lldb::TypeSP CreateTagType(PdbSymUid type_uid,
  const llvm::codeview::UnionRecord &ur);
+  lldb::TypeSP CreateArrayType(PdbSymUid type_uid,
+   const llvm::codeview::ArrayRecord &ar);
   lldb::TypeSP
   CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name, size_t size,
  clang::TagTypeKind ttk,
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -391,9 +391,7 @@
   // If this is an LF_MODIFIER, look through it to get the kind that it
   // modifies.  Note that it's not possible to have an LF_MODIFIER that
   // modifies another LF_MODIFIER, although this would handle that anyway.
-  ModifierRecord mr;
-  llvm::cantFail(TypeDeserializer::deserializeAs(cvt, mr));
-  return GetPdbSymType(tpi, mr.ModifiedType);
+  return GetPdbSymType(tpi, LookThroughModifierRecord(cvt));
 }
 
 static clang::TagTypeKind TranslateUdtKind(const TagRecord &cr) {
@@ -774,6 +772,25 @@
   lldb_private::Type::eResolveStateForward);
 }
 
+TypeSP SymbolFileNativePDB::CreateArrayType(PdbSymUid type_uid,
+const ArrayRecord &ar) {
+  TypeSP element_type = GetOrCreateType(ar.ElementType);
+  uint64_t element_count = ar.Size / element_type->GetByteSize();
+
+  CompilerType element_ct = element_type->GetFullCompilerType();
+
+  CompilerType array_ct =
+  m_clang->CreateArrayType(element_ct, element_count, false);
+
+  Declaration decl;
+  TypeSP array_sp = std::make_shared(
+  type_uid.toOpaqueId(), m_clang->GetSymbolFile(), ConstString(), ar.Size,
+  nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+  array_ct, lldb_private::Type::eResolveStateFull);
+  array_sp->SetEncodingType(element_type.get());
+  return array_sp;
+}
+
 TypeSP SymbolFileNativePDB::CreateType(PdbSymUid type_uid) {
   const PdbTypeSymId &tsid = type_uid.asTypeSym();
   TypeIndex index(tsid.index);
@@ -816,6 +833,12 @@
 return CreateTagType(type_uid, ur);
   }
 
+  if (cvt.kind() == LF_ARRAY) {
+ArrayRecord ar;
+llvm::cantFail(TypeDeserializer::deserializeAs(cvt, ar));
+return CreateArrayType(type_uid, ar);
+  }
+
   return nullptr;
 }
 
@@ -1441,6 +1464,25 @@
   auto types_iter = m_types.find(uid.toOpaqueId());
   lldbassert(types_iter != m_types.end());
 
+  if (cvt.kind() == LF_MODIFIER) {
+TypeIndex unmodified_type = LookThroughModifierRecord(cvt);
+cvt = m_index->tpi().getType(unmodified_type);
+// LF_MODIFIERS usually point to forward decls, so this is the one case
+// where we won't have been able to resolve a forward decl to a full decl
+// earlier on.  So we need to do that now.
+if (IsForwardRefUdt(cvt)) {
+  llvm::Expected expected_full_ti =
+  m_index->tpi().findFullDeclForForwardRef(unmodified_type);
+  if (!expected_full_ti) {
+llvm::consumeError(expected_full_ti.takeError());
+return false;
+  }
+  cvt = m_index->tpi().getType(*expected_full_ti);
+  lldbassert(!IsForwardRefUdt(cvt));
+  unmodified_type = *expected_full_ti;
+}
+uid = PdbSymUid::makeTypeSymId(ui

[Lldb-commits] [PATCH] D53788: [FileSystem] Remove GetByteSize() from FileSpec

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53788#1279096, @shafik wrote:

> Are we refactoring in the right place? Why not just refactor 
> `FileSpec::GetByteSize()` why is `FileSpec` the wrong place?


There was another review thread where we discussed this just the other day.  
Basically, It took a herculean effort to get `FileSpec` into the Utility 
library, which helped greatly with layering and we don't want to undo this.  In 
order to for `GetByteSize` to be able to use the VFS, it has to be able to use 
`FileSystem`, so either `FileSpec` has to move out of `Utility`, `FileSystem` 
has to move into `Utility`, or `GetByteSize` has to move out of `FileSpec`.  
`FileSystem` moving into `Utility` might actually be a worthwhile goal long 
term, but even putting that aside, I think `FileSpec` should just be a thing 
for manipulating paths.  For example, it has the notion of manipulating paths 
which might not even make sense on the given host platform (e.g. it has a 
member variable which represents the path *syntax).  You might be on a Windows 
host debugging a remove Linux target, for example, and your host needs to have 
some way to manipulate paths on the remote which use a non-host syntax.  That's 
what `FileSpec` is supposed to be.  It just deals with strings.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D53788



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

So I started looking into this, and things get tricky.  If we're doing a lookup 
by fully qualified name, I would expect there to never be more than one match, 
but LLDB doesn't seem to hold this same assumption.  For example, in 
`ItaniumABILanguageRuntime::GetTypeInfoFromVTableAddress` there is code that 
tries to get an exact match from a single module, and if it doesn't find one, 
then searches the entire module list for multiple matches on exact name, then 
specifically handles the case where more than one match was found.  Most of 
this code is all just for the purposes of logging, but if we cut through all 
the logging and just get down to the code, it basically amounts to this:

type_info.SetName(class_name);
TypeList class_types;
  
uint32_t num_matches = 0;
// First look in the module that the vtable symbol came from and
// look for a single exact match.
llvm::DenseSet searched_symbol_files;
if (sc.module_sp) {
  num_modules = sc.module_sp->FindTypes(sc, ConstString(lookup_name),
exact_match, 1, searched_symbol_files, class_types);
}
  
// If we didn't find a symbol, then move on to the entire module
// list in the target and get as many unique matches as possible
if (num_matches == 0) {
  num_matches = target.GetImages().FindTypes(
  sc, ConstString(lookup_name), exact_match, UINT32_MAX,
  searched_symbol_files, class_types);
}
  
lldb::TypeSP type_sp;
if (num_matches == 0)
  return TypeAndOrName();
if (num_matches == 1) {
  type_sp = class_types.GetTypeAtIndex(0);
  if (type_sp) {
if (ClangASTContext::IsCXXClassType(
type_sp->GetForwardCompilerType())) {
  type_info.SetTypeSP(type_sp);
  }
} else if (num_matches > 1) {
  size_t i;
  
  for (i = 0; i < num_matches; i++) {
type_sp = class_types.GetTypeAtIndex(i);
if (type_sp) {
  if (ClangASTContext::IsCXXClassType(
  type_sp->GetForwardCompilerType())) {
type_info.SetTypeSP(type_sp);
  }
}
  }
  
}
if (type_info)
  SetDynamicTypeInfo(vtable_addr, type_info);
return type_info;
  }

Why would we ever get more than one type on a fully qualifed exact match name 
lookup?  And even if we did, why would we care which one we chose?


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53662#1279738, @jingham wrote:

> Exact matches aren't a C++ specific thing.  An exact match is useful for 
> mixed C/C++ since you might want to say Foo and not Bar::Foo.  IIRC a couple 
> of the places where exact was dialed up explicitly in FindTypes were for C 
> types.   But since C and ObjC allow multiple types with the same name, that's 
> one way you might see multiple matches for "exact match".  Moreover C+ used 
> to be fuzzy about whether non-exported classes from different linkage uses 
> had to follow the ODR.  I haven't followed whether this got nailed down but 
> it still seems Quixotic to expect you could enforce this, since you are 
> asking two totally unrelated library vendors to avoid each other's names for 
> private classes...  So having several classes with the same name is still a 
> possibility IRL with C++.
>
> Why we care is that if somebody runs the command:
>
> (lldb) expression (::SomeClass *) 0x12345
>
> We're going to try our best to make that work.  If SomeClass is not visible 
> in the module/comp_unit/function in which we are currently stopped, then we 
> will go looking far and wide for SomeClass.  If we find one result somewhere 
> out in the world, the expression will succeed, but if we find two results 
> which are different, then we want to give an error about ambiguous type.


So when designing this new `FindTypesByFullyQualifiedName` API, is it safe to 
assume, for the purposes of the API signature, that at the //Module// level, it 
will either return 0 or 1 match (e.g. the return value shoudl be a `TypeSP`, 
but at the //ModuleList// level, it can return arbitrarily many (so the return 
value should be a `TypeList`?


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53662#1279761, @jingham wrote:

> That depends on the definition of "fully qualified name".  If you can ensure 
> that it means "name of C++ class" - or other ODR enforcing type system, then 
> you could make that assumption.  In C you are free to redefine types on a 
> per-function basis if you so desire; and sadly some interface generators 
> (including MIG the one for generating Mach message handlers) do just this...) 
>  So you would need to see a way to restrict the inputs to this function.  
> That doesn't seem like it would be straightforward to me.


After I've changed up the codebase there is only one usage of it that I'm not 
sure is used in an ODR-enforcing context, which is 
`ObjCLanguageRuntime::LookupInCompleteClassCache`.  All other usages are C++ or 
Java specific.  Anyway, I guess I'll do it that way for now and upload the 
patch.  This already turned out to be more invasvive than I was hoping for, so 
if this doesn't work out or someone doesn't like the approach, maybe I can just 
go back to the original patch and submit that


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53662: Give the SymbolFile plugin enough information to efficiently do exact match lookups

2018-10-29 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D53662#1279777, @jingham wrote:

> Yes, that usage is exactly the sort of use I was talking about.  When we get 
> an ObjC object and want to find its dynamic type, we read the type name by 
> following the object's ISA pointer to the Class object.  Then we go to look 
> up the type from that name.  We know we want an exact match to the name we 
> found, but there's nothing that says the same module can't have an C++ class 
> with the same name as an ObjC class.  So that code needs to get all the types 
> found, and sort through them for the one that is an ObjC interface type, and 
> discard all the others.  You will need to support that behavior somehow.


So just to make sure I understand, The C++ and ObjC classes are both 
potentially in the same Module, both potentially with the same name, correct?


https://reviews.llvm.org/D53662



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53822: [NativePDB] Add tests for dumping global variables of class type

2018-10-30 Thread Zachary Turner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345629: [NativePDB] Add support for dumping global variables 
of class type. (authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53822?vs=171537&id=171755#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53822

Files:
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/globals-classes.lldbinit
  lldb/trunk/lit/SymbolFile/NativePDB/global-classes.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.h
@@ -59,6 +59,8 @@
 
 lldb::AccessType TranslateMemberAccess(llvm::codeview::MemberAccess access);
 llvm::codeview::TypeIndex GetFieldListIndex(llvm::codeview::CVType cvt);
+llvm::codeview::TypeIndex
+LookThroughModifierRecord(llvm::codeview::CVType modifier);
 
 llvm::StringRef DropNameScope(llvm::StringRef name);
 
Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -170,6 +170,8 @@
  const llvm::codeview::EnumRecord &er);
   lldb::TypeSP CreateTagType(PdbSymUid type_uid,
  const llvm::codeview::UnionRecord &ur);
+  lldb::TypeSP CreateArrayType(PdbSymUid type_uid,
+   const llvm::codeview::ArrayRecord &ar);
   lldb::TypeSP
   CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name, size_t size,
  clang::TagTypeKind ttk,
Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
@@ -343,6 +343,13 @@
   }
 }
 
+TypeIndex lldb_private::npdb::LookThroughModifierRecord(CVType modifier) {
+  lldbassert(modifier.kind() == LF_MODIFIER);
+  ModifierRecord mr;
+  llvm::cantFail(TypeDeserializer::deserializeAs(modifier, mr));
+  return mr.ModifiedType;
+}
+
 llvm::StringRef lldb_private::npdb::DropNameScope(llvm::StringRef name) {
   // Not all PDB names can be parsed with CPlusPlusNameParser.
   // E.g. it fails on names containing `anonymous namespace'.
Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -391,9 +391,7 @@
   // If this is an LF_MODIFIER, look through it to get the kind that it
   // modifies.  Note that it's not possible to have an LF_MODIFIER that
   // modifies another LF_MODIFIER, although this would handle that anyway.
-  ModifierRecord mr;
-  llvm::cantFail(TypeDeserializer::deserializeAs(cvt, mr));
-  return GetPdbSymType(tpi, mr.ModifiedType);
+  return GetPdbSymType(tpi, LookThroughModifierRecord(cvt));
 }
 
 static clang::TagTypeKind TranslateUdtKind(const TagRecord &cr) {
@@ -775,6 +773,25 @@
   lldb_private::Type::eResolveStateForward);
 }
 
+TypeSP SymbolFileNativePDB::CreateArrayType(PdbSymUid type_uid,
+const ArrayRecord &ar) {
+  TypeSP element_type = GetOrCreateType(ar.ElementType);
+  uint64_t element_count = ar.Size / element_type->GetByteSize();
+
+  CompilerType element_ct = element_type->GetFullCompilerType();
+
+  CompilerType array_ct =
+  m_clang->CreateArrayType(element_ct, element_count, false);
+
+  Declaration decl;
+  TypeSP array_sp = std::make_shared(
+  type_uid.toOpaqueId(), m_clang->GetSymbolFile(), ConstString(), ar.Size,
+  nullptr, LLDB_INVALID_UID, lldb_private::Type::eEncodingIsUID, decl,
+  array_ct, lldb_private::Type::eResolveStateFull);
+  array_sp->SetEncodingType(element_type.get());
+  return array_sp;
+}
+
 TypeSP SymbolFileNativePDB::CreateType(PdbSymUid type_uid) {
   const PdbTypeSymId &tsid = type_uid.asTypeSym();
   TypeIndex index(tsid.index);
@@ -817,6 +834,12 @@
 return CreateTagType(type_uid, ur);
   }
 
+  if (cvt.kind() == LF_ARRAY) {
+ArrayRecord ar;
+llvm::cantFail(TypeDeserializer::deserializeAs(cvt, ar));
+return CreateArrayType(type_uid, ar);
+  }
+
   return nullptr;
 }
 
@@ -1442,6 +1465,25 @@
   auto types_iter = m_types.find(uid.toOpaqueId())

[Lldb-commits] [PATCH] D52461: [PDB] Introduce `MSVCUndecoratedNameParser`

2018-10-30 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D52461#1280527, @aleksandr.urakov wrote:

> Update the diff according to the discussion, making it possible to parse MSVC 
> demangled names by `CPlusPlusLanguage`. The old PDB plugin still uses 
> `MSVCUndecoratedNameParser` directly because:
>
> - we are sure that the name in PDB is an MSVC name;
> - it has a more convenient interface, especially for restoring namespaces 
> from the parsed name.


So I had an interesting solution to this while working on the native pdb 
plugin.  it is impossible to use it with the old pdb plugin, but given that it 
works flawlessly for the native pdb plugin, depending on how urgent your need 
is, maybe you can just put off working on this until you're ready to move over 
to the native pdb plugin?

Basically the idea is that the raw PDB contains mangled type names for every 
type.  You can see this by dumping types using `llvm-pdbutil`, as follows (I 
just picked a random one from my build directory).

  D:\src\llvmbuild\ninja-x64>bin\llvm-pdbutil.exe dump -types bin\sancov.pdb | 
grep -A 2 LF_STRUCT | more
  0x1001 | LF_STRUCTURE [size = 88] ``anonymous-namespace'::RawCoverage`
   unique name: `.?AURawCoverage@?A0xa74cdb40@@`
   vtable: , base list: , field list: 
  --
  0x100A | LF_STRUCTURE [size = 212] `std::default_delete,std::allocator > >`
   unique name: 
`.?AU?$default_delete@V?$set@_KU?$less@_K@std@@V?$allocator@_K@2@@std@@@std@@`
   vtable: , base list: , field list: 
  --
  0x102B | LF_STRUCTURE [size = 88] ``anonymous-namespace'::FileHeader`
   unique name: `.?AUFileHeader@?A0xa74cdb40@@`
   vtable: , base list: , field list: 
  --
  0x1031 | LF_STRUCTURE [size = 112] 
`std::default_delete`
   unique name: `.?AU?$default_delete@VMemoryBuffer@llvm@@@std@@`
   vtable: , base list: , field list: 
  --
  0x1081 | LF_STRUCTURE [size = 304] 
`llvm::AlignedCharArrayUnion
 >,char,char,char,char,char,char,char,char,char>`
   unique name: 
`.?AU?$AlignedCharArrayUnion@V?$unique_ptr@VMemoryBuffer@llvm@@U?$default_delete@VMemoryBuffer@llvm@@@std@@@std@@D@llvm@@`
   vtable: , base list: , field list: 
  --
  0x1082 | LF_STRUCTURE [size = 176] 
`llvm::AlignedCharArrayUnion`
   unique name: 
`.?AU?$AlignedCharArrayUnion@Verror_code@std@@D@llvm@@`
   vtable: , base list: , field list: 

So the interesting thing here is this "unique name" field.  This is not 
possible to access via DIA SDK but it gives us complete rich information about 
the type that is otherwise impossible.  We don't even have to guess, because we 
can just demangle the name.  And coincidentally, I recently just finished 
writing an Microsoft ABI demangler which is now in LLVM.  :)   This `.?AU` 
syntax is non-standard, but it was easy for me to figure out, and I hacked up 
our demangle library to support this prefix (it's not checked in yet).  And 
basically everything that comes after it exactly matches a mangled type.

So, just to give an example.  Instead of teaching `CPlusPlusNameParser` to 
handle ``anonymous namespace'::RawCoverage`, we simply demangle 
`.?AURawCoverage@?A0xa74cdb40@@`, and we get back a vector of 2 strings which 
are ``anonymous namespace'` and `RawCoverage`.  But instead of just that, there 
are so many other benefits.  Since PDB doesn't contain rich information about 
template parameters, all we could do until now is just say create an entry in 
the AST that says "there's a type with this enormously long name that contains 
angle brackets and other junk".  But with this technique, we could actually 
create legitimate template decls in the AST the way it's supposed to be.

There is obviously a lot of complexity in doing it here, but I think long term 
it will be a richer experience if we parse the mangled name than if we parse 
the demangled name.  But it's only possible with the native plugin.

What do you think?


https://reviews.llvm.org/D52461



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53951: [NativePDB] Get LLDB types from PDB function types

2018-10-31 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: aleksandr.urakov, lemo.

  This adds basic support for getting function signature types
  into LLDB's type system, including into clang's AST.  There are
  a few edge cases which are not correctly handled, mostly dealing
  with nested classes, but this isn't specific to functions and
  apply equally to variable types.  Note that no attempt has been
  made yet to deal with member function types, which will happen
  in subsequent patches.


https://reviews.llvm.org/D53951

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/function-types-builtins.lldbinit
  lldb/lit/SymbolFile/NativePDB/Inputs/function-types-calling-conv.lldbinit
  lldb/lit/SymbolFile/NativePDB/Inputs/function-types-classes.lldbinit
  lldb/lit/SymbolFile/NativePDB/function-types-builtins.cpp
  lldb/lit/SymbolFile/NativePDB/function-types-calling-conv.cpp
  lldb/lit/SymbolFile/NativePDB/function-types-classes.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  llvm/include/llvm/DebugInfo/CodeView/SymbolDeserializer.h

Index: llvm/include/llvm/DebugInfo/CodeView/SymbolDeserializer.h
===
--- llvm/include/llvm/DebugInfo/CodeView/SymbolDeserializer.h
+++ llvm/include/llvm/DebugInfo/CodeView/SymbolDeserializer.h
@@ -47,7 +47,7 @@
 return Error::success();
   }
   template  static Expected deserializeAs(CVSymbol Symbol) {
-T Record(Symbol.kind());
+T Record(static_cast(Symbol.kind()));
 if (auto EC = deserializeAs(Symbol, Record))
   return std::move(EC);
 return Record;
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -172,6 +172,8 @@
  const llvm::codeview::UnionRecord &ur);
   lldb::TypeSP CreateArrayType(PdbSymUid type_uid,
const llvm::codeview::ArrayRecord &ar);
+  lldb::TypeSP CreateProcedureType(PdbSymUid type_uid,
+   const llvm::codeview::ProcedureRecord &pr);
   lldb::TypeSP
   CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name, size_t size,
  clang::TagTypeKind ttk,
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -49,6 +49,8 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
 
+#include "Plugins/Language/CPlusPlus/CPlusPlusNameParser.h"
+
 #include "PdbSymUid.h"
 #include "PdbUtil.h"
 #include "UdtRecordCompleter.h"
@@ -394,6 +396,12 @@
   return GetPdbSymType(tpi, LookThroughModifierRecord(cvt));
 }
 
+static bool IsCVarArgsFunction(llvm::ArrayRef args) {
+  if (args.empty())
+return false;
+  return args.back() == TypeIndex::None();
+}
+
 static clang::TagTypeKind TranslateUdtKind(const TagRecord &cr) {
   switch (cr.Kind) {
   case TypeRecordKind::Class:
@@ -412,6 +420,32 @@
   }
 }
 
+static llvm::Optional
+TranslateCallingConvention(llvm::codeview::CallingConvention conv) {
+  using CC = llvm::codeview::CallingConvention;
+  switch (conv) {
+
+  case CC::NearC:
+  case CC::FarC:
+return clang::CallingConv::CC_C;
+  case CC::NearPascal:
+  case CC::FarPascal:
+return clang::CallingConv::CC_X86Pascal;
+  case CC::NearFast:
+  case CC::FarFast:
+return clang::CallingConv::CC_X86FastCall;
+  case CC::NearStdCall:
+  case CC::FarStdCall:
+return clang::CallingConv::CC_X86StdCall;
+  case CC::ThisCall:
+return clang::CallingConv::CC_X86ThisCall;
+  case CC::NearVector:
+return clang::CallingConv::CC_X86VectorCall;
+  default:
+return llvm::None;
+  }
+}
+
 void SymbolFileNativePDB::Initialize() {
   PluginManager::RegisterPlugin(GetPluginNameStatic(),
 GetPluginDescriptionStatic(), CreateInstance,
@@ -540,7 +574,6 @@
   PdbSymUid sig_uid =
   PdbSymUid::makeTypeSymId(PDB_SymType::FunctionSig, TypeIndex{0}, false);
   Mangled mangled(getSymbolName(sym_record));
-
   FunctionSP func_sp = std::make_shared(
   sc.comp_unit, func_uid.toOpaqueId(), sig_uid.toOpaqueId(), mangled,
   func_type, func_range);
@@ -598,6 +631,8 @@
 lldb::TypeSP SymbolFileNativePDB::CreatePointerType(
 PdbSymUid type_uid, const llvm::codeview::PointerRecord &pr) {
   TypeSP pointee = GetOrCreateType(pr.ReferentType);
+  if (!pointee)
+return nullptr;
   CompilerType pointee_ct = pointee->GetForwardCompilerType();
   lldbassert(pointee_ct);
   Declaration decl;
@@ -670,7 +705,8 @@
 return nullptr;
 
   ll

[Lldb-commits] [PATCH] D53951: [NativePDB] Get LLDB types from PDB function types

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rL345848: [NativePDB] Get LLDB types from PDB function types. 
(authored by zturner, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D53951?vs=172023&id=172151#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D53951

Files:
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/function-types-builtins.lldbinit
  
lldb/trunk/lit/SymbolFile/NativePDB/Inputs/function-types-calling-conv.lldbinit
  lldb/trunk/lit/SymbolFile/NativePDB/Inputs/function-types-classes.lldbinit
  lldb/trunk/lit/SymbolFile/NativePDB/function-types-builtins.cpp
  lldb/trunk/lit/SymbolFile/NativePDB/function-types-calling-conv.cpp
  lldb/trunk/lit/SymbolFile/NativePDB/function-types-classes.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbUtil.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  llvm/trunk/include/llvm/DebugInfo/CodeView/SymbolDeserializer.h

Index: lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/trunk/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -49,6 +49,8 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/MemoryBuffer.h"
 
+#include "Plugins/Language/CPlusPlus/CPlusPlusNameParser.h"
+
 #include "PdbSymUid.h"
 #include "PdbUtil.h"
 #include "UdtRecordCompleter.h"
@@ -394,6 +396,12 @@
   return GetPdbSymType(tpi, LookThroughModifierRecord(cvt));
 }
 
+static bool IsCVarArgsFunction(llvm::ArrayRef args) {
+  if (args.empty())
+return false;
+  return args.back() == TypeIndex::None();
+}
+
 static clang::TagTypeKind TranslateUdtKind(const TagRecord &cr) {
   switch (cr.Kind) {
   case TypeRecordKind::Class:
@@ -412,6 +420,32 @@
   }
 }
 
+static llvm::Optional
+TranslateCallingConvention(llvm::codeview::CallingConvention conv) {
+  using CC = llvm::codeview::CallingConvention;
+  switch (conv) {
+
+  case CC::NearC:
+  case CC::FarC:
+return clang::CallingConv::CC_C;
+  case CC::NearPascal:
+  case CC::FarPascal:
+return clang::CallingConv::CC_X86Pascal;
+  case CC::NearFast:
+  case CC::FarFast:
+return clang::CallingConv::CC_X86FastCall;
+  case CC::NearStdCall:
+  case CC::FarStdCall:
+return clang::CallingConv::CC_X86StdCall;
+  case CC::ThisCall:
+return clang::CallingConv::CC_X86ThisCall;
+  case CC::NearVector:
+return clang::CallingConv::CC_X86VectorCall;
+  default:
+return llvm::None;
+  }
+}
+
 void SymbolFileNativePDB::Initialize() {
   PluginManager::RegisterPlugin(GetPluginNameStatic(),
 GetPluginDescriptionStatic(), CreateInstance,
@@ -540,7 +574,6 @@
   PdbSymUid sig_uid =
   PdbSymUid::makeTypeSymId(PDB_SymType::FunctionSig, TypeIndex{0}, false);
   Mangled mangled(getSymbolName(sym_record));
-
   FunctionSP func_sp = std::make_shared(
   sc.comp_unit, func_uid.toOpaqueId(), sig_uid.toOpaqueId(), mangled,
   func_type, func_range);
@@ -598,6 +631,8 @@
 lldb::TypeSP SymbolFileNativePDB::CreatePointerType(
 PdbSymUid type_uid, const llvm::codeview::PointerRecord &pr) {
   TypeSP pointee = GetOrCreateType(pr.ReferentType);
+  if (!pointee)
+return nullptr;
   CompilerType pointee_ct = pointee->GetForwardCompilerType();
   lldbassert(pointee_ct);
   Declaration decl;
@@ -639,6 +674,17 @@
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateSimpleType(TypeIndex ti) {
+  if (ti == TypeIndex::NullptrT()) {
+PdbSymUid uid =
+PdbSymUid::makeTypeSymId(PDB_SymType::BuiltinType, ti, false);
+CompilerType ct = m_clang->GetBasicType(eBasicTypeNullPtr);
+Declaration decl;
+return std::make_shared(uid.toOpaqueId(), this,
+  ConstString("std::nullptr_t"), 0, nullptr,
+  LLDB_INVALID_UID, Type::eEncodingIsUID, decl,
+  ct, Type::eResolveStateFull);
+  }
+
   if (ti.getSimpleMode() != SimpleTypeMode::Direct) {
 PdbSymUid uid =
 PdbSymUid::makeTypeSymId(PDB_SymType::PointerType, ti, false);
@@ -670,7 +716,8 @@
 return nullptr;
 
   lldb::BasicType bt = GetCompilerTypeForSimpleKind(ti.getSimpleKind());
-  lldbassert(bt != lldb::eBasicTypeInvalid);
+  if (bt == lldb::eBasicTypeInvalid)
+return nullptr;
   CompilerType ct = m_clang->GetBasicType(bt);
   size_t size = GetTypeSizeForSimpleKind(ti.getSimpleKind());
 
@@ -687,10 +734,6 @@
 PdbSymUid type_uid, llvm::StringRef name, size_t size,
 clang::TagTypeKind ttk, clang::MSInheritanceAttr::Spelling inheritance) {
 
-  // Some UDT with trival ctor has zero length. Just ignore.
-  if (size == 0)
-return nullptr;
-
   // Ignore unnamed-tag UDTs.
   name = DropNameScope(name);
   if (name.empty())
@@ -792,6 +8

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added a reviewer: jingham.

char16, char32, and wchar_t were previously broken.  If you had a simple 
variable like `wchar_t x = L'1'` and wrote `p x` LLDB would output `(wchar_t) x 
= 1\0`.  This is because it was using `eFormatChar` with a size of 2.  What we 
needed was to introduce a special format specifically for `wchar_t`.  The only 
valid sizes of `wchar_t` on all existing compilers are 2 and 4, so there's no 
real point trying to handle sizes of 1 and 8.

Along the way, I accidentally stumbled across the reason that references that 
char16 and char32 types were also formatting incorrectly, so I fixed that as 
well.


https://reviews.llvm.org/D53989

Files:
  lldb/include/lldb/lldb-enumerations.h
  lldb/lit/SymbolFile/NativePDB/globals-fundamental.cpp
  lldb/source/Commands/CommandObjectMemory.cpp
  lldb/source/Core/DumpDataExtractor.cpp
  lldb/source/Core/ValueObject.cpp
  lldb/source/DataFormatters/FormatManager.cpp
  lldb/source/DataFormatters/VectorType.cpp
  lldb/source/Symbol/ClangASTContext.cpp

Index: lldb/source/Symbol/ClangASTContext.cpp
===
--- lldb/source/Symbol/ClangASTContext.cpp
+++ lldb/source/Symbol/ClangASTContext.cpp
@@ -5257,11 +5257,12 @@
   return lldb::eFormatBoolean;
 case clang::BuiltinType::Char_S:
 case clang::BuiltinType::SChar:
-case clang::BuiltinType::WChar_S:
 case clang::BuiltinType::Char_U:
 case clang::BuiltinType::UChar:
-case clang::BuiltinType::WChar_U:
   return lldb::eFormatChar;
+case clang::BuiltinType::WChar_S:
+case clang::BuiltinType::WChar_U:
+  return lldb::eFormatWchar;
 case clang::BuiltinType::Char16:
   return lldb::eFormatUnicode16;
 case clang::BuiltinType::Char32:
Index: lldb/source/DataFormatters/VectorType.cpp
===
--- lldb/source/DataFormatters/VectorType.cpp
+++ lldb/source/DataFormatters/VectorType.cpp
@@ -64,9 +64,12 @@
   case lldb::eFormatHexFloat:
 return type_system->GetBasicTypeFromAST(lldb::eBasicTypeFloat);
 
+  case lldb::eFormatWchar:
+return type_system->GetBasicTypeFromAST(lldb::eBasicTypeWChar);
   case lldb::eFormatUnicode16:
+return type_system->GetBasicTypeFromAST(lldb::eBasicTypeChar16);
   case lldb::eFormatUnicode32:
-
+return type_system->GetBasicTypeFromAST(lldb::eBasicTypeChar32);
   case lldb::eFormatUnsigned:
 return type_system->GetBasicTypeFromAST(lldb::eBasicTypeUnsignedInt);
 
Index: lldb/source/DataFormatters/FormatManager.cpp
===
--- lldb/source/DataFormatters/FormatManager.cpp
+++ lldb/source/DataFormatters/FormatManager.cpp
@@ -43,6 +43,7 @@
 {eFormatBytesWithASCII, 'Y', "bytes with ASCII"},
 {eFormatChar, 'c', "character"},
 {eFormatCharPrintable, 'C', "printable character"},
+{eFormatWchar, 'L', "wide character"},
 {eFormatComplexFloat, 'F', "complex float"},
 {eFormatCString, 's', "c-string"},
 {eFormatDecimal, 'd', "decimal"},
Index: lldb/source/Core/ValueObject.cpp
===
--- lldb/source/Core/ValueObject.cpp
+++ lldb/source/Core/ValueObject.cpp
@@ -1383,6 +1383,7 @@
   (custom_format == eFormatOSType) ||
   (custom_format == eFormatUnicode16) ||
   (custom_format == eFormatUnicode32) ||
+  (custom_format == eFormatWchar) ||
   (custom_format == eFormatUnsigned) ||
   (custom_format == eFormatPointer) ||
   (custom_format == eFormatComplexInteger) ||
Index: lldb/source/Core/DumpDataExtractor.cpp
===
--- lldb/source/Core/DumpDataExtractor.cpp
+++ lldb/source/Core/DumpDataExtractor.cpp
@@ -637,6 +637,23 @@
   }
 } break;
 
+case eFormatWchar: {
+  s->PutChar('L');
+  s->PutChar(item_count == 1 ? '\'' : '\"');
+
+  const uint64_t ch = DE.GetMaxU64(&offset, item_byte_size);
+  // wchar_t semantics vary across platforms, and this is complicated even
+  // more by the fact that the host may not be the same as the target, so to
+  // be faithful we would have to follow the target's semantics.  For
+  // simplicity, just print the character if it's ascii.
+  if (ch <= CHAR_MAX && llvm::isPrint(ch))
+s->PutChar(ch);
+  else
+s->PutChar(NON_PRINTABLE_CHAR);
+
+  s->PutChar(item_count == 1 ? '\'' : '\"');
+} break;
+
 case eFormatUnicode16:
   s->Printf("U+%4.4x", DE.GetU16(&offset));
   break;
Index: lldb/source/Commands/CommandObjectMemory.cpp
===
--- lldb/source/Commands/CommandObjectMemory.cpp
+++ lldb/source/Commands/CommandObjectMemory.cpp
@@ -167,6 +167,15 @@
 format_options.GetCountValue() = 8;
   break;
 
+case eFormatWchar:
+  if (!byte

[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

No, but thanks for the pointer.  Interestingly, it worked for me on my home 
machine but not on my work machine, and I'm not sure what the difference 
between the two is.

Clearly the test in lang/cpp/wchar_t is making it into 
`lldb_private::formatters::WCharSummaryProvider` whereas locally I am not.


https://reviews.llvm.org/D53989



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Update: It's because There was a problem with my `PYTHONPATH` and python was 
getting disabled at CMake configure time.  So I was effectively running with 
`LLDB_DISABLE_PYTHON`.  So basically it's only broke on the 
`LLDB_DISABLE_PYTHON` codepath.


https://reviews.llvm.org/D53989



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D53989: Fix formatting of wchar, char16, and char32

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Side question, should we just kill the `LLDB_DISABLE_PYTHON=0` codepath?  It 
can be a headache to get LLDB linking with Python (particularly on Windows), 
but it would be nice to be able to delete all the preprocessor stuff.


https://reviews.llvm.org/D53989



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: stella.stamenova, labath, beanz, davide.
Herald added subscribers: jfb, delcypher, mgorny, ki.stfu.

A year or so ago, I re-wrote most of the lit infrastructure in LLVM so that it 
wasn't so boilerplate-y.  I added lots of common helper type stuff, simplifed 
usage patterns, and made the code more elegant and maintainable.

We migrated to this in LLVM, clang, and lld's lit files, but not in LLDBs.  
This started to bite me recently, as the 4 most recent times I tried to run the 
lit test suite in LLDB on a fresh checkout the first thing that would happen is 
that python would just start crashing with unhelpful backtraces and I would 
have to spend time investigating.

You can reproduce this today by doing a fresh cmake generation, doing `ninja 
lldb` and then `python bin/llvm-lit.py -sv ~/lldb/lit/SymbolFile` at which 
point you'll get a segfault that tells you nothing about what your problem is.

I started trying to fix the issues with bandaids, but it became clear that the 
proper solution was to just bring in the work I did in the rest of the 
projects.  The side benefit of this is that the lit configuration files become 
much cleaner and more understandable as a result.


https://reviews.llvm.org/D54009

Files:
  lldb/lit/Breakpoint/case-insensitive.test
  lldb/lit/Breakpoint/lit.local.cfg
  lldb/lit/CMakeLists.txt
  lldb/lit/Expr/TestIRMemoryMapWindows.test
  lldb/lit/Expr/lit.local.cfg
  lldb/lit/Quit/lit.local.cfg
  lldb/lit/Settings/lit.local.cfg
  lldb/lit/SymbolFile/NativePDB/lit.local.cfg
  lldb/lit/SymbolFile/PDB/ast-restore.test
  lldb/lit/SymbolFile/PDB/calling-conventions.test
  lldb/lit/SymbolFile/PDB/class-layout.test
  lldb/lit/SymbolFile/PDB/compilands.test
  lldb/lit/SymbolFile/PDB/enums-layout.test
  lldb/lit/SymbolFile/PDB/func-symbols.test
  lldb/lit/SymbolFile/PDB/function-level-linking.test
  lldb/lit/SymbolFile/PDB/function-nested-block.test
  lldb/lit/SymbolFile/PDB/lit.local.cfg
  lldb/lit/SymbolFile/PDB/pointers.test
  lldb/lit/SymbolFile/PDB/type-quals.test
  lldb/lit/SymbolFile/PDB/typedefs.test
  lldb/lit/SymbolFile/PDB/udt-layout.test
  lldb/lit/SymbolFile/PDB/variables-locations.test
  lldb/lit/SymbolFile/PDB/variables.test
  lldb/lit/Unit/lit.cfg
  lldb/lit/Unit/lit.cfg.py
  lldb/lit/Unit/lit.site.cfg.in
  lldb/lit/Unit/lit.site.cfg.py.in
  lldb/lit/lit.cfg
  lldb/lit/lit.cfg.py
  lldb/lit/lit.site.cfg.in
  lldb/lit/lit.site.cfg.py.in
  lldb/lit/tools/lldb-mi/breakpoint/break-insert-enable-pending.test
  lldb/lit/tools/lldb-mi/breakpoint/break-insert.test
  lldb/lit/tools/lldb-mi/data/data-info-line.test
  lldb/lit/tools/lldb-mi/exec/exec-continue.test
  lldb/lit/tools/lldb-mi/exec/exec-finish.test
  lldb/lit/tools/lldb-mi/exec/exec-interrupt.test
  lldb/lit/tools/lldb-mi/exec/exec-next-instruction.test
  lldb/lit/tools/lldb-mi/exec/exec-next.test
  lldb/lit/tools/lldb-mi/exec/exec-step-instruction.test
  lldb/lit/tools/lldb-mi/exec/exec-step.test
  lldb/lit/tools/lldb-mi/symbol/symbol-list-lines.test
  llvm/utils/lit/lit/llvm/config.py

Index: llvm/utils/lit/lit/llvm/config.py
===
--- llvm/utils/lit/lit/llvm/config.py
+++ llvm/utils/lit/lit/llvm/config.py
@@ -55,6 +55,8 @@
 features.add('system-windows')
 elif platform.system() == "Linux":
 features.add('system-linux')
+elif platform.system() in ['FreeBSD']:
+config.available_features.add('system-freebsd')
 
 # Native compilation: host arch == default triple arch
 # Both of these values should probably be in every site config (e.g. as
Index: lldb/lit/tools/lldb-mi/symbol/symbol-list-lines.test
===
--- lldb/lit/tools/lldb-mi/symbol/symbol-list-lines.test
+++ lldb/lit/tools/lldb-mi/symbol/symbol-list-lines.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c %p/inputs/symbol-list-lines.c %p/inputs/list-lines-helper.c -g
Index: lldb/lit/tools/lldb-mi/exec/exec-step.test
===
--- lldb/lit/tools/lldb-mi/exec/exec-step.test
+++ lldb/lit/tools/lldb-mi/exec/exec-step.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c -g
Index: lldb/lit/tools/lldb-mi/exec/exec-step-instruction.test
===
--- lldb/lit/tools/lldb-mi/exec/exec-step-instruction.test
+++ lldb/lit/tools/lldb-mi/exec/exec-step-instruction.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c -g
Index: lldb/lit/tools/lldb-mi/exec/exec-next.test
===
--- lldb/lit/tools/lldb-mi/exec/exec-next.test
+++ lldb/lit/tools/lldb-mi/exec

[Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

2018-11-01 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D54009#1284827, @stella.stamenova wrote:

> Looks good. The formatting in lit.cfg.py is a bit messy (indentations, 
> especially), but as long as the tests pass, this is an improvement :). Thanks!


Is there something like clang-format for Python?  Happy to fix it if so.  (I 
don't do a lot of Python so it's hard for me to eyeball it and figure out 
what's wrong, so I have to say it looks fine to me :))


https://reviews.llvm.org/D54009



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54031: [NativePDB] Make tests work on x86 too

2018-11-02 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: aleksandr.urakov.
zturner added a comment.

Lgtm


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54031



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

2018-11-02 Thread Zachary Turner via Phabricator via lldb-commits
This revision was automatically updated to reflect the committed changes.
Closed by commit rLLDB346008: Refactor the lit configuration files (authored by 
zturner, committed by ).
Herald added subscribers: teemperor, abidh.

Changed prior to commit:
  https://reviews.llvm.org/D54009?vs=172254&id=172399#toc

Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54009

Files:
  lit/Breakpoint/case-insensitive.test
  lit/Breakpoint/lit.local.cfg
  lit/CMakeLists.txt
  lit/Expr/TestIRMemoryMapWindows.test
  lit/Expr/lit.local.cfg
  lit/Quit/lit.local.cfg
  lit/Settings/lit.local.cfg
  lit/SymbolFile/NativePDB/lit.local.cfg
  lit/SymbolFile/PDB/ast-restore.test
  lit/SymbolFile/PDB/calling-conventions.test
  lit/SymbolFile/PDB/class-layout.test
  lit/SymbolFile/PDB/compilands.test
  lit/SymbolFile/PDB/enums-layout.test
  lit/SymbolFile/PDB/func-symbols.test
  lit/SymbolFile/PDB/function-level-linking.test
  lit/SymbolFile/PDB/function-nested-block.test
  lit/SymbolFile/PDB/lit.local.cfg
  lit/SymbolFile/PDB/pointers.test
  lit/SymbolFile/PDB/type-quals.test
  lit/SymbolFile/PDB/typedefs.test
  lit/SymbolFile/PDB/udt-layout.test
  lit/SymbolFile/PDB/variables-locations.test
  lit/SymbolFile/PDB/variables.test
  lit/Unit/lit.cfg
  lit/Unit/lit.cfg.py
  lit/Unit/lit.site.cfg.in
  lit/Unit/lit.site.cfg.py.in
  lit/lit.cfg
  lit/lit.cfg.py
  lit/lit.site.cfg.in
  lit/lit.site.cfg.py.in
  lit/tools/lldb-mi/breakpoint/break-insert-enable-pending.test
  lit/tools/lldb-mi/breakpoint/break-insert.test
  lit/tools/lldb-mi/data/data-info-line.test
  lit/tools/lldb-mi/exec/exec-continue.test
  lit/tools/lldb-mi/exec/exec-finish.test
  lit/tools/lldb-mi/exec/exec-interrupt.test
  lit/tools/lldb-mi/exec/exec-next-instruction.test
  lit/tools/lldb-mi/exec/exec-next.test
  lit/tools/lldb-mi/exec/exec-step-instruction.test
  lit/tools/lldb-mi/exec/exec-step.test
  lit/tools/lldb-mi/symbol/symbol-list-lines.test

Index: lit/tools/lldb-mi/breakpoint/break-insert.test
===
--- lit/tools/lldb-mi/breakpoint/break-insert.test
+++ lit/tools/lldb-mi/breakpoint/break-insert.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o a.exe %p/inputs/break-insert.c -g
Index: lit/tools/lldb-mi/breakpoint/break-insert-enable-pending.test
===
--- lit/tools/lldb-mi/breakpoint/break-insert-enable-pending.test
+++ lit/tools/lldb-mi/breakpoint/break-insert-enable-pending.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/break-insert-pending.c -g
Index: lit/tools/lldb-mi/data/data-info-line.test
===
--- lit/tools/lldb-mi/data/data-info-line.test
+++ lit/tools/lldb-mi/data/data-info-line.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/data-info-line.c -g
Index: lit/tools/lldb-mi/symbol/symbol-list-lines.test
===
--- lit/tools/lldb-mi/symbol/symbol-list-lines.test
+++ lit/tools/lldb-mi/symbol/symbol-list-lines.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c %p/inputs/symbol-list-lines.c %p/inputs/list-lines-helper.c -g
Index: lit/tools/lldb-mi/exec/exec-finish.test
===
--- lit/tools/lldb-mi/exec/exec-finish.test
+++ lit/tools/lldb-mi/exec/exec-finish.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c -g
Index: lit/tools/lldb-mi/exec/exec-next.test
===
--- lit/tools/lldb-mi/exec/exec-next.test
+++ lit/tools/lldb-mi/exec/exec-next.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c -g
Index: lit/tools/lldb-mi/exec/exec-next-instruction.test
===
--- lit/tools/lldb-mi/exec/exec-next-instruction.test
+++ lit/tools/lldb-mi/exec/exec-next-instruction.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c -g
Index: lit/tools/lldb-mi/exec/exec-interrupt.test
===
--- lit/tools/lldb-mi/exec/exec-interrupt.test
+++ lit/tools/lldb-mi/exec/exec-interrupt.test
@@ -1,4 +1,4 @@
-# XFAIL: windows
+# XFAIL: system-windows
 # -> llvm.org/pr24452
 #
 # RUN: %cc -o %t %p/inputs/main.c -g
Index: lit/tools/lldb-mi/exec/exec-step.test
===
--- lit/tools/lldb-mi/exec/exec-step.test
+++ lit/tools/lldb-mi/exec/exec-step.test
@@ -1,4 +1,4 @@
-# XFAI

[Lldb-commits] [PATCH] D54003: Refactor ClangASTContext::AddEnumerationValueToEnumerationType() to remove redundant parameter which can be calculated from other parameter

2018-11-02 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

This function is called in `SymbolFile/NativePDB` as well.


https://reviews.llvm.org/D54003



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

2018-11-02 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: aleksandr.urakov, labath, lemo.
Herald added subscribers: erik.pilkington, mgorny.

Previously we were not able to accurately represent tag record types with clang 
record decls.  The primary reason for this is that for type information PDB 
only contains fully instantiated type names.  It does not contain rich 
information about namespaces, declaration contexts (e.g. parent classes, 
function scopes for local classes, etc), template parameters, or anything else. 
 All you have is a big long string that is a fully instantiated type name.  
What it does give you, however, is the mangled name of this type.  So we can 
extract all of the relevant information (minus the distinction between outer 
class and outer namespace) from the mangled name.  This patch is the start of 
this effort.

It uses LLVM's demangler to demangle the name, and treat each component as one 
component of a namespace chain.

Obviously this is not true in the general case, as something like 
`Foo::Bar(double)::`1'::LocalClass<7>::NestedClass` will get treated as if 
it were in a series of namespaces.  However, it's a good start, and clang 
doesn't actually care for most uses.  So we can improve on this incrementally 
with subsequent patches to make this more accurate.


https://reviews.llvm.org/D54053

Files:
  lldb/lit/SymbolFile/NativePDB/function-types-classes.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -175,7 +175,8 @@
   lldb::TypeSP CreateProcedureType(PdbSymUid type_uid,
const llvm::codeview::ProcedureRecord &pr);
   lldb::TypeSP
-  CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name, size_t size,
+  CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name,
+ llvm::StringRef unique_name, size_t size,
  clang::TagTypeKind ttk,
  clang::MSInheritanceAttr::Spelling inheritance);
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -51,6 +51,7 @@
 
 #include "Plugins/Language/CPlusPlus/CPlusPlusNameParser.h"
 
+#include "MangledAST.h"
 #include "PdbSymUid.h"
 #include "PdbUtil.h"
 #include "UdtRecordCompleter.h"
@@ -731,27 +732,11 @@
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateClassStructUnion(
-PdbSymUid type_uid, llvm::StringRef name, size_t size,
-clang::TagTypeKind ttk, clang::MSInheritanceAttr::Spelling inheritance) {
+PdbSymUid type_uid, llvm::StringRef name, llvm::StringRef unique_name,
+size_t size, clang::TagTypeKind ttk,
+clang::MSInheritanceAttr::Spelling inheritance) {
 
-  // Ignore unnamed-tag UDTs.
-  name = DropNameScope(name);
-  if (name.empty())
-return nullptr;
-
-  clang::DeclContext *decl_context = m_clang->GetTranslationUnitDecl();
-
-  lldb::AccessType access =
-  (ttk == clang::TTK_Class) ? lldb::eAccessPrivate : lldb::eAccessPublic;
-
-  ClangASTMetadata metadata;
-  metadata.SetUserID(type_uid.toOpaqueId());
-  metadata.SetIsDynamicCXXType(false);
-
-  CompilerType ct =
-  m_clang->CreateRecordType(decl_context, access, name.str().c_str(), ttk,
-lldb::eLanguageTypeC_plus_plus, &metadata);
-  lldbassert(ct.IsValid());
+  CompilerType ct = CreateClangDeclFromMangledName(*m_clang, unique_name);
 
   clang::CXXRecordDecl *record_decl =
   m_clang->GetAsCXXRecordDecl(ct.GetOpaqueQualType());
@@ -771,7 +756,7 @@
   // FIXME: Search IPI stream for LF_UDT_MOD_SRC_LINE.
   Declaration decl;
   return std::make_shared(type_uid.toOpaqueId(), m_clang->GetSymbolFile(),
-ConstString(name), size, nullptr,
+ct.GetTypeName(), size, nullptr,
 LLDB_INVALID_UID, Type::eEncodingIsUID, decl,
 ct, Type::eResolveStateForward);
 }
@@ -782,14 +767,15 @@
 
   clang::MSInheritanceAttr::Spelling inheritance =
   GetMSInheritance(m_index->tpi().typeCollection(), cr);
-  return CreateClassStructUnion(type_uid, cr.getName(), cr.getSize(), ttk,
-inheritance);
+  return CreateClassStructUnion(type_uid, cr.getName(), cr.getUniqueName(),
+cr.getSiz

[Lldb-commits] [PATCH] D54009: Refactor LLDB lit configuration files

2018-11-02 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: stella.stamenova.
zturner added a comment.

Fix incoming, sorry about that.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D54009



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

2018-11-02 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

In https://reviews.llvm.org/D54053#1286272, @lemo wrote:

> nice!


AFAIU this is just the "default" access of the class.  I should probably 
investigate why it's needed in the first place, since it can be deduced from 
the tag type.


https://reviews.llvm.org/D54053



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

2018-11-03 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

I actually found some issues with this, or at least some potential issues which 
I'm not sure are actual issues.  But I'm adding some new functionality to LLDB 
to help me confirm whether these are real issues or not, and worst case 
scenario it will open up some new testing possibilities and help us understand 
the consequences of messing around with clang ASTs in various ways.  So I'm not 
going to commit this quite yet.  Stay tuned.


https://reviews.llvm.org/D54053



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

2018-11-03 Thread Zachary Turner via Phabricator via lldb-commits
zturner created this revision.
zturner added reviewers: vsk, davide, labath, jingham, aleksandr.urakov, 
clayborg.
Herald added subscribers: JDevlieghere, aprantl.

This can be useful when diagnosing AST related problems.  For example, I had a 
bug where I was accidentally creating a record type multiple times instead of 
re-using the first one.  This is easy to see with a dump of the AST, because it 
will look like this:

  TranslationUnitDecl 0x18a2bc53b98 <> 
  |-CXXRecordDecl 0x18a2bc54458 <>  class 
ClassWithPadding
  |-CXXRecordDecl 0x18a2bc54520 <>  class 
ClassWithPadding
  |-CXXRecordDecl 0x18a2bc545e0 <>  class 
ClassWithPadding definition
  | |-DefinitionData pass_in_registers standard_layout trivially_copyable 
trivial literal
  | | |-DefaultConstructor exists trivial needs_implicit
  | | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | | |-MoveConstructor exists simple trivial needs_implicit
  | | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | | |-MoveAssignment exists simple trivial needs_implicit
  | | `-Destructor simple irrelevant trivial needs_implicit
  | |-MSInheritanceAttr 0x18a2bc546a0 <> Implicit 
__single_inheritance
  | |-FieldDecl 0x18a2bc54748 <>  a 'char'
  | |-FieldDecl 0x18a2bc54798 <>  b 'short'
  | |-FieldDecl 0x18a2bc54828 <>  c 'char [2]'
  | |-FieldDecl 0x18a2bc54878 <>  d 'int'
  | |-FieldDecl 0x18a2bc548c8 <>  e 'char'
  | |-FieldDecl 0x18a2bc54918 <>  f 'int'
  | |-FieldDecl 0x18a2bc54968 <>  g 'long long'
  | |-FieldDecl 0x18a2bc549f8 <>  h 'char [3]'
  | |-FieldDecl 0x18a2bc54a48 <>  i 'long long'
  | |-FieldDecl 0x18a2bc54a98 <>  j 'char [2]'
  | |-FieldDecl 0x18a2bc54ae8 <>  k 'long long'
  | |-FieldDecl 0x18a2bc54b38 <>  l 'char'
  | `-FieldDecl 0x18a2bd96f00 <>  m 'long long'
  `-

Note there are 3 `CXXRecordDecl`s with the same name, but only one definition.  
Given the complex interactions between debug info and AST reconstruction, a 
command like this makes problems within the AST very obvious.  I found several 
other AST-related problems, so this was not even the only one, so I think this 
is a largely unexplored front when it comes to areas for potentially improved 
test coverage.  And since it's in the REPL, it makes it very easy to test out 
commands in different orders, get a dump, do something else, get another dump, 
etc to see how the order of commands affects things.


https://reviews.llvm.org/D54072

Files:
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h

Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
@@ -169,6 +169,8 @@
 
   const llvm::pdb::IPDBSession &GetPDBSession() const;
 
+  void DumpClangAST() override;
+
 private:
   struct SecContribInfo {
 uint32_t Offset;
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -1356,6 +1356,14 @@
   return types.GetSize();
 }
 
+void SymbolFilePDB::DumpClangAST() {
+  auto type_system = GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
+  auto clang_type_system = llvm::dyn_cast_or_null(type_system);
+  if (!clang_type_system)
+return;
+  clang_type_system->getASTContext()->getTranslationUnitDecl()->dumpColor();
+}
+
 void SymbolFilePDB::FindTypesByRegex(
 const lldb_private::RegularExpression ®ex, uint32_t max_matches,
 lldb_private::TypeMap &types) {
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -155,6 +155,8 @@
   ClangASTContext &GetASTContext() { return *m_clang; }
   ClangASTImporter &GetASTImporter() { return *m_importer; }
 
+  void DumpClangAST() override;
+
 private:
   size_t FindTypesByName(llvm::StringRef name, uint32_t max_matches,
  TypeMap &types);
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePD

[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

2018-11-03 Thread Zachary Turner via Phabricator via lldb-commits
zturner updated this revision to Diff 172510.
zturner added a comment.

clang dumps to stderr.  Only use color if stderr supports this (e.g. if it's 
not redirected to a file).


https://reviews.llvm.org/D54072

Files:
  lldb/include/lldb/Symbol/SymbolFile.h
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
  lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h

Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.h
@@ -169,6 +169,8 @@
 
   const llvm::pdb::IPDBSession &GetPDBSession() const;
 
+  void DumpClangAST() override;
+
 private:
   struct SecContribInfo {
 uint32_t Offset;
Index: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
+++ lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp
@@ -1356,6 +1356,18 @@
   return types.GetSize();
 }
 
+void SymbolFilePDB::DumpClangAST() {
+  auto type_system = GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus);
+  auto clang = llvm::dyn_cast_or_null(type_system);
+  if (!clang)
+return;
+  clang::TranslationUnitDecl *tu = clang->getASTContext()->getTranslationUnitDecl();
+  if (llvm::errs().has_colors())
+tu->dumpColor();
+  else
+tu->dump();
+}
+
 void SymbolFilePDB::FindTypesByRegex(
 const lldb_private::RegularExpression ®ex, uint32_t max_matches,
 lldb_private::TypeMap &types) {
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -155,6 +155,8 @@
   ClangASTContext &GetASTContext() { return *m_clang; }
   ClangASTImporter &GetASTImporter() { return *m_importer; }
 
+  void DumpClangAST() override;
+
 private:
   size_t FindTypesByName(llvm::StringRef name, uint32_t max_matches,
  TypeMap &types);
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -1391,6 +1391,17 @@
   return 0;
 }
 
+void SymbolFileNativePDB::DumpClangAST() {
+  if (!m_clang)
+return;
+  
+  clang::TranslationUnitDecl *tu = m_clang->getASTContext()->getTranslationUnitDecl();
+  if (llvm::errs().has_colors())
+tu->dumpColor();
+  else
+tu->dump();
+}
+
 uint32_t SymbolFileNativePDB::FindGlobalVariables(
 const ConstString &name, const CompilerDeclContext *parent_decl_ctx,
 uint32_t max_matches, VariableList &variables) {
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
@@ -126,6 +126,8 @@
   std::vector
   ParseCallEdgesInFunction(lldb_private::UserID func_id) override;
 
+  void DumpClangAST() override;
+
   //--
   // PluginInterface protocol
   //--
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
@@ -1227,6 +1227,13 @@
   return matching_namespace;
 }
 
+void SymbolFileDWARFDebugMap::DumpClangAST() {
+  ForEachSymbolFile([](SymbolFileDWARF *oso_dwarf) -> bool {
+oso_dwarf->DumpClangAST();
+return true;
+  });
+}
+
 //--
 // PluginInterface protocol
 //--
Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
@@ -329,6 +329,8 @@
 
   void Dump(lldb_private::Stream &s) override;
 
+  void DumpClangAST() override;
+
 protected:
   typedef llvm::DenseMap
 

[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

2018-11-03 Thread Zachary Turner via Phabricator via lldb-commits
zturner updated this revision to Diff 172511.
zturner added a comment.

I just added a new test which dumps the clang AST and makes sure no bogus 
records get introduced.  Since this is already LGTM'ed I'm assuming this is 
good to go, but since it now depends on https://reviews.llvm.org/D54072, I 
figure I might as well update this diff since I can't commit it yet.


https://reviews.llvm.org/D54053

Files:
  lldb/lit/SymbolFile/NativePDB/Inputs/ast-reconstruction.lldbinit
  lldb/lit/SymbolFile/NativePDB/ast-reconstruction.cpp
  lldb/lit/SymbolFile/NativePDB/function-types-classes.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/CMakeLists.txt
  lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.h
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
  lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h

Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h
@@ -177,7 +177,8 @@
   lldb::TypeSP CreateProcedureType(PdbSymUid type_uid,
const llvm::codeview::ProcedureRecord &pr);
   lldb::TypeSP
-  CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name, size_t size,
+  CreateClassStructUnion(PdbSymUid type_uid, llvm::StringRef name,
+ llvm::StringRef unique_name, size_t size,
  clang::TagTypeKind ttk,
  clang::MSInheritanceAttr::Spelling inheritance);
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
===
--- lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
+++ lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp
@@ -51,6 +51,7 @@
 
 #include "Plugins/Language/CPlusPlus/CPlusPlusNameParser.h"
 
+#include "MangledAST.h"
 #include "PdbSymUid.h"
 #include "PdbUtil.h"
 #include "UdtRecordCompleter.h"
@@ -731,27 +732,11 @@
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateClassStructUnion(
-PdbSymUid type_uid, llvm::StringRef name, size_t size,
-clang::TagTypeKind ttk, clang::MSInheritanceAttr::Spelling inheritance) {
+PdbSymUid type_uid, llvm::StringRef name, llvm::StringRef unique_name,
+size_t size, clang::TagTypeKind ttk,
+clang::MSInheritanceAttr::Spelling inheritance) {
 
-  // Ignore unnamed-tag UDTs.
-  name = DropNameScope(name);
-  if (name.empty())
-return nullptr;
-
-  clang::DeclContext *decl_context = m_clang->GetTranslationUnitDecl();
-
-  lldb::AccessType access =
-  (ttk == clang::TTK_Class) ? lldb::eAccessPrivate : lldb::eAccessPublic;
-
-  ClangASTMetadata metadata;
-  metadata.SetUserID(type_uid.toOpaqueId());
-  metadata.SetIsDynamicCXXType(false);
-
-  CompilerType ct =
-  m_clang->CreateRecordType(decl_context, access, name.str().c_str(), ttk,
-lldb::eLanguageTypeC_plus_plus, &metadata);
-  lldbassert(ct.IsValid());
+  CompilerType ct = CreateClangDeclFromMangledName(*m_clang, unique_name);
 
   clang::CXXRecordDecl *record_decl =
   m_clang->GetAsCXXRecordDecl(ct.GetOpaqueQualType());
@@ -771,7 +756,7 @@
   // FIXME: Search IPI stream for LF_UDT_MOD_SRC_LINE.
   Declaration decl;
   return std::make_shared(type_uid.toOpaqueId(), m_clang->GetSymbolFile(),
-ConstString(name), size, nullptr,
+ct.GetTypeName(), size, nullptr,
 LLDB_INVALID_UID, Type::eEncodingIsUID, decl,
 ct, Type::eResolveStateForward);
 }
@@ -782,14 +767,15 @@
 
   clang::MSInheritanceAttr::Spelling inheritance =
   GetMSInheritance(m_index->tpi().typeCollection(), cr);
-  return CreateClassStructUnion(type_uid, cr.getName(), cr.getSize(), ttk,
-inheritance);
+  return CreateClassStructUnion(type_uid, cr.getName(), cr.getUniqueName(),
+cr.getSize(), ttk, inheritance);
 }
 
 lldb::TypeSP SymbolFileNativePDB::CreateTagType(PdbSymUid type_uid,
 const UnionRecord &ur) {
   return CreateClassStructUnion(
-  type_uid, ur.getName(), ur.getSize(), clang::TTK_Union,
+  type_uid, ur.getName(), ur.getUniqueName(), ur.getSize(),
+  clang::TTK_Union,
   clang::MSInheritanceAttr::Spelling::Keyword_single_inheritance);
 }
 
Index: lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.h
===
--- /dev/null
+++ lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.h
@@ -0,0 +1,30 @@
+//===-- MangledAST.h *- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is dist

[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

2018-11-03 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a comment.

Bleh, I think I found a problem with this approach.  Since we assume everything 
is a namespace, it can lead to ambiguities.  I think we need to actually 
consult the debug info while parsing components of the mangled name.  For 
example, suppose you have this code:

  namespace NS1 {
struct Tag2 {
  class TagRecord {};
};
  }
  
  NS1::Tag2::TagRecord ATR;
  NS1::Tag2 T2;

And in LLDB you run the following commands:

  (lldb) target variable ATR
  (lldb) target variable T2

We demangle the first name and create an AST from it, but we don't know what 
`Tag2` is so we assume it's a namespace and we create a `NamespaceDecl`.  Then 
we get to the second one we actually know what it is because we have a precise 
debug info record for it so we know it's a struct so we create a 
`CXXRecordDecl` for it.  So now we end up creating an invalid AST.  Clang will 
accept it, but it's going to lead to ambiguities on name lookup and create 
problems down the line.  So, I think what we should do instead is ask the debug 
info "Do you know what the type named `NS1` is in the global scope?"  If no, 
it's a namespace.  Otherwise we create (or lookup) the appropriate decl.  Then 
we can repeat this at each step along the way.

BTW, when I run those above 2 commands with the patch as it is above, here is 
the output I get.  And you can see that there are two conflicting decls.

  D:\src\llvmbuild\ninja-x64>"bin\lldb" "-f" 
"D:\src\llvmbuild\ninja-x64\tools\lldb\lit\SymbolFile\NativePDB\Output\ast-reconstruction.cpp.tmp.exe"
  (lldb) target create 
"D:\\src\\llvmbuild\\ninja-x64\\tools\\lldb\\lit\\SymbolFile\\NativePDB\\Output\\ast-reconstruction.cpp.tmp.exe"
  Current executable set to 
'D:\src\llvmbuild\ninja-x64\tools\lldb\lit\SymbolFile\NativePDB\Output\ast-reconstruction.cpp.tmp.exe'
 (x86_64).
  (lldb) target variable ATR
  (NS1::Tag2::TagRecord) ATR = {}
  (lldb) target variable T2
  (NS1::Tag2) T2 = {}
  (lldb) target modules dump ast
  TranslationUnitDecl 0x2247dff6fd8 <> 
  |-NamespaceDecl 0x2247dff7898 <>  NS1
  | |-NamespaceDecl 0x2247dff7918 <>  Tag2
  | | `-CXXRecordDecl 0x2247dff7998 <>  class 
TagRecord definition
  | |   |-DefinitionData pass_in_registers empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
  | |   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  | |   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  | |   | |-MoveConstructor exists simple trivial needs_implicit
  | |   | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  | |   | |-MoveAssignment exists simple trivial needs_implicit
  | |   | `-Destructor simple irrelevant trivial needs_implicit
  | |   `-MSInheritanceAttr 0x2247dff7a60 <> Implicit 
__single_inheritance
  | `-CXXRecordDecl 0x2247dff7b08 <>  struct Tag2 
definition
  |   |-DefinitionData pass_in_registers empty aggregate standard_layout 
trivially_copyable pod trivial literal has_constexpr_non_copy_move_ctor 
can_const_default_init
  |   | |-DefaultConstructor exists trivial constexpr needs_implicit 
defaulted_is_constexpr
  |   | |-CopyConstructor simple trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveConstructor exists simple trivial needs_implicit
  |   | |-CopyAssignment trivial has_const_param needs_implicit 
implicit_has_const_param
  |   | |-MoveAssignment exists simple trivial needs_implicit
  |   | `-Destructor simple irrelevant trivial needs_implicit
  |   `-MSInheritanceAttr 0x2247dff7bd0 <> Implicit 
__single_inheritance
  `-
  Dumping clang ast for 1 modules.


https://reviews.llvm.org/D54053



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

2018-11-04 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: vsk.
zturner added a comment.

Unfortunately then color output is impossible. Where else would the output
be expected to go?


https://reviews.llvm.org/D54072



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


[Lldb-commits] [PATCH] D54072: Add a command to dump a module's clang ast.

2018-11-04 Thread Zachary Turner via Phabricator via lldb-commits
zturner added a subscriber: davide.
zturner added a comment.

Ok so probably this commands output will not go to a log file when logging
is enabled? I can’t think of any other side effects.

Given that the immediate use case for this is interactive investigation and
testing, the first of which is helped by color output and the second of
which doesn’t need logging, what do you think of allowing it as is?


https://reviews.llvm.org/D54072



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits


  1   2   3   4   5   6   7   8   >