[Lldb-commits] [PATCH] D111454: Move TargetRegistry.(h|cpp) from Support to MC

[Lang Hames via Phabricator via lldb-commits]

lhames added a comment.

Yeah -- this seems like a good idea to me. Thanks Reid!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111454

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

[Lldb-commits] [PATCH] D90789: [docs] Update DebuggingJITedCode page after fix in LLDB

[Lang Hames via Phabricator via lldb-commits]

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

Belatedly -- LGTM.

I think an orc-greedy mode for LLI sounds like a great idea. Also, once we have 
it we should probably make it the default mode instead of MCJIT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90789

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

[Lldb-commits] [PATCH] D47551: [IRMemoryMap] Fix the alignment adjustment in Malloc

[Lang Hames via Phabricator via lldb-commits]

lhames added a comment.

LGTM.

I haven't looked at process memory management. How hard would your FIXME be to 
implement?

- Lang.


https://reviews.llvm.org/D47551



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

[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[Lang Hames via Phabricator via lldb-commits]

lhames accepted this revision.
lhames added a comment.

Committed in r323163.




Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2106
+// a vtable entry) from overloads (which require distinct entries).
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.

clayborg wrote:
> I don't like things that can crash when asserts are off. I don't see why we 
> wouldn't just check this, If someone does pass in a method from on AST and 
> another from another AST, what will happen? Crash somewhere else? Why would 
> we risk crashing or misbehaving here when it is so easy to check and avoid. 
> I'll leave it at your discretion to do what you think is right though since I 
> know clang does this all over.
This can not crash unless someone has called it out-of-contract (like passing a 
nullptr into a routine that requires not-null). If user-input can cause a value 
that would have been used as a non-null argument to be null it should be 
guarded outside the method, not inside. I.e.:

  static void foo(Foo *f) {
assert(f && "f must not be null");
//...
  }

  auto *f = lookup(readline());
  if (!f)
return; // Can't call foo without a valid f so bail out
  foo(f);

If you're passing something that can't be null, there's no need for the check:

  Foo* bar(); // Never returns null
  foo(bar()); // this is fine.

The case in this patch is like the latter: isOverload is called with two 
CXXMethodDecls, where the second was found via a lookup on a CXXRecordDecl that 
is on the same context as the first CXXMethodDecl. There's no intervening user 
input that could mess with that, so we're safe.


Repository:
  rL LLVM

https://reviews.llvm.org/D41997



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

[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[Lang Hames via Phabricator via lldb-commits]

lhames updated this revision to Diff 130284.
lhames added a comment.

Updated to address review comments:

- assert changed to lldbassert
- comments added
- test case breakpoint comment simplified
- unused import removed from testcase
- testcase Makefile cleaned up


Repository:
  rL LLVM

https://reviews.llvm.org/D41997

Files:
  Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  Python/lldbsuite/test/expression_command/call-overridden-method/Makefile
  
Python/lldbsuite/test/expression_command/call-overridden-method/TestCallOverriddenMethod.py
  Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp

Index: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -40,6 +40,7 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 
@@ -2099,6 +2100,92 @@
   return template_param_infos.args.size() == template_param_infos.names.size();
 }
 
+// Checks whether m1 is an overload of m2 (as opposed to an override).
+// This is called by addOverridesForMethod to distinguish overrides (which share
+// a vtable entry) from overloads (which require distinct entries).
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  lldbassert(>getASTContext() == >getASTContext() &&
+ "Methods should have the same AST context");
+  clang::ASTContext  = m1->getASTContext();
+
+  const auto *m1Type =
+llvm::cast(
+  context.getCanonicalType(m1->getType()));
+
+  const auto *m2Type =
+llvm::cast(
+  context.getCanonicalType(m2->getType()));
+
+  auto compareArgTypes =
+[](const clang::QualType , const clang::QualType ) {
+  return context.hasSameType(m1p.getUnqualifiedType(),
+ m2p.getUnqualifiedType());
+};
+
+  return !std::equal(m1Type->param_type_begin(), m1Type->param_type_end(),
+ m2Type->param_type_begin(), compareArgTypes);
+}
+
+// If decl is a virtual method, walk the base classes looking for methods that
+// decl overrides. This table of overridden methods is used by IRGen to determine
+// the vtable layout for decl's parent class.
+static void addOverridesForMethod(clang::CXXMethodDecl *decl) {
+  if (!decl->isVirtual())
+return;
+
+  clang::CXXBasePaths paths;
+
+  auto find_overridden_methods =
+[decl](const clang::CXXBaseSpecifier *specifier, clang::CXXBasePath ) {
+  if (auto *base_record =
+  llvm::dyn_cast(
+specifier->getType()->getAs()->getDecl())) {
+
+clang::DeclarationName name = decl->getDeclName();
+
+// If this is a destructor, check whether the base class destructor is
+// virtual.
+if (name.getNameKind() == clang::DeclarationName::CXXDestructorName)
+  if (auto *baseDtorDecl = base_record->getDestructor()) {
+if (baseDtorDecl->isVirtual()) {
+  path.Decls = baseDtorDecl;
+  return true;
+} else
+  return false;
+  }
+
+// Otherwise, search for name in the base class.
+for (path.Decls = base_record->lookup(name); !path.Decls.empty();
+ path.Decls = path.Decls.slice(1)) {
+  if (auto *method_decl =
+llvm::dyn_cast(path.Decls.front()))
+if (method_decl->isVirtual() && !isOverload(decl, method_decl)) {
+  path.Decls = method_decl;
+  return true;
+}
+}
+  }
+
+  return false;
+};
+
+  if (decl->getParent()->lookupInBases(find_overridden_methods, paths)) {
+for (auto *overridden_decl : paths.found_decls())
+  decl->addOverriddenMethod(
+llvm::cast(overridden_decl));
+  }
+}
+
+// If clang_type is a CXXRecordDecl, builds the method override list for each
+// of its virtual methods.
+static void addMethodOverrides(ClangASTContext , CompilerType _type) {
+  if (auto *record =
+  ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType()))
+for (auto *method : record->methods())
+  addOverridesForMethod(method);
+}
+
 bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE ,
 lldb_private::Type *type,
 CompilerType _type) {
@@ -2311,6 +2398,7 @@
   }
 }
 
