[PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Changes look fine to me. I would like someone that specializes in the 
expression parser to give the final ok though.




Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp:551
 const LayoutInfo ) {
+
   m_record_decl_to_layout_map.insert(std::make_pair(decl, layout));

revert whitespace only changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143347

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

[PATCH] D116113: Add LLDB synthetic child and summary scripts for llvm::SmallVector, llvm::Optional, llvm::ErrorOr and llvm::Expected.

[Greg Clayton via Phabricator via cfe-commits]

clayborg created this revision.
clayborg added reviewers: labath, teemperor, aprantl.
Herald added subscribers: usaxena95, arphaman.
clayborg requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This allows us to see the contents of the SmallVector just like std::vector and 
also adds a summary string that shows the size of the array.

For llvm::Optional, the value will show as llvm::None when it has no value, or 
it will show the value of the type T.

For llvm::Error or, it will show the std::error_code as a structure or the 
value if it has a value.

For llvm::Expected, it will show the dynamic classname of the contained error, 
or the value.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D116113

Files:
  clang/utils/ClangDataFormat.py

Index: clang/utils/ClangDataFormat.py
===
--- clang/utils/ClangDataFormat.py
+++ clang/utils/ClangDataFormat.py
@@ -22,140 +22,408 @@
 import lldb
 
 def __lldb_init_module(debugger, internal_dict):
-	debugger.HandleCommand("type summary add -F ClangDataFormat.SourceLocation_summary clang::SourceLocation")
-	debugger.HandleCommand("type summary add -F ClangDataFormat.QualType_summary clang::QualType")
-	debugger.HandleCommand("type summary add -F ClangDataFormat.StringRef_summary llvm::StringRef")
+debugger.HandleCommand("type summary add -F ClangDataFormat.SourceLocation_summary clang::SourceLocation")
+debugger.HandleCommand("type summary add -F ClangDataFormat.QualType_summary clang::QualType")
+debugger.HandleCommand("type summary add -F ClangDataFormat.StringRef_summary llvm::StringRef")
+debugger.HandleCommand("type summary add -F ClangDataFormat.Optional_summary -x 'llvm::Optional<.*>'")
+debugger.HandleCommand("type summary add -F ClangDataFormat.SmallVector_summary -x 'llvm::SmallVector<.*>'")
+debugger.HandleCommand("type summary add -F ClangDataFormat.Expected_summary -x 'llvm::Expected<.*>'")
+debugger.HandleCommand("type summary add -F ClangDataFormat.ErrorOr_summary -x 'llvm::ErrorOr<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.Optional -x 'llvm::Optional<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.SmallVector -x 'llvm::SmallVector<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.Expected -x 'llvm::Expected<.*>'")
+debugger.HandleCommand("type synthetic add -l ClangDataFormat.ErrorOr -x 'llvm::ErrorOr<.*>'")
 
-def SourceLocation_summary(srcloc, internal_dict):
-	return SourceLocation(srcloc).summary()
+def SourceLocation_summary(valobj, internal_dict):
+return SourceLocation(valobj).summary()
 
-def QualType_summary(qualty, internal_dict):
-	return QualType(qualty).summary()
+def QualType_summary(valobj, internal_dict):
+return QualType(valobj).summary()
 
-def StringRef_summary(strref, internal_dict):
-	return StringRef(strref).summary()
+def StringRef_summary(valobj, internal_dict):
+return StringRef(valobj).summary()
+
+def Optional_summary(valobj, internal_dict):
+return Optional(valobj, internal_dict).summary()
+
+def SmallVector_summary(valobj, internal_dict):
+return SmallVector(valobj, internal_dict).summary()
+
+def Expected_summary(valobj, internal_dict):
+return Expected(valobj, internal_dict).summary()
+
+def ErrorOr_summary(valobj, internal_dict):
+return ErrorOr(valobj, internal_dict).summary()
 
 class SourceLocation(object):
-	def __init__(self, srcloc):
-		self.srcloc = srcloc
-		self.ID = srcloc.GetChildAtIndex(0).GetValueAsUnsigned()
-		self.frame = srcloc.GetFrame()
-	
-	def offset(self):
-		return getValueFromExpression(self.srcloc, ".getOffset()").GetValueAsUnsigned()
-
-	def isInvalid(self):
-		return self.ID == 0
-
-	def isMacro(self):
-		return getValueFromExpression(self.srcloc, ".isMacroID()").GetValueAsUnsigned()
-
-	def isLocal(self, srcmgr_path):
-		return self.frame.EvaluateExpression("(%s).isLocalSourceLocation(%s)" % (srcmgr_path, getExpressionPath(self.srcloc))).GetValueAsUnsigned()
-
-	def getPrint(self, srcmgr_path):
-		print_str = getValueFromExpression(self.srcloc, ".printToString(%s)" % srcmgr_path)
-		return print_str.GetSummary()
-
-	def summary(self):
-		if self.isInvalid():
-			return ""
-		srcmgr_path = findObjectExpressionPath("clang::SourceManager", self.frame)
-		if srcmgr_path:
-			return "%s (offset: %d, %s, %s)" % (self.getPrint(srcmgr_path), self.offset(), "macro" if self.isMacro() else "file", "local" if self.isLocal(srcmgr_path) else "loaded")
-		return "(offset: %d, %s)" % (self.offset(), "macro" if self.isMacro() else "file")
+def __init__(self, srcloc):
+self.srcloc = srcloc
+self.ID = srcloc.GetChildAtIndex(0).GetValueAsUnsigned()
+self.frame = srcloc.GetFrame()
+
+def offset(self):
+return getValueFromExpression(self.srcloc, ".getOffset()").GetValueAsUnsigned()
+
+def 

[PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added inline comments.



Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1305
+
+  // If a function has an auto return type we need to find the defintion since
+  // that will have the deduced return type and adjust the FunctionDecl to

s/defintion/definition/



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2677
+
+  Symbol *defintion_class_symbol = nullptr;
+  if (m_objfile_sp) {

type



Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2689
+  DWARFUnit *unit = die.GetCU();
+  DWARFCompileUnit *dcu = llvm::cast_or_null(unit);
+  DWARFDIE other_die =

check dcu to ensure it isn't NULL


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

https://reviews.llvm.org/D105564

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

[PATCH] D27683: Prepare PrettyStackTrace for LLDB adoption

[Greg Clayton via Phabricator via cfe-commits]

clayborg added inline comments.



Comment at: llvm/trunk/lib/Support/PrettyStackTrace.cpp:92-93
+#elif defined(__APPLE__) && HAVE_CRASHREPORTER_INFO
+extern "C" const char *__crashreporter_info__
+__attribute__((visibility("hidden"))) = 0;
 asm(".desc ___crashreporter_info__, 0x10");

lol, if we still need the ___crashreporter_info__ symbol to be around, then it 
should be visible so that ReportCrash can find it... 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D27683

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

[PATCH] D83731: Add Debug Info Size to Symbol Status

[Greg Clayton via Phabricator via cfe-commits]

clayborg added inline comments.



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:55-61
+active_modules = self.vscode.get_active_modules()
+program_module = active_modules[program_basename]
+self.assertEqual(program_basename, program_module['name'])
+self.assertEqual(program, program_module['path'])
+self.assertIn('symbolFilePath', program_module)
+self.assertEqual(symbols_path, program_module['symbolFilePath'])
+self.assertIn('addressRange', program_module)

aelitashen wrote:
> clayborg wrote:
> > Unindent everything after the self.waitUntil()? Otherwise we are not 
> > testing the program, symbolFilePath and addressRange on symbols with size.
> When unindenting these codes, the dsym test for darwin fails as the symbol 
> paths don't match. One is using dysm path, one is using a.out.path.
Then we need to fix this function so that it does work. One thing to note in 
the dSYM case: dSYM files are bundles which means it is a directory that 
contains a file within it. So you might specify "/tmp/a.out.dSYM" as the path, 
but end up with "/tmp/a.out.dSYM/Context/Resources/DWARF/a.out" as the symbol 
path. So you could switch the symbol file path to use assertIn:

```
self.assertIn(symbols_path, program_module['symbolFilePath'])
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83731



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

[PATCH] D83731: Add Debug Info Size to Symbol Status

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Looks like there is an indentation issue in the test. See inline comments.




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:55-61
+active_modules = self.vscode.get_active_modules()
+program_module = active_modules[program_basename]
+self.assertEqual(program_basename, program_module['name'])
+self.assertEqual(program, program_module['path'])
+self.assertIn('symbolFilePath', program_module)
+self.assertEqual(symbols_path, program_module['symbolFilePath'])
+self.assertIn('addressRange', program_module)

Unindent everything after the self.waitUntil()? Otherwise we are not testing 
the program, symbolFilePath and addressRange on symbols with size.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83731



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

[PATCH] D83731: Add Debug Info Size to Symbol Status

[Greg Clayton via Phabricator via cfe-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83731



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

[PATCH] D83731: Add Debug Info Size to Symbol Status

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just a space before the '(' of the debug info size.




Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:357
+  std::ostringstream oss;
+  oss << "(";
+  oss << std::fixed << std::setprecision(1);

Need as space before the '(' character as we append this string to the "Symbols 
loaded." string.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83731



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

[PATCH] D83731: Add Debug Info Size to Symbol Status

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So you should revert any clang format changes that are not in modified lines of 
source, mostly revert a lot of lines in lldb-vscode.cpp.




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:19
 
-
-@skipIfWindows
-@skipUnlessDarwin
-@skipIfRemote
-def test_modules_event(self):
-program_basename = "a.out.stripped"
+def get_modules(self, program_basename):
 program= self.getBuildArtifact(program_basename)

Name should be more descriptive. Maybe "setup_test_common" is a better name



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:19
 
-
-@skipIfWindows
-@skipUnlessDarwin
-@skipIfRemote
-def test_modules_event(self):
-program_basename = "a.out.stripped"
+def get_modules(self, program_basename):
 program= self.getBuildArtifact(program_basename)

clayborg wrote:
> Name should be more descriptive. Maybe "setup_test_common" is a better name
So all of these tests can re-use this function if we switch it up a bit. Here 
is what I was thinking:

```
def run_test(self, symbol_basename, expect_debug_info_size):
program_basename = "a.out.stripped"
program = self.getBuildArtifact(program_basename)
self.build_and_launch(program)
functions = ['foo']
breakpoint_ids = self.set_function_breakpoints(functions)
self.assertEquals(len(breakpoint_ids), len(functions), 'expect one 
breakpoint')
self.continue_to_breakpoints(breakpoint_ids)
active_modules = self.vscode.get_active_modules()
program_module = active_modules[program_basename]
self.assertIn(program_basename, active_modules, '%s module is in active 
modules' % (program_basename))
self.assertIn('name', program_module, 'make sure name is in module')
self.assertEqual(program_basename, program_module['name'])
self.assertIn('path', program_module, 'make sure path is in module')
self.assertEqual(program, program_module['path'])
self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')
self.assertEqual('Symbols not found.', program_module['symbolStatus'])
symbols_path = self.getBuildArtifact(symbol_basename)
self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" "%s"' % 
(program, symbols_path)))

def checkSymbolsLoaded():
active_modules = self.vscode.get_active_modules()
program_module = active_modules[program_basename]
return 'Symbols loaded.' == program_module['symbolStatus']

def checkSymbolsLoadedWithSize():
active_modules = self.vscode.get_active_modules()
program_module = active_modules[program_basename]
symbolsStatus = program_module['symbolStatus']
symbol_regex = re.compile(r"Symbols loaded. 
\([0-9]+(\.[0-9]*)?[KMG]?B\)")
return symbol_regex.match(program_module['symbolStatus'])

if expect_debug_info_size:
self.waitUntil(checkSymbolsLoadedWithSize)
else:
self.waitUntil(checkSymbolsLoaded)
```

Then your tests would be:
```
@skipIfWindows
@skipIfRemote
def test_module_event(self):
# Mac or linux.

# On mac, if we load a.out as our symbol file, we will use DWARF with .o 
files and we will
# have debug symbols, but we won't see any debug info size because all of 
the DWARF
# sections are in .o files.

# On other platforms, we expect a.out to have debug info, so we will expect 
a size.
expect_debug_info_size = platform.system() != 'Darwin'
return run_test("a.out", expect_debug_info_size)

@skipIfWindows
@skipUnlessDarwin
@skipIfRemote
def test_module_event_dsym(self):
# Darwin only test with dSYM file.

# On mac, if we load a.out.dSYM as our symbol file, we will have debug 
symbols and we
# will have DWARF sections added to the module, so we will expect a size.
return run_test("a.out.dSYM", True)
```
This should cover both mac and non windows correctly.



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:20
+def get_modules(self, program_basename):
 program= self.getBuildArtifact(program_basename)
 self.build_and_launch(program)

add a space after program:

```
program = self.getBuildArtifact(program_basename)
```



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:32
+@skipIfWindows
+@skipUnlessDarwin
+@skipIfRemote

Remove @skipUnlessDarwin here. This should work on linux.



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:59
+# it will end up using the executable and the .o files as the debug 
info.
+# We won't see any debug information size for this case since all of 
the debug info 

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

In D82477#2145565 , @MaskRay wrote:

> Hi, your git commit contains extra Phabricator tags. You can drop 
> `Reviewers:` `Subscribers:` `Tags:` and the text `Summary:` from the git 
> commit with the following script:
>
>   arcfilter () {
>   arc amend
>   git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} 
> /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ 
> {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
>   }
>   
>
> `Reviewed By: ` is considered important by some people. Please keep the tag. 
> (`--date=now` is my personal preference (author dates are usually not useful. 
> Using committer dates can make log almost monotonic in time))
>
> `llvm/utils/git/pre-push.py` can validate the message does not include 
> unneeded tags.


Can we modify this script to remove the unneeded tags instead of detecting it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg accepted this revision.
clayborg added a comment.
This revision is now accepted and ready to land.

Looks good!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Just test for paths and this will be good to go!




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:35
+self.assertEqual(program_basename, program_module['name'])
+self.assertIn('path', program_module, 'make sure path is in module')
+self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')

add a check for the path:
```
self.assertEqual(program, program_module['path'])
```



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:42
+program_module = active_modules[program_basename]
+self.assertEqual(program_basename, program_module['name'])
+self.assertEqual('Symbols loaded.', program_module['symbolStatus'])

check 'path' again here.



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:47
+self.assertIn('addressRange', program_module)
\ No newline at end of file


add a newline


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

So looks like you didn't use "git commit --amend -a" again here. These changes 
are incremental changes on your patch which hasn't been submitted yet?




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:21
+program_basename = "a.out.stripped"
+program= self.getBuildArtifact(program_basename)
 self.build_and_launch(program)

add a space after "program"



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:33-34
+program_module = active_modules[program_basename]
+self.assertIn('name', program_module, 'make sure name is in module')
+self.assertEqual(program_basename, program_module['name'])
+self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')

Do a similar check for ='path'
```
self.assertIn('name', program_module, 'make sure "name" is in module')
self.assertEqual(program_basename, program_module['name'])
self.assertIn('path', program_module, 'make sure "path" is in module')
self.assertEqual(program, program_module['path'])
```
In general, make sure you test every key that you have added support for.



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:35
+self.assertEqual(program_basename, program_module['name'])
+self.assertTrue('symbolFilePath' not in program_module, 'Make sure 
a.out.stripped has no debug info')
+symbol_path = self.getBuildArtifact("a.out")

Check for the symbol status here:
```
self.assertEqual('Symbols not found.', program_module['symbolStatus'])
```



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:40
+program_module = active_modules[program_basename]
+self.assertEqual(program_basename, program_module['name'])
+self.assertEqual('Symbols loaded.', program_module['symbolStatus'])

verify 'path' again



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:346
+object.try_emplace("symbolFilePath", symbol_path);
   }
   std::string loaded_addr = std::to_string(

Add an else clause here:

```
} else {
  object.try_emplace("symbolStatus", "Symbols not found.");
}
```



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:352-357
+  uint32_t num_versions = module.GetVersion(version_nums, 
sizeof(version_nums)/sizeof(uint32_t));
+  for (uint32_t i=0; ihttps://reviews.llvm.org/D82477/new/

https://reviews.llvm.org/D82477



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

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

See inline comments. Changes needed are:

- test for eBroadcastBitSymbolsLoaded
- add "version" to module info
- test all data we expect in module info ('name', 'symbolFilePath', 'version', 
'symbolStatus', 'addressRange')




Comment at: lldb/test/API/tools/lldb-vscode/module/Makefile:3
+
+include Makefile.rules

We need to test eBroadcastBitSymbolsLoaded in this test. I would suggest we 
make an "a.out.stripped"  (see the makefile form 
lldb/test/API/lang/objc/objc-ivar-stripped/Makefile for how to do this).



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:20
+def test_modules_event(self):
+program= self.getBuildArtifact("a.out")
+self.build_and_launch(program)

eBroadcastBitSymbolsLoaded needs to be tested: change this line to:
```
program_basename = 'a.out.stripped'
program = self.getBuildArtifact(program_basename)
```
after modifying the Makefile as suggested above.




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:27
+self.continue_to_breakpoints(breakpoint_ids)
+self.assertTrue('a.out' in self.vscode.get_active_modules(), 
+'Module: a.out is loaded')

Cached active modules, and use assertIn here and switch to "a.out.stripped", 
and verify the 'name' is in module info and that path is correct:
```
active_modules = self.vscode.get_active_modules()
self.assertIn(program_basename, active_modules, '%s module is in active 
modules' % (program_basename))
program_module = active_modules[program_basename]
self.assertIn('name', program_module, 'make sure 'path' key is in module')
self.assertEqual(program, program_module['name'])
```



Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:29-30
+'Module: a.out is loaded')
+self.assertTrue('symbolFilePath' in 
self.vscode.get_active_modules()['a.out'],
+'Symbol exists')

use self.assertIn(...) and use the cached "active_modules" variable:
```
self.assertIn('symbolFilePath', program_module, 'Make sure 'symbolFilePath' key 
is in module info')
self.assertEqual(program, program_module['symbolFilePath'])
```
Then you need to verify all other information that you put into the Module like 
"symbolStatus" and "addressRange".




Comment at: lldb/test/API/tools/lldb-vscode/module/TestVSCode_module.py:29-30
+'Module: a.out is loaded')
+self.assertTrue('symbolFilePath' in 
self.vscode.get_active_modules()['a.out'],
+'Symbol exists')

clayborg wrote:
> use self.assertIn(...) and use the cached "active_modules" variable:
> ```
> self.assertIn('symbolFilePath', program_module, 'Make sure 'symbolFilePath' 
> key is in module info')
> self.assertEqual(program, program_module['symbolFilePath'])
> ```
> Then you need to verify all other information that you put into the Module 
> like "symbolStatus" and "addressRange".
> 
Now we would send a LLDB command line command to load the "a.out" symbols:

```
symbols = self.getBuildArtifact("a.out")
response = self.vscode.request_evaluate('`%s' % ('target symbols add -s "%s" 
"%s"' % (program, symbols)))
```
This will cause LLDB to load the symbol file "a.out" for the executable 
"a.out.stripped". This should cause the eBroadcastBitSymbolsLoaded notification 
to fire and the modules to get updated. Then you will get the active modules 
again and verify:
- program is still "program" (the full path to the a.out.stripped binary)
- 'symbolFilePath' is equal to our "symbols" variable from above (full path to 
"a.out")
- 'symbolStatus' is now set to 'Symbols Loaded.'
- 'addressRange' is still what we expect




Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:344-345
+module.GetSymbolFileSpec().GetPath(symbol_path_arr, 
sizeof(symbol_path_arr));
+std::string symbol_path(symbol_path_arr);
+object.try_emplace("symbolFilePath", symbol_path);
+  }

Do we only want to try and add this "symbolFilePath" key if the path differs 
from "module_path"?



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:351
+  return llvm::json::Value(std::move(object));
+}
+

We should also fetch the version number from the module and populate the 
"version" key. To get the version number from a module you can use:

```
uint32_t version_nums[5];
const uint32_t num_versions = module.GetVersion(version_nums, 
sizeof(version_nums));
std::string version_str;
for (uint32_t i=0; ihttps://reviews.llvm.org/D82477/new/

https://reviews.llvm.org/D82477



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Here is a makefile that does stripping:

llvm-project/lldb/test/API/lang/objc/objc-ivar-stripped/Makefile

Then when creating the target, use a.out.stripped:

  exe = self.getBuildArtifact("a.out.stripped")
  symbols = exe = self.getBuildArtifact("a.out")
  target = self.dbg.CreateTarget(exe)

Then after you get the modules, you can do:

  self.dbg.HandleCommand('target symbols add "%s"' % (symbols))

And that should trigger the symbols added notification and then you can fetch 
the modules again and verify that there is now a symbol file for 
"a.out.stripped".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[PATCH] D82477: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

We need to add a test for the symbols added target notification. See my 
previous comment on stripping a.out to a.out.stripped and then using 
"a.out.stripped" as the main executable.




Comment at: lldb/test/API/tools/lldb-vscode/module/main.cpp:2-9
+int multiply(int x, int y) {
+  return x * y; // breakpoint 1
+}
+
+int main(int argc, char const *argv[]) {
+  int result = multiply(argc, 20);
+  return result < 0;

We can probably simplify this to just be:

```
int main(int argc, char const *argv[]) {
  return 0; // breakpoint 1
}
```



Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:337-340
+std::string(module.GetFileSpec().GetFilename()));
+  std::string module_path = std::string(module.GetFileSpec().GetDirectory()) +
+"/" +
+std::string(module.GetFileSpec().GetFilename());

Let LLDB take care of the directory delimiter. Otherwise this will be bad on 
windows:
```
char module_path[PATH_MAX];
module.GetFileSpec().GetPath(module_path, sizeof(module_path));
```




Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:344-346
+std::string symbol_path =
+std::string(module.GetSymbolFileSpec().GetDirectory()) + "/" +
+std::string(module.GetSymbolFileSpec().GetFilename());

Let LLDB take care of the directory delimiter. Otherwise this will be bad on 
windows:

```
char symbol_path[PATH_MAX];
module.GetSymbolFileSpec().GetPath(module_path, sizeof(symbol_path));
```




Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:349-350
+  }
+  std::string loaded_addr = std::to_string(
+  module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target));
+  object.try_emplace("addressRange", loaded_addr);

We need to make sure the address returned is valid and we want the string value 
to be in hex. Are we sure std::to_string() will create a hex number? If not we 
can use sprintf:

```
uint64_t load_addr = 
module.GetObjectFileHeaderAddress().GetLoadAddress(g_vsc.target);
if (load_addr != LLDB_INVALID_ADDRESS) {
  char load_addr_str[64];
  snprintf(load_addr_str, sizeof(load_addr_str), "0x%016" PRIx64, load_addr);
  object.try_emplace("addressRange", load_addr_str);
}
```



Comment at: lldb/tools/lldb-vscode/JSONUtils.h:241
 
+/// Converts Module Event to a Visual Studio Code "Module"
+///

Might be better with text like:

```
/// Converts a LLDB module to a VS Code DAP module for use in "modules" events.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82477



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

[PATCH] D82505: [lldb-vscode] Add Support for Module Event

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

This seems like a continuation of https://reviews.llvm.org/D82477. I would 
close this one down by selecting abandon revision, and then click the "update 
diff" button in D82477  and upload your new 
patch. I am guessing you did this manually and not with "arc diff"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82505



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

[PATCH] D78874: [clang] Add vendor identity for Hygon Dhyana processor

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

I am not sure I am the right person to review? I work on LLDB and LLVM mostly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78874



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

[PATCH] D69933: [ASTImporter] Limit imports of structs

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Clang AST contexts know how to complete types and is done via the external AST 
source code that will ask a type to complete itself. Each object file has an 
AST that knows how to lazily complete a type when and only when it is needed. 
Each object file also only knows about the type information in the binary 
itself. So if we have a forward declaration to "Foo" with something like 
"struct Foo;" that is all the object file AST will ever know about this type. 
This is required because each module can be re-used on subsequent debug 
sessions if they haven't changed. So if we have a forward declaration for "Foo" 
in the AST for "bbb.so" that is ok. We don't want to copy some definition for 
"Foo" from "foo.so" over into bbb.so's AST context because if we run again and 
we get a new foo.so we would have to reload bbb.so because its copy of "Foo" 
might be out of date. And we would need to track these interactions.

When we run expressions, we create a new AST and copy types as needed. It would 
be great if the AST importer only copy over forward declarations of types that 
can be completed later and can also complete types only as needed when asked.

If I understand correctly that is what this patch is trying to do. Seems like 
we have a code path that is copying over the type and also completing it 
sometimes without being asked which should be fixed. If we do fix this, complex 
expressions become a lot faster. To do this right we should always import 
forward declarations from the source, and be able to complete the new types in 
the destination as needed. As teemperor said, the source AST should not be 
mutated in any way. We should track all of this in the importer and know where 
we should try to complete the type from.

When using expression AST contexts it **is** ok to try and import "Foo" from 
bbb.so since that where is where we first saw the type, and if we aren't 
successful, we can grab the definition from anywhere else in the debug session. 
Since each expression has its own AST, it **is** ok to get the type from 
anywhere. When searching for this type we should start in the current 
lldb_private::Block, their pareent blocks, then the file, then the module and 
then all modules. I think that works today already, but I am not sure if this 
works for a type "Foo" that is mentioned in a type from a file that doesn't 
have a complete definition. for example if bbb.so contains:

  struct Bar : public Foo {...};

Due to "-flimit-debug-info" the definition for Foo might be forward declared 
(if the vtable for Foo isn't in the current binary) and not included in this 
binary. This won't happen on darwin since the default is 
-fno-limit-debug-info". The DWARF parser knows how to work around this issue 
when creating the type in the AST for bbb.so, but when we run an expression 
with this type, we want to be able to have an AST type from bbb.so with an 
incomplete definition for "Foo" that we complete during AST import. To do this, 
we will need to use metadata in the bbb.so AST to indicate we have no 
definition for this type when we normally would require one and be able to 
complete the type from another source.

So quick things to stick to:

- no modification of the source AST context
- importer can track anything it needs to in order to complete types in complex 
situations as mentioned above
  - need metadata that tracks types that need to be complete but aren't in the 
debug info so they can be properly imported for expressions ("struct Bar: 
public Foo {}", not ok for Foo to not be complete but we allow it for object 
file AST contexts otherwise clang crashes us)
  - legal forward declarations should be able to be imported as needed even if 
the AST form the original source doesn't have a complete type ("struct Bar { 
Foo *foo_ptr; }", ok for Foo to be forward declared here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69933



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

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Actually, we might need to still emit some Apple tables as the objective C and 
namespace tables might still be needed even with the DWARF5 tables.


Repository:
  rC Clang

https://reviews.llvm.org/D51576



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

[PATCH] D51576: Enable DWARF accelerator tables by default when tuning for lldb (-glldb => -gpubnames)

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

We want to ensure that both Apple and DWARF5 tables never get generated though. 
That would waste a lot of space. I would say if DWARF5 tables are enabled, then 
we need ensure we disable Apple tables.


Repository:
  rC Clang

https://reviews.llvm.org/D51576



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

[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Each lldb.SBValue has accessors for the stuff in an execution context:

``

  lldb::SBTarget GetTarget();
  lldb::SBProcess GetProcess();
  lldb::SBThread GetThread();
  lldb::SBFrame GetFrame();

  You could keep a global map of process ID to diagtool if you want?
  
  What are you thinking of using this for?


https://reviews.llvm.org/D36347



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

[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Looks good.


https://reviews.llvm.org/D36347



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

[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Greg Clayton via Phabricator via cfe-commits]

clayborg accepted this revision.
clayborg added inline comments.



Comment at: utils/clangdiag.py:117
+disable(exe_ctx)
+else:
+getDiagtool(exe_ctx.GetTarget(), args.path[0])

hintonda wrote:
> clayborg wrote:
> > should probably verify that this 'diagtool' with:
> > 
> > ```
> > elif args.subcommands == 'diagtool':
> > ```
> > and add an else that creates an error:
> > ```
> > else:
> > result.SetError("invalid subcommand '%s'" % (args.subcommands))
> > ```
> This is already handled by `argparser`, which will raise an exception if the 
> first parameter isn't one of (enable, disable, daigtool).  That's the benefit 
> of using `subparsers`.  So, by the time you get to this point, it must be 
> "diagtool".
> 
> However, I think I should probably verify the path past actually exists.  
> Plus, I think I could add a better exception to alert the user how to fix the 
> problem if diagtool couldn't be found in the callback.  Suggestions welcome.
nice, I didn't realized that argparser would protect against that. Checking the 
file exists would be a good idea.


https://reviews.llvm.org/D36347



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

[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Greg Clayton via Phabricator via cfe-commits]

clayborg added inline comments.



Comment at: utils/clangdiag.py:62
+
+def enable(debugger, args):
+# Always disable existing breakpoints

Pass exe_ctx or target into this instead of the debugger (see Jim's comment on 
execution contexts below.



Comment at: utils/clangdiag.py:78
+print('diagtool not found along side %s' % exe)
+return
+

No need. just a suggestion. Is this information available in the main 
executable as any kind of debug info? If so you can read it from there. If it 
is always in a stand alone separate file, then what you have is ok.



Comment at: utils/clangdiag.py:87
+def disable(debugger):
+global target
+global diagtool

jingham wrote:
> clayborg wrote:
> > remove and use:
> > ```
> > target = debugger.GetSelectedTarget()
> > ```
> See my comment.  Don't use selected targets, use the command form that takes 
> an SBExecutionContext.  That's been around for more than a couple of years 
> now, so it's pretty safe to use.
Just pass the target or exe_ctx into this function then instead of "debugger".



Comment at: utils/clangdiag.py:92
+for bp in Breakpoints:
+target.BreakpointDelete(bp.GetID())
+target = None

nice!



Comment at: utils/clangdiag.py:100
+# Use the Shell Lexer to properly parse up command options just like a
+# shell would
+command_args = shlex.split(command)

Great idea. Forgot about the execution context variant. The "exe_ctx" is a 
lldb.SBExecutionContext object. You get your target using:

```
target = exe_ctx.GetTarget()
```



https://reviews.llvm.org/D36347



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

[PATCH] D36347: New lldb python module for adding diagnostic breakpoints

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Looks good. A little bit of cleanup. Let me know what you think of the comments.




Comment at: utils/clangdiag.py:17
+import os
+from subprocess import check_output as qx;
+

I would rather just do:

```
import subprocess
```

Then later use:

```
subprocess.check_output(...)
```

It makes the code easier to read.



Comment at: utils/clangdiag.py:20
+# State
+Breakpoints = []
+target = None

Remove? See inlined comment for line 92



Comment at: utils/clangdiag.py:21
+Breakpoints = []
+target = None
+diagtool = None

I would avoid making this a global. It will keep the target alive since it has 
a strong reference count to the underlying lldb_private::Target.



Comment at: utils/clangdiag.py:52
+
+global target
+global diagtool

Get the target from the frame instead of grabbing it from a global:
```
target = frame.thread.process.target
```
or without the shortcuts:
```
target = frame.GetThread().GetProcess().GetTarget()
```



Comment at: utils/clangdiag.py:66
+
+global target
+global diagtool

Always grab the target and don't store it in a global. so just remove this line



Comment at: utils/clangdiag.py:75-78
+diagtool = os.path.join(exe.GetDirectory(), 'diagtool')
+if not os.path.exists(diagtool):
+print('diagtool not found along side %s' % exe)
+return

Allow "diagtool" to be set from an option maybe? This would require the options 
to be passed into this function as an argument. If it doesn't get set, then 
modify the options to contain this default value? Then this error message can 
just complain about the actual path:

```
print('diagtool "%s" doesn't exist' % diagtool)
```



Comment at: utils/clangdiag.py:80
+
+bp = target.BreakpointCreateByName('DiagnosticsEngine::Report')
+bp.SetScriptCallbackFunction('clangdiag.setDiagBreakpoint')

Add name to breakpoint? See inlined comment at line 92



Comment at: utils/clangdiag.py:87
+def disable(debugger):
+global target
+global diagtool

remove and use:
```
target = debugger.GetSelectedTarget()
```



Comment at: utils/clangdiag.py:91-92
+# Remove all diag breakpoints.
+for bp in Breakpoints:
+target.BreakpointDelete(bp.GetID())
+target = None

Another way to do this would be to give the breakpoints you create using 
"target.BreakpointCreateXXX" to have a name when you create them:

```
bp = target.BreakpointCreateByName('DiagnosticsEngine::Report')
bp.AddName("llvm::Diagnostic")
```

Then here you can iterate over all breakpoints in the target:

```
for bp in target.breakpoint_iter():
  if bp.MatchesName("llvm::Diagnostic"):
target.BreakpointDelete(bp.GetID())
```

Then you don't need a global list?



https://reviews.llvm.org/D36347



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

[PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

If you want to run the script from the command line, then it is necessary. If 
it is run from within LLDB it will just work. I like to have my LLDB python 
scripts work both ways.

This might be better implemented as a new command that gets installed and can 
be used within LLDB. See:

http://llvm.org/svn/llvm-project/lldb/trunk/examples/python/cmdtemplate.py


https://reviews.llvm.org/D36347



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

[PATCH] D36347: Add new script to launch lldb and set breakpoints for diagnostics all diagnostics seen.

[Greg Clayton via Phabricator via cfe-commits]

clayborg added a comment.

Please do convert to python. Just know that you can use "lldb -P" to get the 
python path that is needed in order to do "import lldb" in the python script. 
So you can try doing a "import lldb", and if that fails, catch the exception, 
run "lldb -P", add that path to the python path:

  try:
  # Just try for LLDB in case the lldb module is already in the python 
search
  # paths
  import lldb
  except ImportError:
  # We failed to import lldb automatically. Run lldb with the -P option so
  # it tells us the python path we should use.
  lldb_py_dirs = list()
  (status, output) = commands.getstatusoutput("lldb -P")
  dir = os.path.realpath(output)
  if status == 0 and os.path.isdir(dir):
  lldb_py_dirs.append(dir)
  success = False
  for lldb_py_dir in lldb_py_dirs:
  if os.path.exists(lldb_py_dir):
  if not (sys.path.__contains__(lldb_py_dir)):
  sys.path.append(lldb_py_dir)
  try:
  import lldb
  except ImportError:
  pass
  else:
  success = True
  break
  if not success:
  print("error: couldn't locate the 'lldb' module, please set "
"PYTHONPATH correctly")
  sys.exit(1)


https://reviews.llvm.org/D36347



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