+addMethodOverrides(m_ast, clang_type);
 ClangASTContext::BuildIndirectFields(clang_type);
 ClangASTContext::CompleteTagDeclarationDefinition(clang_type);
 
Index: Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp
===
--- /dev/null
+++ 

[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[Lang Hames via Phabricator via lldb-commits]

lhames added inline comments.



Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2103
 
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.

aprantl wrote:
> Could you add some doxygen comments explaining what the new function do and 
> why doing this is necessary?
Will do.



Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2105-2106
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  assert(>getASTContext() == >getASTContext() &&
+ "Methods should have the same AST context");
+  clang::ASTContext  = m1->getASTContext();

clayborg wrote:
> Use lldb_assert and possibly return false afterwards in case the asserts are 
> compiled out
We can switch to lldb_assert to give us more control (at compile time) about 
when we turn it on or off, but I don't think we should bail out: this is an 
invariant, rather than a potential error case.



Comment at: 
Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp:15
+  Base *b = 
+  return 0; // Please test these expressions while stopped at this line:
+}

jingham wrote:
> aprantl wrote:
> > the expressions are missing :-)
> > Perhaps convert this into an inline testcase?
> The expressions are in the test .py file.  It isn't necessary to put them 
> here, I'd just fix the comment.
> 
> Please don't convert regular test cases into inline ones, however.  The 
> benefit of inline test cases is mostly that they are easier to write, but 
> they are harder to debug when they go wrong.  So if the are already in 
> regular form I'd rather not convert them.
The text of the comment was just cribbed from one of the other expression 
tests. Is there a preferred phraseology?

  return 0; // run expressions here

?


Repository:
  rL LLVM

https://reviews.llvm.org/D41997



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

[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

[Lang Hames via Phabricator via lldb-commits]

lhames created this revision.
lhames added a reviewer: davide.
lhames added a project: LLDB.
Herald added subscribers: llvm-commits, JDevlieghere.

Failure to build the method override tables results in expression failures on 
the following trivial test case:

  class Base {
  public:
virtual ~Base() {}
virtual void foo() {}
  };
  
  class Derived : public Base {
  public:
virtual void foo() {}
  };
  
  int main() {
Derived d;
Base *b = 
return 0; // "expr b->foo()" ok. "expr d.foo()" crashes. 
  }

The reason is that without an overrides table, the definition of foo in derived 
is treated as a new method definition (rather than an override) and allocated 
its own vtable entry which does not exist in the vtable of Derived in the 
compiled program.


Repository:
  rL LLVM

https://reviews.llvm.org/D41997

Files:
  Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  Python/lldbsuite/test/expression_command/call-overridden-method/Makefile
  
Python/lldbsuite/test/expression_command/call-overridden-method/TestCallOverriddenMethod.py
  Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp

Index: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
===
--- Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
+++ Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
@@ -40,6 +40,7 @@
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/StreamString.h"
 
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclObjC.h"
 
@@ -2099,6 +2100,84 @@
   return template_param_infos.args.size() == template_param_infos.names.size();
 }
 
+static bool isOverload(clang::CXXMethodDecl *m1, clang::CXXMethodDecl *m2) {
+  // FIXME: This should detect covariant return types, but currently doesn't.
+  assert(>getASTContext() == >getASTContext() &&
+ "Methods should have the same AST context");
+  clang::ASTContext  = m1->getASTContext();
+
+  const auto *m1Type =
+llvm::cast(
+  context.getCanonicalType(m1->getType()));
+
+  const auto *m2Type =
+llvm::cast(
+  context.getCanonicalType(m2->getType()));
+
+  auto compareArgTypes =
+[](const clang::QualType , const clang::QualType ) {
+  return context.hasSameType(m1p.getUnqualifiedType(),
+ m2p.getUnqualifiedType());
+};
+
+  return !std::equal(m1Type->param_type_begin(), m1Type->param_type_end(),
+ m2Type->param_type_begin(), compareArgTypes);
+}
+
+static void addOverridesForMethod(clang::CXXMethodDecl *decl) {
+  if (!decl->isVirtual())
+return;
+
+  clang::CXXBasePaths paths;
+
+  auto find_overridden_methods =
+[decl](const clang::CXXBaseSpecifier *specifier, clang::CXXBasePath ) {
+  if (auto *base_record =
+  llvm::dyn_cast(
+specifier->getType()->getAs()->getDecl())) {
+
+clang::DeclarationName name = decl->getDeclName();
+
+// If this is a destructor, check whether the base class destructor is
+// virtual.
+if (name.getNameKind() == clang::DeclarationName::CXXDestructorName)
+  if (auto *baseDtorDecl = base_record->getDestructor()) {
+if (baseDtorDecl->isVirtual()) {
+  path.Decls = baseDtorDecl;
+  return true;
+} else
+  return false;
+  }
+
+// Otherwise, search for name in the base class.
+for (path.Decls = base_record->lookup(name); !path.Decls.empty();
+ path.Decls = path.Decls.slice(1)) {
+  if (auto *method_decl =
+llvm::dyn_cast(path.Decls.front()))
+if (method_decl->isVirtual() && !isOverload(decl, method_decl)) {
+  path.Decls = method_decl;
+  return true;
+}
+}
+  }
+
+  return false;
+};
+
+  if (decl->getParent()->lookupInBases(find_overridden_methods, paths)) {
+for (auto *overridden_decl : paths.found_decls())
+  decl->addOverriddenMethod(
+llvm::cast(overridden_decl));
+  }
+}
+
+static void addMethodOverrides(ClangASTContext , CompilerType _type) {
+  if (auto *record =
+  ast.GetAsCXXRecordDecl(clang_type.GetOpaqueQualType()))
+for (auto *method : record->methods())
+  addOverridesForMethod(method);
+}
+
 bool DWARFASTParserClang::CompleteTypeFromDWARF(const DWARFDIE ,
 lldb_private::Type *type,
 CompilerType _type) {
@@ -2311,6 +2390,7 @@
   }
 }
 
+addMethodOverrides(m_ast, clang_type);
 ClangASTContext::BuildIndirectFields(clang_type);
 ClangASTContext::CompleteTagDeclarationDefinition(clang_type);
 
Index: Python/lldbsuite/test/expression_command/call-overridden-method/main.cpp
===
--- /dev/null
+++ 

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

[Lang Hames via Phabricator via lldb-commits]

lhames added a comment.

I prefer the lambda syntax: Locking with an argument requires you to know the 
idiom (which wasn't immediately obvious to me), whereas the lambda enforces it 
implicitly.

We could also consider writing an atomic access adapter for this (and other 
types). That would be generally useful, but would also require more 
consideration (i.e. we shouldn't block this patch on that discussion).




Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:24
+template 
+static decltype(std::declval()(std::declval()))
+WithExclusiveSourceMap(FnType fn) {

spyffe wrote:
> lhames wrote:
> > Does std::result_of::type work as the return type?
> > 
> > http://en.cppreference.com/w/cpp/types/result_of
> No, it doesn't.  If I change the declaration to
> ```
> template 
> static typename std::result_of::type
> WithExclusiveSourceMap(FnType fn) {
> ```
> then the compiler can no longer resolve calls:
> ```
> …/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:25:1: Candidate 
> template ignored: substitution failure [with FnType = (lambda at 
> …/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:39:7)]: implicit 
> instantiation of undefined template 'std::__1::result_of<(lambda at 
> …/lldb/source/Symbol/ClangExternalASTSourceCommon.cpp:39:7)>'
> ```
Oh right, result_of only works for function types at the moment.

One minor tidy-up would be to use the 'auto' function declaration syntax so 
that you can use the name of the functor in the decltype expression, rather 
than having to declval one up. i.e.:

  static decltype(std::declval()(std::declval()))
  WithExclusiveSourceMap(FnType fn) {

becomes

  static auto WithExclusiveSourceMap(FnType fn) -> 
declval(fn(std::declval())) {
...


Repository:
  rL LLVM

https://reviews.llvm.org/D35083



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

[Lldb-commits] [PATCH] D35083: [TypeSystem] Guard the global `ASTSourceMap` with a mutex

[Lang Hames via Phabricator via lldb-commits]

lhames added inline comments.



Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:24
+template 
+static decltype(std::declval()(std::declval()))
+WithExclusiveSourceMap(FnType fn) {

Does std::result_of::type work as the return type?

http://en.cppreference.com/w/cpp/types/result_of



Comment at: source/Symbol/ClangExternalASTSourceCommon.cpp:28
   static ASTSourceMap *s_source_map = new ASTSourceMap;
-  return *s_source_map;
+  static std::mutex s_source_map_mutex;
+  

Should this be on a context object of some kind (ASTContext?).


Repository:
  rL LLVM

https://reviews.llvm.org/D35083



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

[Lldb-commits] [PATCH] D32899: [RuntimeDyld] Fix debug section relocation (pr20457)

[Lang Hames via Phabricator via lldb-commits]

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

LGTM. :)

And yes - I think it's reasonable to add that MCJIT unit test case too.

Thanks for working on this!


https://reviews.llvm.org/D32899



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

[Lldb-commits] [PATCH] D32295: [RFC] Fix crash in expression evaluation

[Lang Hames via Phabricator via lldb-commits]

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

Approving the LLDB/Linux working again.

Thanks for digging in to this Pavel. Greg Clayton appears to have hit the same 
issue recently too. Digging through bugs.llvm.org, it looks like this might be: 
https://bugs.llvm.org/show_bug.cgi?id=20457 . If we can fix this it would allow 
us to break in and debug JIT'd expressions on Linux, which would be cool.


https://reviews.llvm.org/D32295



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