[Lldb-commits] [PATCH] D150168: [lldb] Simplify predicates of find_if in BroadcastManager

[Jonas Devlieghere via Phabricator via lldb-commits]

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

🚢


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150168

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

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[CaprYang via Phabricator via lldb-commits]

CaprYang updated this revision to Diff 520660.

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

https://reviews.llvm.org/D150043

Files:
  llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
  llvm/test/Transforms/InferAddressSpaces/AMDGPU/icmp.ll
  llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll
  llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll

Index: llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll
===
--- /dev/null
+++ llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll
@@ -0,0 +1,101 @@
+; RUN: opt -S -passes=infer-address-spaces -assume-default-is-flat-addrspace %s | FileCheck %s
+
+; CHECK-LABEL: @double_ascast(
+; CHECK: call void @use(<4 x ptr addrspace(3)> %input)
+; CHECK: ret void
+define void @double_ascast(<4 x ptr addrspace(3)> %input) {
+entry:
+  %tmp0 = addrspacecast <4 x ptr addrspace(3)> %input to <4 x ptr>
+  %tmp1 = addrspacecast <4 x ptr> %tmp0 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp1)
+  ret void
+}
+
+; CHECK-LABEL: @double_gep(
+; CHECK: %tmp1 = getelementptr float, ptr addrspace(3) %input, <4 x i64> %i
+; CHECK: %tmp2 = getelementptr float, <4 x ptr addrspace(3)> %tmp1, i64 %j
+; CHECK: call void @use(<4 x ptr addrspace(3)> %tmp2)
+; CHECK: ret void
+define void @double_gep(ptr addrspace(3) %input, <4 x i64> %i, i64 %j) {
+entry:
+  %tmp0 = addrspacecast ptr addrspace(3) %input to ptr
+  %tmp1 = getelementptr float, ptr %tmp0, <4 x i64> %i
+  %tmp2 = getelementptr float, <4 x ptr> %tmp1, i64 %j
+  %tmp3 = addrspacecast <4 x ptr> %tmp2 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp3)
+  ret void
+}
+
+; CHECK-LABEL: @inferas_phi(
+; CHECK: entry:
+; CHECK: br i1 %cond, label %inc, label %end
+; CHECK: inc:
+; CHECK: %tmp1 = getelementptr float, <4 x ptr addrspace(3)> %input, i64 1
+; CHECK: br label %end
+; CHECK: end:
+; CHECK: %tmp2 = phi <4 x ptr addrspace(3)> [ %input, %entry ], [ %tmp1, %inc ]
+; CHECK: call void @use(<4 x ptr addrspace(3)> %tmp2)
+; CHECK: ret void
+define void @inferas_phi(<4 x ptr addrspace(3)> %input, i1 %cond) {
+entry:
+  %tmp0 = addrspacecast <4 x ptr addrspace(3)> %input to <4 x ptr>
+  br i1 %cond, label %inc, label %end
+
+inc:
+  %tmp1 = getelementptr float, <4 x ptr> %tmp0, i64 1
+  br label %end
+
+end:
+  %tmp2 = phi <4 x ptr> [ %tmp0, %entry ], [ %tmp1, %inc ]
+  %tmp3 = addrspacecast <4 x ptr> %tmp2 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp3)
+  ret void
+}
+
+; CHECK-LABEL: @inferas_ptr2int2ptr(
+; CHECK: call void @use(<4 x ptr addrspace(3)> %input)
+; CHECK: ret void
+define void @inferas_ptr2int2ptr(<4 x ptr addrspace(3)> %input) {
+entry:
+  %tmp0 = addrspacecast <4 x ptr addrspace(3)> %input to <4 x ptr>
+  %tmp1 = ptrtoint <4 x ptr> %tmp0 to <4 x i64>
+  %tmp2 = inttoptr <4 x i64> %tmp1 to <4 x ptr>
+  %tmp3 = addrspacecast <4 x ptr> %tmp2 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp3)
+  ret void
+}
+
+; CHECK-LABEL: @inferas_loop(
+; CHECK: entry:
+; CHECK: br label %loop
+; CHECK: loop:
+; CHECK: %now = phi <4 x ptr addrspace(3)> [ %begin, %entry ], [ %next, %loop ]
+; CHECK: call void @use(<4 x ptr addrspace(3)> %now)
+; CHECK: %next = getelementptr float, <4 x ptr addrspace(3)> %now, i64 1
+; CHECK: %veq = icmp eq <4 x ptr addrspace(3)> %next, %end
+; CHECK: %mask = bitcast <4 x i1> %veq to i4
+; CHECK: %cond = icmp eq i4 %mask, 0
+; CHECK: br i1 %cond, label %loop, label %exit
+; CHECK: exit:
+; CHECK: ret void
+define void @inferas_loop(<4 x ptr addrspace(3)> %begin, <4 x ptr addrspace(3)> %end) {
+entry:
+  %begin0 = addrspacecast <4 x ptr addrspace(3)> %begin to <4 x ptr>
+  %end0 = addrspacecast <4 x ptr addrspace(3)> %end to <4 x ptr>
+  br label %loop
+
+loop:
+  %now = phi <4 x ptr> [ %begin0, %entry ], [ %next, %loop ]
+  %now3 = addrspacecast <4 x ptr> %now to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %now3)
+  %next = getelementptr float, <4 x ptr> %now, i64 1
+  %veq = icmp eq <4 x ptr> %next, %end0
+  %mask = bitcast <4 x i1> %veq to i4
+  %cond = icmp eq i4 %mask, 0
+  br i1 %cond, label %loop, label %exit
+
+exit:
+  ret void
+}
+
+declare void @use(<4 x ptr addrspace(3)>)
\ No newline at end of file
Index: llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll
===
--- /dev/null
+++ llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll
@@ -0,0 +1,25 @@
+; RUN: opt -S -passes=infer-address-spaces -assume-default-is-flat-addrspace %s | FileCheck %s
+
+; CHECK-LABEL: @masked_gather_inferas(
+; CHECK: tail call <4 x i32> @llvm.masked.gather.v4i32.v4p1
+define <4 x i32> @masked_gather_inferas(ptr addrspace(1) %out, <4 x i64> %index) {
+entry:
+  %out.1 = addrspacecast ptr addrspace(1) %out to ptr
+  %ptrs = getelementptr inbounds i32, ptr %out.1, <4 x i64> %index
+  %value

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[CaprYang via Phabricator via lldb-commits]

CaprYang added inline comments.



Comment at: llvm/test/Transforms/InferAddressSpaces/AMDGPU/icmp.ll:151
 ; CHECK-LABEL: @icmp_flat_flat_from_group_vector(
-; CHECK: %cmp = icmp eq <2 x ptr> %cast0, %cast1
+; CHECK: %cmp = icmp eq <2 x ptr addrspace(3)> %group.ptr.0, %group.ptr.1
 define <2 x i1> @icmp_flat_flat_from_group_vector(<2 x ptr addrspace(3)> 
%group.ptr.0, <2 x ptr addrspace(3)> %group.ptr.1) #0 {

CaprYang wrote:
> arsenm wrote:
> > You touched a lot more than just icmp, so this needs more tests to cover 
> > all the newly handled cases 
> I will add more tests later
added


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

https://reviews.llvm.org/D150043

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

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[Matt Arsenault via Phabricator via lldb-commits]

arsenm added inline comments.



Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:289
+
+static bool hasSameElementOfPtrOrVecPtrs(Type *Ty1, Type *Ty2) {
+  assert(isPtrOrVecOfPtrsType(Ty1) && isPtrOrVecOfPtrsType(Ty2));

arsenm wrote:
> Ditto, only opaque pointers matter now
You don't need to bother using getWithSamePointeeType. You can use 
Type::getWithNewType



Comment at: llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll:3
+
+; CHECK-LABEL: @masked_gather_inferas(
+; CHECK: tail call <4 x i32> @llvm.masked.gather.v4i32.v4p1

Generate full checks 



Comment at: llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll:2
+; RUN: opt -S -passes=infer-address-spaces -assume-default-is-flat-addrspace 
%s | FileCheck %s
+
+; CHECK-LABEL: @double_ascast(

Generate full checks 


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

https://reviews.llvm.org/D150043

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

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[CaprYang via Phabricator via lldb-commits]

CaprYang added inline comments.



Comment at: llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp:289
+
+static bool hasSameElementOfPtrOrVecPtrs(Type *Ty1, Type *Ty2) {
+  assert(isPtrOrVecOfPtrsType(Ty1) && isPtrOrVecOfPtrsType(Ty2));

arsenm wrote:
> arsenm wrote:
> > Ditto, only opaque pointers matter now
> You don't need to bother using getWithSamePointeeType. You can use 
> Type::getWithNewType
Does it mean this? do't check non-opaque types.

```
static Type *getPtrOrVecOfPtrsWithNewAS(Type *Ty, unsigned NewAddrSpace) {
  assert(Ty->isPtrOrPtrVectorTy());
  PointerType *NPT = PointerType::get(Ty->getContext(), NewAddrSpace);
  return Ty->getWithNewType(NPT);
}
```


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

https://reviews.llvm.org/D150043

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Andrey Ali Khan Bolshakov via Phabricator via lldb-commits]

bolshakov-a updated this revision to Diff 520678.
bolshakov-a added a comment.

Fix MS compatibility mangling algorithm. Tested with MSVC ver. 19.35 (toolset 
ver. 143).


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

https://reviews.llvm.org/D140996

Files:
  clang-tools-extra/clangd/DumpAST.cpp
  clang-tools-extra/clangd/FindTarget.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ODRHash.h
  clang/include/clang/AST/PropertiesBase.td
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TemplateArgumentVisitor.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TypeLoc.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CXX/drs/dr12xx.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
  clang/test/CodeGenCXX/mangle-ms-templates.cpp
  clang/test/CodeGenCXX/mangle-template.cpp
  clang/test/CodeGenCXX/template-arguments.cpp
  clang/test/Index/USR/structural-value-tpl-arg.cpp
  clang/test/Modules/odr_hash.cpp
  clang/test/SemaCXX/warn-bool-conversion.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7305,6 +7305,9 @@
 
   case clang::TemplateArgument::Pack:
 return eTemplateArgumentKindPack;
+
+  case clang::TemplateArgument::StructuralValue:
+return eTemplateArgumentKindStructuralValue;
   }
   llvm_unreachable("Unhandled clang::TemplateArgument::ArgKind");
 }
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -849,6 +849,7 @@
   eTemplateArgumentKindExpression,
   eTemplateArgumentKindPack,
   eTemplateArgumentKindNullPtr,
+  eTemplateArgumentKindStructuralValue,
 };
 
 /// Type of match to be performed when looking for a formatter for a data type.
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1053,13 +1053,21 @@
 
 
 
-  Class types as non-type template parameters
+  Class types as non-type template parameters
   https://wg21.link/p0732r2";>P0732R2
-  Partial
+  Clang 12
+
+ 
+  Generalized non-type template parameters of scalar type
+  https://wg21.link/p1907r1";>P1907R1
+  
+
+  Clang 17 (Partial)
+  Reference type template arguments referring to instantiation-dependent objects and subobjects
+  (i.e. declared inside a template but neither type- nor value-dependent) aren't fully supported.
+
+  
 
-   
-https://wg21.link/p1907r1";>P1907R1
-  
 
   Destroying operator delete
   https://wg21.link/p0722r3";>P0722R3
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -1463,6 +1463,9 @@
 return CXTemplateArgumentKind_NullPtr;
   case TemplateArgument::Integral:
 return CXTemplateArgumentKind_Integral;
+  case TemplateArgument::StructuralValue:
+// FIXME: Expose these values.
+return CXTemplateArgumentKind_Invalid;
   case TemplateArgument::Template:
 return CXTemplateArgumentKind_Template;
   case TemplateArgument::TemplateExpansion:
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -1570,6 +1570,11 @@
   return Visit(MakeCXCursor(E, StmtParent, TU, RegionOfInterest));
 return false;
 
+  case TemplateArgument::StructuralValue:
+if (Expr *E = TAL.getSourceStructuralValueExpression())
+  return Vis

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[CaprYang via Phabricator via lldb-commits]

CaprYang updated this revision to Diff 520692.

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

https://reviews.llvm.org/D150043

Files:
  llvm/lib/Transforms/Scalar/InferAddressSpaces.cpp
  llvm/test/Transforms/InferAddressSpaces/AMDGPU/icmp.ll
  llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll
  llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll

Index: llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll
===
--- /dev/null
+++ llvm/test/Transforms/InferAddressSpaces/vector-of-pointers.ll
@@ -0,0 +1,104 @@
+; RUN: opt -S -passes=infer-address-spaces -assume-default-is-flat-addrspace %s | FileCheck %s
+
+; CHECK-LABEL: @double_ascast(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: call void @use(<4 x ptr addrspace(3)> [[INPUT:%.*]])
+; CHECK-NEXT: ret void
+define void @double_ascast(<4 x ptr addrspace(3)> %input) {
+entry:
+  %tmp0 = addrspacecast <4 x ptr addrspace(3)> %input to <4 x ptr>
+  %tmp1 = addrspacecast <4 x ptr> %tmp0 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp1)
+  ret void
+}
+
+; CHECK-LABEL: @double_gep(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[TMP1:%.*]] = getelementptr float, ptr addrspace(3) [[INPUT:%.*]], <4 x i64> [[I:%.*]]
+; CHECK-NEXT: [[TMP2:%.*]] = getelementptr float, <4 x ptr addrspace(3)> [[TMP1]], i64 [[J:%.*]]
+; CHECK-NEXT: call void @use(<4 x ptr addrspace(3)> [[TMP2]])
+; CHECK-NEXT: ret void
+define void @double_gep(ptr addrspace(3) %input, <4 x i64> %i, i64 %j) {
+entry:
+  %tmp0 = addrspacecast ptr addrspace(3) %input to ptr
+  %tmp1 = getelementptr float, ptr %tmp0, <4 x i64> %i
+  %tmp2 = getelementptr float, <4 x ptr> %tmp1, i64 %j
+  %tmp3 = addrspacecast <4 x ptr> %tmp2 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp3)
+  ret void
+}
+
+; CHECK-LABEL: @inferas_phi(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br i1 [[COND:%.*]], label %inc, label %end
+; CHECK: inc:
+; CHECK-NEXT: [[TMP1:%.*]] = getelementptr float, <4 x ptr addrspace(3)> [[INPUT:%.*]], i64 1
+; CHECK-NEXT: br label %end
+; CHECK: end:
+; CHECK-NEXT: [[TMP2:%.*]] = phi <4 x ptr addrspace(3)> [ [[INPUT]], %entry ], [ [[TMP1]], %inc ]
+; CHECK-NEXT: call void @use(<4 x ptr addrspace(3)> [[TMP2]])
+; CHECK-NEXT: ret void
+define void @inferas_phi(<4 x ptr addrspace(3)> %input, i1 %cond) {
+entry:
+  %tmp0 = addrspacecast <4 x ptr addrspace(3)> %input to <4 x ptr>
+  br i1 %cond, label %inc, label %end
+
+inc:
+  %tmp1 = getelementptr float, <4 x ptr> %tmp0, i64 1
+  br label %end
+
+end:
+  %tmp2 = phi <4 x ptr> [ %tmp0, %entry ], [ %tmp1, %inc ]
+  %tmp3 = addrspacecast <4 x ptr> %tmp2 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp3)
+  ret void
+}
+
+; CHECK-LABEL: @inferas_ptr2int2ptr(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: call void @use(<4 x ptr addrspace(3)> [[INPUT:%.*]])
+; CHECK-NEXT: ret void
+define void @inferas_ptr2int2ptr(<4 x ptr addrspace(3)> %input) {
+entry:
+  %tmp0 = addrspacecast <4 x ptr addrspace(3)> %input to <4 x ptr>
+  %tmp1 = ptrtoint <4 x ptr> %tmp0 to <4 x i64>
+  %tmp2 = inttoptr <4 x i64> %tmp1 to <4 x ptr>
+  %tmp3 = addrspacecast <4 x ptr> %tmp2 to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %tmp3)
+  ret void
+}
+
+; CHECK-LABEL: @inferas_loop(
+; CHECK-NEXT: entry:
+; CHECK-NEXT: br label %loop
+; CHECK: loop:
+; CHECK-NEXT: [[NOW:%.*]] = phi <4 x ptr addrspace(3)> [ [[BEGIN:%.*]], %entry ], [ [[NEXT:%.*]], %loop ]
+; CHECK-NEXT: call void @use(<4 x ptr addrspace(3)> [[NOW]])
+; CHECK-NEXT: [[NEXT]] = getelementptr float, <4 x ptr addrspace(3)> [[NOW]], i64 1
+; CHECK-NEXT: [[VEQ:%.*]] = icmp eq <4 x ptr addrspace(3)> [[NEXT]], [[END:%.*]]
+; CHECK-NEXT: [[MASK:%.*]] = bitcast <4 x i1> [[VEQ]] to i4
+; CHECK-NEXT: [[COND:%.*]] = icmp eq i4 [[MASK]], 0
+; CHECK-NEXT: br i1 [[COND]], label %loop, label %exit
+; CHECK: exit:
+; CHECK-NEXT: ret void
+define void @inferas_loop(<4 x ptr addrspace(3)> %begin, <4 x ptr addrspace(3)> %end) {
+entry:
+  %begin0 = addrspacecast <4 x ptr addrspace(3)> %begin to <4 x ptr>
+  %end0 = addrspacecast <4 x ptr addrspace(3)> %end to <4 x ptr>
+  br label %loop
+
+loop:
+  %now = phi <4 x ptr> [ %begin0, %entry ], [ %next, %loop ]
+  %now3 = addrspacecast <4 x ptr> %now to <4 x ptr addrspace(3)>
+  call void @use(<4 x ptr addrspace(3)> %now3)
+  %next = getelementptr float, <4 x ptr> %now, i64 1
+  %veq = icmp eq <4 x ptr> %next, %end0
+  %mask = bitcast <4 x i1> %veq to i4
+  %cond = icmp eq i4 %mask, 0
+  br i1 %cond, label %loop, label %exit
+
+exit:
+  ret void
+}
+
+declare void @use(<4 x ptr addrspace(3)>)
\ No newline at end of file
Index: llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll
===
--- /dev/null
+++ llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll
@@ -0,0 +1,31 @@
+; RUN: opt -S -passes=infer-address-spaces -assume-default-is-flat-ad

[Lldb-commits] [PATCH] D150043: [InferAddressSpaces] Handle vector of pointers type & Support intrinsic masked gather/scatter

[CaprYang via Phabricator via lldb-commits]

CaprYang added inline comments.



Comment at: llvm/test/Transforms/InferAddressSpaces/masked-gather-scatter.ll:3
+
+; CHECK-LABEL: @masked_gather_inferas(
+; CHECK: tail call <4 x i32> @llvm.masked.gather.v4i32.v4p1

arsenm wrote:
> Generate full checks 
updated


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

https://reviews.llvm.org/D150043

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

[Lldb-commits] [PATCH] D140996: [c++20] P1907R1: Support for generalized non-type template arguments of scalar type.

[Andrey Ali Khan Bolshakov via Phabricator via lldb-commits]

bolshakov-a updated this revision to Diff 520716.
bolshakov-a added a comment.

Rebased.


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

https://reviews.llvm.org/D140996

Files:
  clang-tools-extra/clangd/DumpAST.cpp
  clang-tools-extra/clangd/FindTarget.cpp
  clang/docs/ReleaseNotes.rst
  clang/include/clang/AST/ODRHash.h
  clang/include/clang/AST/PropertiesBase.td
  clang/include/clang/AST/RecursiveASTVisitor.h
  clang/include/clang/AST/TemplateArgumentVisitor.h
  clang/include/clang/AST/TemplateBase.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Sema/Sema.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/AST/ASTStructuralEquivalence.cpp
  clang/lib/AST/Decl.cpp
  clang/lib/AST/ItaniumMangle.cpp
  clang/lib/AST/MicrosoftMangle.cpp
  clang/lib/AST/ODRHash.cpp
  clang/lib/AST/StmtProfile.cpp
  clang/lib/AST/TemplateBase.cpp
  clang/lib/AST/TypeLoc.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/Index/USRGeneration.cpp
  clang/lib/Sema/SemaLookup.cpp
  clang/lib/Sema/SemaOverload.cpp
  clang/lib/Sema/SemaTemplate.cpp
  clang/lib/Sema/SemaTemplateDeduction.cpp
  clang/lib/Sema/SemaTemplateInstantiate.cpp
  clang/lib/Sema/SemaTemplateVariadic.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/ASTWriter.cpp
  clang/test/CXX/drs/dr12xx.cpp
  clang/test/CXX/temp/temp.arg/temp.arg.nontype/p1.cpp
  clang/test/CodeGenCXX/mangle-ms-templates.cpp
  clang/test/CodeGenCXX/mangle-template.cpp
  clang/test/CodeGenCXX/template-arguments.cpp
  clang/test/Index/USR/structural-value-tpl-arg.cpp
  clang/test/Modules/odr_hash.cpp
  clang/test/SemaCXX/warn-bool-conversion.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx1z.cpp
  clang/test/SemaTemplate/temp_arg_nontype_cxx20.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/CXCursor.cpp
  clang/www/cxx_status.html
  lldb/include/lldb/lldb-enumerations.h
  lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp

Index: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
===
--- lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
+++ lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
@@ -7305,6 +7305,9 @@
 
   case clang::TemplateArgument::Pack:
 return eTemplateArgumentKindPack;
+
+  case clang::TemplateArgument::StructuralValue:
+return eTemplateArgumentKindStructuralValue;
   }
   llvm_unreachable("Unhandled clang::TemplateArgument::ArgKind");
 }
Index: lldb/include/lldb/lldb-enumerations.h
===
--- lldb/include/lldb/lldb-enumerations.h
+++ lldb/include/lldb/lldb-enumerations.h
@@ -849,6 +849,7 @@
   eTemplateArgumentKindExpression,
   eTemplateArgumentKindPack,
   eTemplateArgumentKindNullPtr,
+  eTemplateArgumentKindStructuralValue,
 };
 
 /// Type of match to be performed when looking for a formatter for a data type.
Index: clang/www/cxx_status.html
===
--- clang/www/cxx_status.html
+++ clang/www/cxx_status.html
@@ -1049,13 +1049,21 @@
 
 
 
-  Class types as non-type template parameters
+  Class types as non-type template parameters
   https://wg21.link/p0732r2";>P0732R2
-  Partial
+  Clang 12
+
+ 
+  Generalized non-type template parameters of scalar type
+  https://wg21.link/p1907r1";>P1907R1
+  
+
+  Clang 17 (Partial)
+  Reference type template arguments referring to instantiation-dependent objects and subobjects
+  (i.e. declared inside a template but neither type- nor value-dependent) aren't fully supported.
+
+  
 
-   
-https://wg21.link/p1907r1";>P1907R1
-  
 
   Destroying operator delete
   https://wg21.link/p0722r3";>P0722R3
Index: clang/tools/libclang/CXCursor.cpp
===
--- clang/tools/libclang/CXCursor.cpp
+++ clang/tools/libclang/CXCursor.cpp
@@ -1463,6 +1463,9 @@
 return CXTemplateArgumentKind_NullPtr;
   case TemplateArgument::Integral:
 return CXTemplateArgumentKind_Integral;
+  case TemplateArgument::StructuralValue:
+// FIXME: Expose these values.
+return CXTemplateArgumentKind_Invalid;
   case TemplateArgument::Template:
 return CXTemplateArgumentKind_Template;
   case TemplateArgument::TemplateExpansion:
Index: clang/tools/libclang/CIndex.cpp
===
--- clang/tools/libclang/CIndex.cpp
+++ clang/tools/libclang/CIndex.cpp
@@ -1570,6 +1570,11 @@
   return Visit(MakeCXCursor(E, StmtParent, TU, RegionOfInterest));
 return false;
 
+  case TemplateArgument::StructuralValue:
+if (Expr *E = TAL.getSourceStructuralValueExpression())
+  return Visit(MakeCXCursor(E, StmtParent, TU, RegionOfInterest));
+return false;
+
   ca

[Lldb-commits] [lldb] ec77d1f - [lldb] Simplify predicates of find_if in BroadcastManager

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-05-09T10:00:02-07:00
New Revision: ec77d1f3d9fcf7105b6bda25fb4d0e5ed5afd0c5

URL: 
https://github.com/llvm/llvm-project/commit/ec77d1f3d9fcf7105b6bda25fb4d0e5ed5afd0c5
DIFF: 
https://github.com/llvm/llvm-project/commit/ec77d1f3d9fcf7105b6bda25fb4d0e5ed5afd0c5.diff

LOG: [lldb] Simplify predicates of find_if in BroadcastManager

We had some custom classes that were used as the predicate for
`std::find_if`. It would be a lot simpler if we used lambdas instead.

Differential Revision: https://reviews.llvm.org/D150168

Added: 


Modified: 
lldb/include/lldb/Utility/Broadcaster.h
lldb/source/Utility/Broadcaster.cpp

Removed: 




diff  --git a/lldb/include/lldb/Utility/Broadcaster.h 
b/lldb/include/lldb/Utility/Broadcaster.h
index 85e2468a111f5..081e6ee5c883f 100644
--- a/lldb/include/lldb/Utility/Broadcaster.h
+++ b/lldb/include/lldb/Utility/Broadcaster.h
@@ -112,103 +112,6 @@ class BroadcasterManager
   listener_collection m_listeners;
 
   mutable std::recursive_mutex m_manager_mutex;
-
-  // A couple of comparator classes for find_if:
-
-  class BroadcasterClassMatches {
-  public:
-BroadcasterClassMatches(const ConstString &broadcaster_class)
-: m_broadcaster_class(broadcaster_class) {}
-
-~BroadcasterClassMatches() = default;
-
-bool operator()(const event_listener_key &input) const {
-  return (input.first.GetBroadcasterClass() == m_broadcaster_class);
-}
-
-  private:
-ConstString m_broadcaster_class;
-  };
-
-  class BroadcastEventSpecMatches {
-  public:
-BroadcastEventSpecMatches(const BroadcastEventSpec &broadcaster_spec)
-: m_broadcaster_spec(broadcaster_spec) {}
-
-~BroadcastEventSpecMatches() = default;
-
-bool operator()(const event_listener_key &input) const {
-  return (input.first.IsContainedIn(m_broadcaster_spec));
-}
-
-  private:
-BroadcastEventSpec m_broadcaster_spec;
-  };
-
-  class ListenerMatchesAndSharedBits {
-  public:
-explicit ListenerMatchesAndSharedBits(
-const BroadcastEventSpec &broadcaster_spec,
-const lldb::ListenerSP &listener_sp)
-: m_broadcaster_spec(broadcaster_spec), m_listener_sp(listener_sp) {}
-
-~ListenerMatchesAndSharedBits() = default;
-
-bool operator()(const event_listener_key &input) const {
-  return (input.first.GetBroadcasterClass() ==
-  m_broadcaster_spec.GetBroadcasterClass() &&
-  (input.first.GetEventBits() &
-   m_broadcaster_spec.GetEventBits()) != 0 &&
-  input.second == m_listener_sp);
-}
-
-  private:
-BroadcastEventSpec m_broadcaster_spec;
-const lldb::ListenerSP m_listener_sp;
-  };
-
-  class ListenerMatches {
-  public:
-explicit ListenerMatches(const lldb::ListenerSP &in_listener_sp)
-: m_listener_sp(in_listener_sp) {}
-
-~ListenerMatches() = default;
-
-bool operator()(const event_listener_key &input) const {
-  if (input.second == m_listener_sp)
-return true;
-
-  return false;
-}
-
-  private:
-const lldb::ListenerSP m_listener_sp;
-  };
-
-  class ListenerMatchesPointer {
-  public:
-ListenerMatchesPointer(const Listener *in_listener)
-: m_listener(in_listener) {}
-
-~ListenerMatchesPointer() = default;
-
-bool operator()(const event_listener_key &input) const {
-  if (input.second.get() == m_listener)
-return true;
-
-  return false;
-}
-
-bool operator()(const lldb::ListenerSP &input) const {
-  if (input.get() == m_listener)
-return true;
-
-  return false;
-}
-
-  private:
-const Listener *m_listener;
-  };
 };
 
 /// \class Broadcaster Broadcaster.h "lldb/Utility/Broadcaster.h" An event

diff  --git a/lldb/source/Utility/Broadcaster.cpp 
b/lldb/source/Utility/Broadcaster.cpp
index 4e6710e1108b3..66c78978571fa 100644
--- a/lldb/source/Utility/Broadcaster.cpp
+++ b/lldb/source/Utility/Broadcaster.cpp
@@ -336,10 +336,13 @@ uint32_t BroadcasterManager::RegisterListenerForEvents(
   collection::iterator iter = m_event_map.begin(), end_iter = 
m_event_map.end();
   uint32_t available_bits = event_spec.GetEventBits();
 
+  auto class_matches = [&event_spec](const event_listener_key &input) -> bool {
+return input.first.GetBroadcasterClass() ==
+   event_spec.GetBroadcasterClass();
+  };
+
   while (iter != end_iter &&
- (iter = find_if(iter, end_iter,
- BroadcasterClassMatches(
- event_spec.GetBroadcasterClass( != end_iter) {
+ (iter = find_if(iter, end_iter, class_matches)) != end_iter) {
 available_bits &= ~((*iter).first.GetEventBits());
 iter++;
   }
@@ -362,7 +365,13 @@ bool BroadcasterManager::UnregisterListenerForEvents(
   if (m_listeners.erase(listener_sp) == 0)
 return false;
 
-  ListenerMatchesAndSharedBits predicate(event_spec, lis

[Lldb-commits] [PATCH] D150168: [lldb] Simplify predicates of find_if in BroadcastManager

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGec77d1f3d9fc: [lldb] Simplify predicates of find_if in 
BroadcastManager (authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150168

Files:
  lldb/include/lldb/Utility/Broadcaster.h
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -336,10 +336,13 @@
   collection::iterator iter = m_event_map.begin(), end_iter = m_event_map.end();
   uint32_t available_bits = event_spec.GetEventBits();
 
+  auto class_matches = [&event_spec](const event_listener_key &input) -> bool {
+return input.first.GetBroadcasterClass() ==
+   event_spec.GetBroadcasterClass();
+  };
+
   while (iter != end_iter &&
- (iter = find_if(iter, end_iter,
- BroadcasterClassMatches(
- event_spec.GetBroadcasterClass( != end_iter) {
+ (iter = find_if(iter, end_iter, class_matches)) != end_iter) {
 available_bits &= ~((*iter).first.GetEventBits());
 iter++;
   }
@@ -362,7 +365,13 @@
   if (m_listeners.erase(listener_sp) == 0)
 return false;
 
-  ListenerMatchesAndSharedBits predicate(event_spec, listener_sp);
+  auto listener_matches_and_shared_bits =
+  [&listener_sp, &event_spec](const event_listener_key &input) -> bool {
+return input.first.GetBroadcasterClass() ==
+   event_spec.GetBroadcasterClass() &&
+   (input.first.GetEventBits() & event_spec.GetEventBits()) != 0 &&
+   input.second == listener_sp;
+  };
   std::vector to_be_readded;
   uint32_t event_bits_to_remove = event_spec.GetEventBits();
 
@@ -370,7 +379,8 @@
   // matches that weren't exact to re-add:
   while (true) {
 collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, predicate);
+iter = find_if(m_event_map.begin(), end_iter,
+   listener_matches_and_shared_bits);
 if (iter == end_iter) {
   break;
 }
@@ -397,9 +407,13 @@
 const BroadcastEventSpec &event_spec) const {
   std::lock_guard guard(m_manager_mutex);
 
+  auto event_spec_matches =
+  [&event_spec](const event_listener_key &input) -> bool {
+return input.first.IsContainedIn(event_spec);
+  };
+
   collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter,
- BroadcastEventSpecMatches(event_spec));
+  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
   if (iter != end_iter)
 return (*iter).second;
 
@@ -408,17 +422,25 @@
 
 void BroadcasterManager::RemoveListener(Listener *listener) {
   std::lock_guard guard(m_manager_mutex);
-  ListenerMatchesPointer predicate(listener);
+  auto listeners_predicate =
+  [&listener](const lldb::ListenerSP &input) -> bool {
+return input.get() == listener;
+  };
+
   listener_collection::iterator iter = m_listeners.begin(),
 end_iter = m_listeners.end();
 
-  iter = std::find_if(iter, end_iter, predicate);
+  iter = std::find_if(iter, end_iter, listeners_predicate);
   if (iter != end_iter)
 m_listeners.erase(iter);
 
   while (true) {
+auto events_predicate =
+[&listener](const event_listener_key &input) -> bool {
+  return input.second.get() == listener;
+};
 collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, predicate);
+iter = find_if(m_event_map.begin(), end_iter, events_predicate);
 if (iter == end_iter)
   break;
 
@@ -428,14 +450,18 @@
 
 void BroadcasterManager::RemoveListener(const lldb::ListenerSP &listener_sp) {
   std::lock_guard guard(m_manager_mutex);
-  ListenerMatches predicate(listener_sp);
+
+  auto listener_matches =
+  [&listener_sp](const event_listener_key &input) -> bool {
+return input.second == listener_sp;
+  };
 
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
   while (true) {
 collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, predicate);
+iter = find_if(m_event_map.begin(), end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
@@ -449,10 +475,13 @@
 
   collection::iterator iter = m_event_map.begin(), end_iter = m_event_map.end();
 
+  auto class_matches = [&broadcaster](const event_listener_key &input) -> bool {
+return input.first.GetBroadcasterClass() ==
+   broadcaster.GetBroadcasterClass();
+  };
+
   while (iter != end_iter &&
- (iter = find_if(iter, end_iter,
- BroadcasterClassMatches(
- broadcaster.GetBroadcasterClass( != end_iter) {
+ 

[Lldb-commits] [lldb] 5864149 - [lldb] Simplify Log::PutString (NFC)

[Dave Lee via lldb-commits]

Author: Dave Lee
Date: 2023-05-09T10:40:42-07:00
New Revision: 58641492741818b987ec86ab75c2cd438dd3ac63

URL: 
https://github.com/llvm/llvm-project/commit/58641492741818b987ec86ab75c2cd438dd3ac63
DIFF: 
https://github.com/llvm/llvm-project/commit/58641492741818b987ec86ab75c2cd438dd3ac63.diff

LOG: [lldb] Simplify Log::PutString (NFC)

* As no format string is involved, avoid unecessary call into `Printf`
* Eliminate creation of a `std::string` to print a `StringRef`

Differential Revision: https://reviews.llvm.org/D150160

Added: 


Modified: 
lldb/source/Utility/Log.cpp

Removed: 




diff  --git a/lldb/source/Utility/Log.cpp b/lldb/source/Utility/Log.cpp
index 7d94a89a8c974..da8569efd444b 100644
--- a/lldb/source/Utility/Log.cpp
+++ b/lldb/source/Utility/Log.cpp
@@ -131,8 +131,15 @@ Log::MaskType Log::GetMask() const {
   return m_mask.load(std::memory_order_relaxed);
 }
 
-void Log::PutCString(const char *cstr) { Printf("%s", cstr); }
-void Log::PutString(llvm::StringRef str) { PutCString(str.str().c_str()); }
+void Log::PutCString(const char *cstr) { PutString(cstr); }
+
+void Log::PutString(llvm::StringRef str) {
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
+  WriteHeader(Stream, "", "");
+  Stream << str << "\n";
+  WriteMessage(FinalMessage);
+}
 
 // Simple variable argument logging with flags.
 void Log::Printf(const char *format, ...) {
@@ -142,20 +149,10 @@ void Log::Printf(const char *format, ...) {
   va_end(args);
 }
 
-// All logging eventually boils down to this function call. If we have a
-// callback registered, then we call the logging callback. If we have a valid
-// file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  std::string FinalMessage;
-  llvm::raw_string_ostream Stream(FinalMessage);
-  WriteHeader(Stream, "", "");
-
   llvm::SmallString<64> Content;
   lldb_private::VASprintf(Content, format, args);
-
-  Stream << Content << "\n";
-
-  WriteMessage(FinalMessage);
+  PutString(Content);
 }
 
 // Printing of errors that are not fatal.
@@ -344,6 +341,8 @@ void Log::WriteHeader(llvm::raw_ostream &OS, 
llvm::StringRef file,
   }
 }
 
+// If we have a callback registered, then we call the logging callback. If we
+// have a valid file handle, we also log to the file.
 void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream



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

[Lldb-commits] [PATCH] D150160: [lldb] Simplify Log::PutString (NFC)

[Dave Lee via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG586414927418: [lldb] Simplify Log::PutString (NFC) (authored 
by kastiglione).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150160

Files:
  lldb/source/Utility/Log.cpp


Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -131,8 +131,15 @@
   return m_mask.load(std::memory_order_relaxed);
 }
 
-void Log::PutCString(const char *cstr) { Printf("%s", cstr); }
-void Log::PutString(llvm::StringRef str) { PutCString(str.str().c_str()); }
+void Log::PutCString(const char *cstr) { PutString(cstr); }
+
+void Log::PutString(llvm::StringRef str) {
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
+  WriteHeader(Stream, "", "");
+  Stream << str << "\n";
+  WriteMessage(FinalMessage);
+}
 
 // Simple variable argument logging with flags.
 void Log::Printf(const char *format, ...) {
@@ -142,20 +149,10 @@
   va_end(args);
 }
 
-// All logging eventually boils down to this function call. If we have a
-// callback registered, then we call the logging callback. If we have a valid
-// file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  std::string FinalMessage;
-  llvm::raw_string_ostream Stream(FinalMessage);
-  WriteHeader(Stream, "", "");
-
   llvm::SmallString<64> Content;
   lldb_private::VASprintf(Content, format, args);
-
-  Stream << Content << "\n";
-
-  WriteMessage(FinalMessage);
+  PutString(Content);
 }
 
 // Printing of errors that are not fatal.
@@ -344,6 +341,8 @@
   }
 }
 
+// If we have a callback registered, then we call the logging callback. If we
+// have a valid file handle, we also log to the file.
 void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream


Index: lldb/source/Utility/Log.cpp
===
--- lldb/source/Utility/Log.cpp
+++ lldb/source/Utility/Log.cpp
@@ -131,8 +131,15 @@
   return m_mask.load(std::memory_order_relaxed);
 }
 
-void Log::PutCString(const char *cstr) { Printf("%s", cstr); }
-void Log::PutString(llvm::StringRef str) { PutCString(str.str().c_str()); }
+void Log::PutCString(const char *cstr) { PutString(cstr); }
+
+void Log::PutString(llvm::StringRef str) {
+  std::string FinalMessage;
+  llvm::raw_string_ostream Stream(FinalMessage);
+  WriteHeader(Stream, "", "");
+  Stream << str << "\n";
+  WriteMessage(FinalMessage);
+}
 
 // Simple variable argument logging with flags.
 void Log::Printf(const char *format, ...) {
@@ -142,20 +149,10 @@
   va_end(args);
 }
 
-// All logging eventually boils down to this function call. If we have a
-// callback registered, then we call the logging callback. If we have a valid
-// file handle, we also log to the file.
 void Log::VAPrintf(const char *format, va_list args) {
-  std::string FinalMessage;
-  llvm::raw_string_ostream Stream(FinalMessage);
-  WriteHeader(Stream, "", "");
-
   llvm::SmallString<64> Content;
   lldb_private::VASprintf(Content, format, args);
-
-  Stream << Content << "\n";
-
-  WriteMessage(FinalMessage);
+  PutString(Content);
 }
 
 // Printing of errors that are not fatal.
@@ -344,6 +341,8 @@
   }
 }
 
+// If we have a callback registered, then we call the logging callback. If we
+// have a valid file handle, we also log to the file.
 void Log::WriteMessage(llvm::StringRef message) {
   // Make a copy of our stream shared pointer in case someone disables our log
   // while we are logging and releases the stream
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve created this revision.
Herald added a project: All.
fdeazeve requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The code inside Broadcaster makes usage of iterators using olden C++ coding
style. Hidden in this old style is a couple of N^2 loops: we iterate over a map
(sequentially), removing the first element that matches some predicate. The
search is _always_ done from the start of the map, which implies that, if the
map has N elements and if all matches happen on the second half of the map, then
we visit the first N/2 elements exactly N/2 * N/2 times.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,20 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +451,12 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +481,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 520786.
fdeazeve added a comment.
Herald added a subscriber: JDevlieghere.

Update commit message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,20 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +451,12 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +481,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Heavily inspired by @bulbazord's D150168 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:382
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())
   break;

Oh, this should be `== end`



Comment at: lldb/source/Utility/Broadcaster.cpp:436
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;

this should be `== end`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:430
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;





Comment at: lldb/source/Utility/Broadcaster.cpp:434-439
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;
 
+iter = m_event_map.erase(iter);

Would be nice to add a `// TODO: use 'std::map::erase_if' when moving to c++20` 
comment



Comment at: lldb/source/Utility/Broadcaster.cpp:454-459
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
+iter = m_event_map.erase(iter);

ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:430
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;

mib wrote:
> 
`listener` is a raw pointer, we shouldn't capture those by reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:430
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;

fdeazeve wrote:
> mib wrote:
> > 
> `listener` is a raw pointer, we shouldn't capture those by reference
I didn't pay attention, I thought it was a local variable. Good point!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Med Ismail Bennani via Phabricator via lldb-commits]

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

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Alex Langford via Phabricator via lldb-commits]

bulbazord accepted this revision.
bulbazord added a comment.

I like this. :)




Comment at: lldb/source/Utility/Broadcaster.cpp:392
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }

I don't think you need to actually capture the iterator here? std::map::erase 
doesn't invalidate existing iterators* (so `begin` and `end` are fine) and we 
redefine `iter` at the beginning of this loop.

*:https://en.cppreference.com/w/cpp/container/map/erase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Jim Ingham via Phabricator via lldb-commits]

jingham accepted this revision.
jingham added a comment.

You are inconsistent in a couple of places about whether you re-look up 
m_event_map.end or use the version you captured in a variable, which is a 
little confusing.  Other than that this looks equivalent




Comment at: lldb/source/Utility/Broadcaster.cpp:381
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())

Why do you re-look-up `m_event_map.end()` instead of using the `end` variable 
you defined in the `for` initializer?

std::map::erase says it only invalidates iterators to the erased element, so 
you should be fine to use `end` here (and since you do exactly that in the 
previous line) it should be good here too.



Comment at: lldb/source/Utility/Broadcaster.cpp:436
+iter = find_if(iter, end, events_predicate);
+if (iter == m_event_map.end())
   break;

fdeazeve wrote:
> this should be `== end`
Again here, you don't trust the "end" variable, but then in the next function 
you do reuse end_iter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve updated this revision to Diff 520796.
fdeazeve edited the summary of this revision.
fdeazeve added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

Files:
  lldb/source/Utility/Broadcaster.cpp

Index: lldb/source/Utility/Broadcaster.cpp
===
--- lldb/source/Utility/Broadcaster.cpp
+++ lldb/source/Utility/Broadcaster.cpp
@@ -377,13 +377,10 @@
 
   // Go through the map and delete the exact matches, and build a list of
   // matches that weren't exact to re-add:
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter,
-   listener_matches_and_shared_bits);
-if (iter == end_iter) {
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, listener_matches_and_shared_bits);
+if (iter == end)
   break;
-}
 uint32_t iter_event_bits = (*iter).first.GetEventBits();
 removed_some = true;
 
@@ -392,12 +389,12 @@
   to_be_readded.emplace_back(event_spec.GetBroadcasterClass(),
  new_event_bits);
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 
   // Okay now add back the bits that weren't completely removed:
-  for (size_t i = 0; i < to_be_readded.size(); i++) {
-m_event_map.insert(event_listener_key(to_be_readded[i], listener_sp));
+  for (const auto &event : to_be_readded) {
+m_event_map.insert(event_listener_key(event, listener_sp));
   }
 
   return removed_some;
@@ -412,9 +409,8 @@
 return input.first.IsContainedIn(event_spec);
   };
 
-  collection::const_iterator iter, end_iter = m_event_map.end();
-  iter = find_if(m_event_map.begin(), end_iter, event_spec_matches);
-  if (iter != end_iter)
+  auto iter = llvm::find_if(m_event_map, event_spec_matches);
+  if (iter != m_event_map.end())
 return (*iter).second;
 
   return nullptr;
@@ -427,24 +423,21 @@
 return input.get() == listener;
   };
 
-  listener_collection::iterator iter = m_listeners.begin(),
-end_iter = m_listeners.end();
-
-  iter = std::find_if(iter, end_iter, listeners_predicate);
-  if (iter != end_iter)
+  if (auto iter = llvm::find_if(m_listeners, listeners_predicate);
+  iter != m_listeners.end())
 m_listeners.erase(iter);
 
-  while (true) {
-auto events_predicate =
-[&listener](const event_listener_key &input) -> bool {
-  return input.second.get() == listener;
-};
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, events_predicate);
-if (iter == end_iter)
+  auto events_predicate = [listener](const event_listener_key &input) -> bool {
+return input.second.get() == listener;
+  };
+
+  // TODO: use 'std::map::erase_if' when moving to c++20.
+  for (auto iter = m_event_map.begin(), end = m_event_map.end();;) {
+iter = find_if(iter, end, events_predicate);
+if (iter == end)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -459,13 +452,13 @@
   if (m_listeners.erase(listener_sp) == 0)
 return;
 
-  while (true) {
-collection::iterator iter, end_iter = m_event_map.end();
-iter = find_if(m_event_map.begin(), end_iter, listener_matches);
+  // TODO: use 'std::map::erase_if' when moving to c++20.
+  for (auto iter = m_event_map.begin(), end_iter = m_event_map.end();;) {
+iter = find_if(iter, end_iter, listener_matches);
 if (iter == end_iter)
   break;
 
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }
 }
 
@@ -490,11 +483,9 @@
 
 void BroadcasterManager::Clear() {
   std::lock_guard guard(m_manager_mutex);
-  listener_collection::iterator end_iter = m_listeners.end();
 
-  for (listener_collection::iterator iter = m_listeners.begin();
-   iter != end_iter; iter++)
-(*iter)->BroadcasterManagerWillDestruct(this->shared_from_this());
+  for (auto &listener : m_listeners)
+listener->BroadcasterManagerWillDestruct(this->shared_from_this());
   m_listeners.clear();
   m_event_map.clear();
 }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150219: [lldb][NFCI] Remove n^2 loops and simplify iterator usage

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added inline comments.



Comment at: lldb/source/Utility/Broadcaster.cpp:381
+  for (auto begin = m_event_map.begin(), end = m_event_map.end();;) {
+auto iter = find_if(begin, end, listener_matches_and_shared_bits);
+if (iter == m_event_map.end())

jingham wrote:
> Why do you re-look-up `m_event_map.end()` instead of using the `end` variable 
> you defined in the `for` initializer?
> 
> std::map::erase says it only invalidates iterators to the erased element, so 
> you should be fine to use `end` here (and since you do exactly that in the 
> previous line) it should be good here too.
Yup, this was just a mistake on my part. Will address in the next version



Comment at: lldb/source/Utility/Broadcaster.cpp:392
 }
-m_event_map.erase(iter);
+iter = m_event_map.erase(iter);
   }

bulbazord wrote:
> I don't think you need to actually capture the iterator here? std::map::erase 
> doesn't invalidate existing iterators* (so `begin` and `end` are fine) and we 
> redefine `iter` at the beginning of this loop.
> 
> *:https://en.cppreference.com/w/cpp/container/map/erase
Argh, the variable `begin` shouldn't exist, it should all be `iter`, otherwise 
we're not addressing the quadratic behaviour.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150219

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

[Lldb-commits] [PATCH] D150222: [lldb][NFCI] Remove custom dwarf LEB128 types

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: aprantl, rastogishubham, fdeazeve, JDevlieghere, 
clayborg.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The LEB128 type defined by the DWARF standard is explicitly a variable-length
encoding of an integer. LLDB had defined `uleb128` and `sleb128` types
to be 32-bit  but in many places in both LLVM and LLDB we treat the maximum
width of LEB128 types to be 64, so let's remove these types and be
consistent.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150222

Files:
  lldb/include/lldb/Core/dwarf.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
@@ -216,22 +216,22 @@
   // in the .debug_info
   case DW_FORM_exprloc:
   case DW_FORM_block: {
-dw_uleb128_t size = debug_info_data.GetULEB128(offset_ptr);
+uint64_t size = debug_info_data.GetULEB128(offset_ptr);
 *offset_ptr += size;
   }
 return true;
   case DW_FORM_block1: {
-dw_uleb128_t size = debug_info_data.GetU8(offset_ptr);
+uint8_t size = debug_info_data.GetU8(offset_ptr);
 *offset_ptr += size;
   }
 return true;
   case DW_FORM_block2: {
-dw_uleb128_t size = debug_info_data.GetU16(offset_ptr);
+uint16_t size = debug_info_data.GetU16(offset_ptr);
 *offset_ptr += size;
   }
 return true;
   case DW_FORM_block4: {
-dw_uleb128_t size = debug_info_data.GetU32(offset_ptr);
+uint32_t size = debug_info_data.GetU32(offset_ptr);
 *offset_ptr += size;
   }
 return true;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
@@ -42,7 +42,7 @@
   void GetUnsupportedForms(std::set &invalid_forms) const;
 
   const DWARFAbbreviationDeclaration *
-  GetAbbreviationDeclaration(dw_uleb128_t abbrCode) const;
+  GetAbbreviationDeclaration(uint32_t abbrCode) const;
 
   /// Unit test accessor functions.
   /// @{
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
@@ -27,7 +27,7 @@
   m_offset = begin_offset;
   Clear();
   DWARFAbbreviationDeclaration abbrevDeclaration;
-  dw_uleb128_t prev_abbr_code = 0;
+  uint32_t prev_abbr_code = 0;
   while (true) {
 llvm::Expected es =
 abbrevDeclaration.extract(data, offset_ptr);
@@ -50,7 +50,7 @@
 // DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration()
 const DWARFAbbreviationDeclaration *
 DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration(
-dw_uleb128_t abbrCode) const {
+uint32_t abbrCode) const {
   if (m_idx_offset == UINT32_MAX) {
 DWARFAbbreviationDeclarationCollConstIter pos;
 DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
@@ -66,7 +66,6 @@
   return nullptr;
 }
 
-
 // DWARFAbbreviationDeclarationSet::GetUnsupportedForms()
 void DWARFAbbreviationDeclarationSet::GetUnsupportedForms(
 std::set &invalid_forms) const {
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
@@ -22,8 +22,8 @@
   // For hand crafting an abbreviation declaration
   DWARFAbbreviationDeclaration(dw_tag_t tag, uint8_t has_children);
 
-  dw_uleb128_t Code() const { return m_code; }
-  void SetCode(dw_uleb128_t code) { m_code = code; }
+  uint32_t Code() const { return m_code; }
+  void SetCode(uint32_t code) { m_code = code; }
   dw_tag_t Tag() const { return m_tag; }
   bool HasChildren() const { return m_has_children; }
   size_t NumAttributes() const { return m_attributes.size(); }
@@ -55,7 +55,7 @@
   bool IsValid();
 
 protected:
-  dw_uleb128_t m_code = InvalidCode;
+  uint32_t m_code = InvalidCode;
   dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
   uint8_t m_has_children = 0;
   DWARFAttribute::collection m_attributes;
Index: lldb/include/lldb/Core/dwarf.h
===
--- lldb/include/lldb/Core/dwarf.h
+++ lldb/include/lldb/Core/dwarf.h
@@ -21,8 +21,6 @@
 }
 }
 
-typedef uint32_t dw_uleb128_t;
-typedef int32_t dw_sleb128_

[Lldb-commits] [PATCH] D150222: [lldb][NFCI] Remove custom dwarf LEB128 types

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

fdeazeve added a comment.

Nice cleanup! This gets rid of some implicit conversions


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150222

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

[Lldb-commits] [PATCH] D150157: [lldb] Mark most SBAPI methods involving private types as protected or private

[Med Ismail Bennani via Phabricator via lldb-commits]

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

LGTM with some comments.




Comment at: lldb/include/lldb/API/SBBreakpoint.h:18-19
 class ScriptInterpreter;
+namespace python {
+class SWIGBridge;
 }

We've talked about this offline, but I think we should stay language agnostic 
in the SBAPI, so exposing a python namespace here is bothering me a little bit.



Comment at: lldb/source/API/SBCommandInterpreter.cpp:37
+namespace lldb_private {
 class CommandPluginInterfaceImplementation : public CommandObjectParsed {
 public:

I find it a bit odd to have this here ... May be we should move this class out 
the the `API` directory ?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:64-90
+class SWIGBridge {
+public:
+  static PythonObject ToSWIGWrapper(std::unique_ptr value_sb);
+  static PythonObject ToSWIGWrapper(lldb::ValueObjectSP value_sp);
+  static PythonObject ToSWIGWrapper(lldb::TargetSP target_sp);
+  static PythonObject ToSWIGWrapper(lldb::ProcessSP process_sp);
+  static PythonObject ToSWIGWrapper(lldb::ThreadPlanSP thread_plan_sp);

Although this is nested in the `python` namespace, I think `SWIGBridge` should 
be a templated class defined in the `ScriptedInterpreter` header and this class 
should be instead renamed as `SWIGPythonBridge` and be a specialization of the 
`SWIGBridge` class. We can do that as a follow-up but a `TODO` comment would be 
nice.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:248-254
+void *LLDBSWIGPython_CastPyObjectToSBData(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBBreakpoint(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBAttachInfo(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBLaunchInfo(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBError(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBValue(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBMemoryRegionInfo(PyObject *data);

Can we move these in the `python` namespace as well ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157

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

[Lldb-commits] [PATCH] D150157: [lldb] Mark most SBAPI methods involving private types as protected or private

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/API/SBBreakpoint.h:18-19
 class ScriptInterpreter;
+namespace python {
+class SWIGBridge;
 }

mib wrote:
> We've talked about this offline, but I think we should stay language agnostic 
> in the SBAPI, so exposing a python namespace here is bothering me a little 
> bit.
I agree, but we can refactor this later. Removing and/or changing forward 
declarations doesn't break ABI.



Comment at: lldb/source/API/SBCommandInterpreter.cpp:37
+namespace lldb_private {
 class CommandPluginInterfaceImplementation : public CommandObjectParsed {
 public:

mib wrote:
> I find it a bit odd to have this here ... May be we should move this class 
> out the the `API` directory ?
It's a little odd for sure, but it is an implementation detail so having it in 
the cpp file seems alright. Where would you put it?



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:64-90
+class SWIGBridge {
+public:
+  static PythonObject ToSWIGWrapper(std::unique_ptr value_sb);
+  static PythonObject ToSWIGWrapper(lldb::ValueObjectSP value_sp);
+  static PythonObject ToSWIGWrapper(lldb::TargetSP target_sp);
+  static PythonObject ToSWIGWrapper(lldb::ProcessSP process_sp);
+  static PythonObject ToSWIGWrapper(lldb::ThreadPlanSP thread_plan_sp);

mib wrote:
> Although this is nested in the `python` namespace, I think `SWIGBridge` 
> should be a templated class defined in the `ScriptedInterpreter` header and 
> this class should be instead renamed as `SWIGPythonBridge` and be a 
> specialization of the `SWIGBridge` class. We can do that as a follow-up but a 
> `TODO` comment would be nice.
Sure, I can add the TODO. I'd rather not add the templates or subclass in this 
patch as it is complicated enough already.



Comment at: 
lldb/source/Plugins/ScriptInterpreter/Python/SWIGPythonBridge.h:248-254
+void *LLDBSWIGPython_CastPyObjectToSBData(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBBreakpoint(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBAttachInfo(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBLaunchInfo(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBError(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBValue(PyObject *data);
+void *LLDBSWIGPython_CastPyObjectToSBMemoryRegionInfo(PyObject *data);

mib wrote:
> Can we move these in the `python` namespace as well ?
Sure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157

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

[Lldb-commits] [PATCH] D150222: [lldb][NFCI] Remove custom dwarf LEB128 types

[Shubham Sandeep Rastogi via Phabricator via lldb-commits]

rastogishubham accepted this revision.
rastogishubham added a comment.

LGTM!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150222

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

[Lldb-commits] [lldb] b77e41f - [lldb][NFCI] Remove custom dwarf LEB128 types

[Alex Langford via lldb-commits]

Author: Alex Langford
Date: 2023-05-09T13:46:27-07:00
New Revision: b77e41f2886aca278b41a85fc0f947e078b3da13

URL: 
https://github.com/llvm/llvm-project/commit/b77e41f2886aca278b41a85fc0f947e078b3da13
DIFF: 
https://github.com/llvm/llvm-project/commit/b77e41f2886aca278b41a85fc0f947e078b3da13.diff

LOG: [lldb][NFCI] Remove custom dwarf LEB128 types

The LEB128 type defined by the DWARF standard is explicitly a variable-length
encoding of an integer. LLDB had defined `uleb128` and `sleb128` types
to be 32-bit  but in many places in both LLVM and LLDB we treat the maximum
width of LEB128 types to be 64, so let's remove these types and be
consistent.

Differential Revision: https://reviews.llvm.org/D150222

Added: 


Modified: 
lldb/include/lldb/Core/dwarf.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

Removed: 




diff  --git a/lldb/include/lldb/Core/dwarf.h b/lldb/include/lldb/Core/dwarf.h
index af0762ea7b704..e930200393c27 100644
--- a/lldb/include/lldb/Core/dwarf.h
+++ b/lldb/include/lldb/Core/dwarf.h
@@ -21,8 +21,6 @@ namespace dwarf {
 }
 }
 
-typedef uint32_t dw_uleb128_t;
-typedef int32_t dw_sleb128_t;
 typedef uint16_t dw_attr_t;
 typedef uint16_t dw_form_t;
 typedef llvm::dwarf::Tag dw_tag_t;

diff  --git 
a/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
index 378ba888f4e0f..7922a14b33f18 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
@@ -22,8 +22,8 @@ class DWARFAbbreviationDeclaration {
   // For hand crafting an abbreviation declaration
   DWARFAbbreviationDeclaration(dw_tag_t tag, uint8_t has_children);
 
-  dw_uleb128_t Code() const { return m_code; }
-  void SetCode(dw_uleb128_t code) { m_code = code; }
+  uint32_t Code() const { return m_code; }
+  void SetCode(uint32_t code) { m_code = code; }
   dw_tag_t Tag() const { return m_tag; }
   bool HasChildren() const { return m_has_children; }
   size_t NumAttributes() const { return m_attributes.size(); }
@@ -55,7 +55,7 @@ class DWARFAbbreviationDeclaration {
   bool IsValid();
 
 protected:
-  dw_uleb128_t m_code = InvalidCode;
+  uint32_t m_code = InvalidCode;
   dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
   uint8_t m_has_children = 0;
   DWARFAttribute::collection m_attributes;

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
index d890288cdf567..2c02fbe64f54a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
@@ -27,7 +27,7 @@ DWARFAbbreviationDeclarationSet::extract(const 
DWARFDataExtractor &data,
   m_offset = begin_offset;
   Clear();
   DWARFAbbreviationDeclaration abbrevDeclaration;
-  dw_uleb128_t prev_abbr_code = 0;
+  uint32_t prev_abbr_code = 0;
   while (true) {
 llvm::Expected es =
 abbrevDeclaration.extract(data, offset_ptr);
@@ -50,7 +50,7 @@ DWARFAbbreviationDeclarationSet::extract(const 
DWARFDataExtractor &data,
 // DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration()
 const DWARFAbbreviationDeclaration *
 DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration(
-dw_uleb128_t abbrCode) const {
+uint32_t abbrCode) const {
   if (m_idx_offset == UINT32_MAX) {
 DWARFAbbreviationDeclarationCollConstIter pos;
 DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
@@ -66,7 +66,6 @@ DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration(
   return nullptr;
 }
 
-
 // DWARFAbbreviationDeclarationSet::GetUnsupportedForms()
 void DWARFAbbreviationDeclarationSet::GetUnsupportedForms(
 std::set &invalid_forms) const {

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
index ec6b93ce0e7f8..c7a776b0dfbbf 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
@@ -42,7 +42,7 @@ class DWARFAbbreviationDeclarationSet {
   void GetUnsupportedForms(std::set &invalid_forms) const;
 
   const DWARFAbbreviationDeclaration *
-  GetAbbreviationDeclaration(dw_uleb128_t abbrCode) const;
+  GetAbbreviationDeclaration(uint32_t abbrCode) const;
 
   /// Unit test accessor functions.
   /// @{

diff  --git a/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp 
b/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
index a1578b47ae94f..2b1d3b37a95a5 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
@@

[Lldb-commits] [PATCH] D150222: [lldb][NFCI] Remove custom dwarf LEB128 types

[Alex Langford via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb77e41f2886a: [lldb][NFCI] Remove custom dwarf LEB128 types 
(authored by bulbazord).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150222

Files:
  lldb/include/lldb/Core/dwarf.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
@@ -216,22 +216,22 @@
   // in the .debug_info
   case DW_FORM_exprloc:
   case DW_FORM_block: {
-dw_uleb128_t size = debug_info_data.GetULEB128(offset_ptr);
+uint64_t size = debug_info_data.GetULEB128(offset_ptr);
 *offset_ptr += size;
   }
 return true;
   case DW_FORM_block1: {
-dw_uleb128_t size = debug_info_data.GetU8(offset_ptr);
+uint8_t size = debug_info_data.GetU8(offset_ptr);
 *offset_ptr += size;
   }
 return true;
   case DW_FORM_block2: {
-dw_uleb128_t size = debug_info_data.GetU16(offset_ptr);
+uint16_t size = debug_info_data.GetU16(offset_ptr);
 *offset_ptr += size;
   }
 return true;
   case DW_FORM_block4: {
-dw_uleb128_t size = debug_info_data.GetU32(offset_ptr);
+uint32_t size = debug_info_data.GetU32(offset_ptr);
 *offset_ptr += size;
   }
 return true;
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
@@ -42,7 +42,7 @@
   void GetUnsupportedForms(std::set &invalid_forms) const;
 
   const DWARFAbbreviationDeclaration *
-  GetAbbreviationDeclaration(dw_uleb128_t abbrCode) const;
+  GetAbbreviationDeclaration(uint32_t abbrCode) const;
 
   /// Unit test accessor functions.
   /// @{
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
@@ -27,7 +27,7 @@
   m_offset = begin_offset;
   Clear();
   DWARFAbbreviationDeclaration abbrevDeclaration;
-  dw_uleb128_t prev_abbr_code = 0;
+  uint32_t prev_abbr_code = 0;
   while (true) {
 llvm::Expected es =
 abbrevDeclaration.extract(data, offset_ptr);
@@ -50,7 +50,7 @@
 // DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration()
 const DWARFAbbreviationDeclaration *
 DWARFAbbreviationDeclarationSet::GetAbbreviationDeclaration(
-dw_uleb128_t abbrCode) const {
+uint32_t abbrCode) const {
   if (m_idx_offset == UINT32_MAX) {
 DWARFAbbreviationDeclarationCollConstIter pos;
 DWARFAbbreviationDeclarationCollConstIter end = m_decls.end();
@@ -66,7 +66,6 @@
   return nullptr;
 }
 
-
 // DWARFAbbreviationDeclarationSet::GetUnsupportedForms()
 void DWARFAbbreviationDeclarationSet::GetUnsupportedForms(
 std::set &invalid_forms) const {
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
@@ -22,8 +22,8 @@
   // For hand crafting an abbreviation declaration
   DWARFAbbreviationDeclaration(dw_tag_t tag, uint8_t has_children);
 
-  dw_uleb128_t Code() const { return m_code; }
-  void SetCode(dw_uleb128_t code) { m_code = code; }
+  uint32_t Code() const { return m_code; }
+  void SetCode(uint32_t code) { m_code = code; }
   dw_tag_t Tag() const { return m_tag; }
   bool HasChildren() const { return m_has_children; }
   size_t NumAttributes() const { return m_attributes.size(); }
@@ -55,7 +55,7 @@
   bool IsValid();
 
 protected:
-  dw_uleb128_t m_code = InvalidCode;
+  uint32_t m_code = InvalidCode;
   dw_tag_t m_tag = llvm::dwarf::DW_TAG_null;
   uint8_t m_has_children = 0;
   DWARFAttribute::collection m_attributes;
Index: lldb/include/lldb/Core/dwarf.h
===
--- lldb/include/lldb/Core/dwarf.h
+++ lldb/include/lldb/Core/dwarf.h
@@ -21,8 +21,6 @@
 }
 }
 
-typedef uint32_t dw_uleb128_t;
-typedef int32_t dw_sleb128_t;
 typedef uint16_t dw_attr_t;
 typedef uint16_t dw_form_t;
 typedef llvm::dwarf::Tag dw_tag_t;
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D150157: [lldb] Mark most SBAPI methods involving private types as protected or private

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

This seems like a pretty non-intrusive way of protecting the lldb_private side 
of the SB API construction.

Looking at the patch makes it seem like we've been semi-randomly assorting 
members of the SB classes to "protected" and "private".  We have NO intentions 
of ever subclassing these classes, so protected vrs. private is a meaningless 
distinction (thus the seeming randomness of the assignment, maybe?)  It would 
be cleaner to go make them all private, since we don't intend to offer these 
for subclassing...  But this patch is getting big already, probably don't want 
to fold that into this one.




Comment at: lldb/unittests/API/SBCommandInterpreterTest.cpp:24
 SBDebugger::Initialize();
 m_dbg = SBDebugger::Create(/*source_init_files=*/false);
   }

It isn't clear to me how the changes in this file fit in with your overall goal?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157

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

[Lldb-commits] [PATCH] D150157: [lldb] Mark most SBAPI methods involving private types as protected or private

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/unittests/API/SBCommandInterpreterTest.cpp:24
 SBDebugger::Initialize();
 m_dbg = SBDebugger::Create(/*source_init_files=*/false);
   }

jingham wrote:
> It isn't clear to me how the changes in this file fit in with your overall 
> goal?
I'm removing the `SBCommandInterpreter` member of this class because in order 
to construct an object of this class, `SBCommandInterpreter` would need to have 
an empty default constructor (which we do not have currently). I didn't want to 
add a constructor for SBCommandInterpreter just to avoid changing the test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157

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

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: aprantl, rastogishubham, fdeazeve, clayborg.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

LLDB currently defines `dw_form_t` as a `uint16_t` which makes sense.
However, LLVM also defines a similar type `llvm::dwarf::Form` which is
an enum backed by a `uint16_t`. Switching to the llvm implementation
means that we can more easily interoperate with the LLVM DWARF code.

Additionally, we get some type checking out of this: I found that
DWARFAttribute had a method called `FormAtIndex` that returned a
`dw_attr_t`. Although `dw_attr_t` is also a `uint16_t` under the hood,
the type checking benefits here are undeniable: If this had returned a
something of different signedness/width, we could have had some bad
bugs.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150228

Files:
  lldb/include/lldb/Core/dwarf.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugAbbrev.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -542,7 +542,7 @@
 
   DWARFDebugAbbrev *abbrev = DebugAbbrev();
   if (abbrev) {
-std::set invalid_forms;
+std::set invalid_forms;
 abbrev->GetUnsupportedForms(invalid_forms);
 if (!invalid_forms.empty()) {
   StreamString error;
Index: lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
+++ lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.h
@@ -71,7 +71,7 @@
 
   struct Atom {
 AtomType type;
-dw_form_t form;
+llvm::dwarf::Form form;
   };
 
   typedef std::vector DIEInfoArray;
@@ -87,7 +87,7 @@
 
 void Clear();
 
-void AppendAtom(AtomType type, dw_form_t form);
+void AppendAtom(AtomType type, llvm::dwarf::Form form);
 
 lldb::offset_t Read(const lldb_private::DataExtractor &data,
 lldb::offset_t offset);
Index: lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
@@ -154,7 +154,8 @@
   ClearAtoms();
 }
 
-void DWARFMappedHash::Prologue::AppendAtom(AtomType type, dw_form_t form) {
+void DWARFMappedHash::Prologue::AppendAtom(AtomType type,
+   llvm::dwarf::Form form) {
   atoms.push_back({type, form});
   atom_mask |= 1u << type;
   switch (form) {
@@ -227,7 +228,7 @@
   } else {
 for (uint32_t i = 0; i < atom_count; ++i) {
   AtomType type = (AtomType)data.GetU16(&offset);
-  dw_form_t form = (dw_form_t)data.GetU16(&offset);
+  auto form = static_cast(data.GetU16(&offset));
   AppendAtom(type, form);
 }
   }
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
@@ -13,6 +13,8 @@
 #include 
 #include 
 
+#include "llvm/BinaryFormat/Dwarf.h"
+
 class DWARFUnit;
 class SymbolFileDWARF;
 class DWARFDIE;
@@ -40,13 +42,13 @@
 
   DWARFFormValue() = default;
   DWARFFormValue(const DWARFUnit *unit) : m_unit(unit) {}
-  DWARFFormValue(const DWARFUnit *unit, dw_form_t form)
+  DWARFFormValue(const DWARFUnit *unit, llvm::dwarf::Form form)
   : m_unit(unit), m_form(form) {}
   const DWARFUnit *GetUnit() const { return m_unit; }
   void SetUnit(const DWARFUnit *unit) { m_unit = unit; }
-  dw_form_t Form() const { return m_form; }
-  dw_form_t& FormRef() { return m_form; }
-  void SetForm(dw_form_t form) { m_form = form; }
+  llvm::dwarf::Form Form() const { return m_form; }
+  llvm::dwarf::Form &FormRef() { return m_form; }
+  void SetForm(llvm::dwarf::Form form) { m_form = form; }
   const ValueType &Value() const { return m_value; }
   ValueType &ValueRef() { return m_value; }
   void SetValue(const ValueType &val) { m_value = val; }
@@ -55,7 +57,7 @@
   bool 

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Shubham Sandeep Rastogi via Phabricator via lldb-commits]

rastogishubham accepted this revision.
rastogishubham added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: JDevlieghere.

Great refactoring!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150228

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

[Lldb-commits] [PATCH] D150157: [lldb] Mark most SBAPI methods involving private types as protected or private

[Jim Ingham via Phabricator via lldb-commits]

jingham added inline comments.



Comment at: lldb/unittests/API/SBCommandInterpreterTest.cpp:24
 SBDebugger::Initialize();
 m_dbg = SBDebugger::Create(/*source_init_files=*/false);
   }

bulbazord wrote:
> jingham wrote:
> > It isn't clear to me how the changes in this file fit in with your overall 
> > goal?
> I'm removing the `SBCommandInterpreter` member of this class because in order 
> to construct an object of this class, `SBCommandInterpreter` would need to 
> have an empty default constructor (which we do not have currently). I didn't 
> want to add a constructor for SBCommandInterpreter just to avoid changing the 
> test.
Sounds reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150157

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

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Apparently a similar change was made with dw_tag_t, in the line below your 
first deletion I see:

typedef llvm::dwarf::Tag dw_tag_t;

It seems weird to have dw_tag_t but lvm::dwarf::Form.  If there's a good reason 
to use the more verbose form, we should probably do the same with the Tag for 
consistency.  Otherwise, you can just play the same re-typedef-ing as was done 
for tag, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150228

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

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a reviewer: JDevlieghere.
bulbazord added a comment.

In D150228#4330798 , @jingham wrote:

> Apparently a similar change was made with dw_tag_t, in the line below your 
> first deletion I see:
>
> typedef llvm::dwarf::Tag dw_tag_t;
>
> It seems weird to have dw_tag_t but lvm::dwarf::Form.  If there's a good 
> reason to use the more verbose form, we should probably do the same with the 
> Tag for consistency.  Otherwise, you can just play the same re-typedef-ing as 
> was done for tag, right?

Yes, Jonas did that work in 7fa72881d4cbf. I intentionally chose to not typedef 
here as a matter of personal preference, I prefer the explicitness of the full 
type. I do agree that we should be consistent here though. I don't think it 
quite matters which solution we go with, but do you (or anybody else) have 
strong opinions about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150228

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

[Lldb-commits] [PATCH] D149379: [lldb] Add tests for command removal

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

What happens if you remove a command that had an alias bound to it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149379

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

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 520853.
bulbazord added a comment.

Do a typedef in dwarf.h (like llvm::dwarf::Tag) instead of explicitly writing 
the type out everywhere


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150228

Files:
  lldb/include/lldb/Core/dwarf.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
@@ -227,7 +227,7 @@
   } else {
 for (uint32_t i = 0; i < atom_count; ++i) {
   AtomType type = (AtomType)data.GetU16(&offset);
-  dw_form_t form = (dw_form_t)data.GetU16(&offset);
+  auto form = static_cast(data.GetU16(&offset));
   AppendAtom(type, form);
 }
   }
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
@@ -13,6 +13,8 @@
 #include 
 #include 
 
+#include "llvm/BinaryFormat/Dwarf.h"
+
 class DWARFUnit;
 class SymbolFileDWARF;
 class DWARFDIE;
@@ -45,7 +47,7 @@
   const DWARFUnit *GetUnit() const { return m_unit; }
   void SetUnit(const DWARFUnit *unit) { m_unit = unit; }
   dw_form_t Form() const { return m_form; }
-  dw_form_t& FormRef() { return m_form; }
+  dw_form_t &FormRef() { return m_form; }
   void SetForm(dw_form_t form) { m_form = form; }
   const ValueType &Value() const { return m_value; }
   ValueType &ValueRef() { return m_value; }
@@ -83,7 +85,7 @@
   // Compile unit where m_value was located.
   // It may be different from compile unit where m_value refers to.
   const DWARFUnit *m_unit = nullptr; // Unit for this form
-  dw_form_t m_form = 0;  // Form for this value
+  dw_form_t m_form = dw_form_t(0);   // Form for this value
   ValueType m_value;// Contains all data for the form
 };
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
@@ -25,7 +25,7 @@
 
 void DWARFFormValue::Clear() {
   m_unit = nullptr;
-  m_form = 0;
+  m_form = dw_form_t(0);
   m_value = ValueTypeTag();
 }
 
@@ -127,7 +127,7 @@
   m_value.value.uval = data.GetMaxU64(offset_ptr, ref_addr_size);
   break;
 case DW_FORM_indirect:
-  m_form = data.GetULEB128(offset_ptr);
+  m_form = static_cast(data.GetULEB128(offset_ptr));
   indirect = true;
   break;
 case DW_FORM_flag_present:
@@ -321,9 +321,10 @@
   return true;
 
   case DW_FORM_indirect: {
-dw_form_t indirect_form = debug_info_data.GetULEB128(offset_ptr);
-return DWARFFormValue::SkipValue(indirect_form, debug_info_data, offset_ptr,
- unit);
+  auto indirect_form =
+  static_cast(debug_info_data.GetULEB128(offset_ptr));
+  return DWARFFormValue::SkipValue(indirect_form, debug_info_data,
+   offset_ptr, unit);
   }
 
   default:
@@ -573,8 +574,10 @@
   case DW_FORM_block2:
   case DW_FORM_block4:
 return true;
+  default:
+return false;
   }
-  return false;
+  llvm_unreachable("All cases handled above!");
 }
 
 bool DWARFFormValue::IsDataForm(const dw_form_t form) {
@@ -586,8 +589,10 @@
   case DW_FORM_data4:
   case DW_FORM_data8:
 return true;
+  default:
+return false;
   }
-  return false;
+  llvm_unreachable("All cases handled above!");
 }
 
 bool DWARFFormValue::FormIsSupported(dw_form_t form) {
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -177,7 +177,7 @@
 
 case DW_FORM_indirect:
   form_is_indirect = true;
-  form = data.GetULEB128(&offset);
+  form = static_cast(data.GetULEB128(&offset));
   break;
 
 case DW_FORM_strp:
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
@@ -55,7

[Lldb-commits] [PATCH] D150228: [lldb][NFCI] Replace dw_form_t with llvm::dwarf::Form

[Alex Langford via Phabricator via lldb-commits]

bulbazord updated this revision to Diff 520854.
bulbazord added a comment.

Remove unused include in header


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150228

Files:
  lldb/include/lldb/Core/dwarf.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAbbreviationDeclaration.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
  lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp

Index: lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp
@@ -227,7 +227,7 @@
   } else {
 for (uint32_t i = 0; i < atom_count; ++i) {
   AtomType type = (AtomType)data.GetU16(&offset);
-  dw_form_t form = (dw_form_t)data.GetU16(&offset);
+  auto form = static_cast(data.GetU16(&offset));
   AppendAtom(type, form);
 }
   }
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.h
@@ -45,7 +45,7 @@
   const DWARFUnit *GetUnit() const { return m_unit; }
   void SetUnit(const DWARFUnit *unit) { m_unit = unit; }
   dw_form_t Form() const { return m_form; }
-  dw_form_t& FormRef() { return m_form; }
+  dw_form_t &FormRef() { return m_form; }
   void SetForm(dw_form_t form) { m_form = form; }
   const ValueType &Value() const { return m_value; }
   ValueType &ValueRef() { return m_value; }
@@ -83,7 +83,7 @@
   // Compile unit where m_value was located.
   // It may be different from compile unit where m_value refers to.
   const DWARFUnit *m_unit = nullptr; // Unit for this form
-  dw_form_t m_form = 0;  // Form for this value
+  dw_form_t m_form = dw_form_t(0);   // Form for this value
   ValueType m_value;// Contains all data for the form
 };
 
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp
@@ -25,7 +25,7 @@
 
 void DWARFFormValue::Clear() {
   m_unit = nullptr;
-  m_form = 0;
+  m_form = dw_form_t(0);
   m_value = ValueTypeTag();
 }
 
@@ -127,7 +127,7 @@
   m_value.value.uval = data.GetMaxU64(offset_ptr, ref_addr_size);
   break;
 case DW_FORM_indirect:
-  m_form = data.GetULEB128(offset_ptr);
+  m_form = static_cast(data.GetULEB128(offset_ptr));
   indirect = true;
   break;
 case DW_FORM_flag_present:
@@ -321,9 +321,10 @@
   return true;
 
   case DW_FORM_indirect: {
-dw_form_t indirect_form = debug_info_data.GetULEB128(offset_ptr);
-return DWARFFormValue::SkipValue(indirect_form, debug_info_data, offset_ptr,
- unit);
+  auto indirect_form =
+  static_cast(debug_info_data.GetULEB128(offset_ptr));
+  return DWARFFormValue::SkipValue(indirect_form, debug_info_data,
+   offset_ptr, unit);
   }
 
   default:
@@ -573,8 +574,10 @@
   case DW_FORM_block2:
   case DW_FORM_block4:
 return true;
+  default:
+return false;
   }
-  return false;
+  llvm_unreachable("All cases handled above!");
 }
 
 bool DWARFFormValue::IsDataForm(const dw_form_t form) {
@@ -586,8 +589,10 @@
   case DW_FORM_data4:
   case DW_FORM_data8:
 return true;
+  default:
+return false;
   }
-  return false;
+  llvm_unreachable("All cases handled above!");
 }
 
 bool DWARFFormValue::FormIsSupported(dw_form_t form) {
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
@@ -177,7 +177,7 @@
 
 case DW_FORM_indirect:
   form_is_indirect = true;
-  form = data.GetULEB128(&offset);
+  form = static_cast(data.GetULEB128(&offset));
   break;
 
 case DW_FORM_strp:
Index: lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
===
--- lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
+++ lldb/source/Plugins/SymbolFile/DWARF/DWARFAttribute.h
@@ -55,7 +55,7 @@
   dw_attr_t AttributeAtIndex(uint32_t i) const {
 return m_infos[i].attr.get_attr();
   }
-  dw_attr_t FormAtIndex(uint32_t i) const { return m_infos[i].attr.get_form(); }
+  dw_form_t FormAtIndex(

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

Most of this is fine.  I wonder about avoiding caching the full name and name 
w/o category & selector name.  One of the main uses of this class is to take 
incoming ObjC names from the ConstString pool, chop them up into full name, 
name w/o category, and selectorName, which we then insert into the lookup names 
tables for the modules they are found in.  All those lookup table names will 
also exist in the ConstString pool.

The other instance where we get these names is when someone passes them to 
break set.  In that case, we're going to break the name apart and then look it 
up in the ConstString pools for symbol names to find a match.

So it seems like this is a case where you aren't really getting savings in the 
ConstString pool, you're just causing more copying from std::string to 
ConstString.




Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:37
+///
+/// \return If the name did parse as a valid Objective-C method name,
+/// returns std::nullopt. Otherwise returns a const MethodName.





Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:100
+///   will give you "my_additions"
+/// \return A StringRef to the category name of this method.
+llvm::StringRef GetCategory() const;

What does this return if there's no category?

We have two behaviors above:

GetFullNameWithoutCategory - returns an empty string if the original class had 
no category
GetClassNameWithCategory - returns the original class name if it had no category
GetCategory - returns ??? if it had no category.

These seem a little skew somehow...



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:117
 
-  protected:
-ConstString
-m_full; // Full name:   "+[NSString(my_additions) 
myStringWithCString:]"
-ConstString m_class; // Class name:  "NSString"
-ConstString
-m_class_category;   // Class with category: "NSString(my_additions)"
-ConstString m_category; // Category:"my_additions"
-ConstString m_selector; // Selector:"myStringWithCString:"
-Type m_type = eTypeUnspecified;
-bool m_category_is_valid = false;
+const std::string m_full;
+Type m_type;

For the most part, we find ObjC method names either from debug info, from the 
symbol table or from the runtime symbol vendor.  All of those are going to 
store their names in the ConstString pool.

If we only make these MethodName entities one by one then we're just doing a 
string copy every so often, and that shouldn't be noticeable.  But if we make 
and hold a lot of these, we're doubling our storage requirements for a thing 
that at least on Darwin there are a lot of...

This one seems a reasonable candidate to be a ConstString... 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149914

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

[Lldb-commits] [PATCH] D140630: [lldb-vscode] Add data breakpoint support

[Jim Ingham via Phabricator via lldb-commits]

jingham added a comment.

In D140630#4276855 , @cimacmillan 
wrote:

> Ping.

Greg's been out since late March, and isn't expected back for a while still.  I 
am entirely unfamiliar with the lldb-vscode part of lldb, so I don't feel 
comfortable reviewing this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140630

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

[Lldb-commits] [lldb] 448bd59 - [nfc] Remove dead code from ObjectFileMachO

[Jason Molenda via lldb-commits]

Author: Jason Molenda
Date: 2023-05-09T17:01:06-07:00
New Revision: 448bd59e18721e5357a37f080051db87cc3a4448

URL: 
https://github.com/llvm/llvm-project/commit/448bd59e18721e5357a37f080051db87cc3a4448
DIFF: 
https://github.com/llvm/llvm-project/commit/448bd59e18721e5357a37f080051db87cc3a4448.diff

LOG: [nfc] Remove dead code from ObjectFileMachO

The old way of lldb reading the on-disk shared cache is still in
the sources, but we use dyld SPI to inspect this binary now.  This
code is no longer called.

Added: 


Modified: 
lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp

Removed: 




diff  --git a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp 
b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
index 8ff613d0cf9bc..e941850d29b78 100644
--- a/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ b/lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -141,26 +141,6 @@ using namespace llvm::MachO;
 
 LLDB_PLUGIN_DEFINE(ObjectFileMachO)
 
-// Some structure definitions needed for parsing the dyld shared cache files
-// found on iOS devices.
-
-struct lldb_copy_dyld_cache_header_v1 {
-  char magic[16]; // e.g. "dyld_v0i386", "dyld_v1   armv7", etc.
-  uint32_t mappingOffset; // file offset to first dyld_cache_mapping_info
-  uint32_t mappingCount;  // number of dyld_cache_mapping_info entries
-  uint32_t imagesOffset;
-  uint32_t imagesCount;
-  uint64_t dyldBaseAddress;
-  uint64_t codeSignatureOffset;
-  uint64_t codeSignatureSize;
-  uint64_t slideInfoOffset;
-  uint64_t slideInfoSize;
-  uint64_t localSymbolsOffset;
-  uint64_t localSymbolsSize;
-  uint8_t uuid[16]; // v1 and above, also recorded in dyld_all_image_infos v13
-// and later
-};
-
 static void PrintRegisterValue(RegisterContext *reg_ctx, const char *name,
const char *alt_name, size_t reg_byte_size,
Stream &data) {
@@ -2172,35 +2152,6 @@ static SymbolType GetSymbolType(const char *&symbol_name,
   return type;
 }
 
-// Read the UUID out of a dyld_shared_cache file on-disk.
-UUID ObjectFileMachO::GetSharedCacheUUID(FileSpec dyld_shared_cache,
- const ByteOrder byte_order,
- const uint32_t addr_byte_size) {
-  UUID dsc_uuid;
-  DataBufferSP DscData = MapFileData(
-  dyld_shared_cache, sizeof(struct lldb_copy_dyld_cache_header_v1), 0);
-  if (!DscData)
-return dsc_uuid;
-  DataExtractor dsc_header_data(DscData, byte_order, addr_byte_size);
-
-  char version_str[7];
-  lldb::offset_t offset = 0;
-  memcpy(version_str, dsc_header_data.GetData(&offset, 6), 6);
-  version_str[6] = '\0';
-  if (strcmp(version_str, "dyld_v") == 0) {
-offset = offsetof(struct lldb_copy_dyld_cache_header_v1, uuid);
-dsc_uuid =
-UUID(dsc_header_data.GetData(&offset, sizeof(uuid_t)), sizeof(uuid_t));
-  }
-  Log *log = GetLog(LLDBLog::Symbols);
-  if (log && dsc_uuid.IsValid()) {
-LLDB_LOGF(log, "Shared cache %s has UUID %s",
-  dyld_shared_cache.GetPath().c_str(),
-  dsc_uuid.GetAsString().c_str());
-  }
-  return dsc_uuid;
-}
-
 static std::optional
 ParseNList(DataExtractor &nlist_data, lldb::offset_t &nlist_data_offset,
size_t nlist_byte_size) {



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

[Lldb-commits] [PATCH] D149379: [lldb] Add tests for command removal

[walter erquinigo via Phabricator via lldb-commits]

wallace added a comment.

The alias still works because it still holds a reference to it. I could add 
that as a test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149379

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

[Lldb-commits] [PATCH] D150235: [lldb] Change definition of DisassemblerCreateInstance

[Alex Langford via Phabricator via lldb-commits]

bulbazord created this revision.
bulbazord added reviewers: jingham, mib, JDevlieghere.
Herald added a project: All.
bulbazord requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

DissassemblerCreateInstance is a function pointer whos return type is
`Disassembler *`. But Disassembler::FindPlugin always returns a
DisassemblerSP, so there's no reason why we can't just create a
DisassemblerSP in the first place.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150235

Files:
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Core/Disassembler.cpp
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
  lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
@@ -34,8 +34,8 @@
 
   static llvm::StringRef GetPluginNameStatic() { return "llvm-mc"; }
 
-  static lldb_private::Disassembler *
-  CreateInstance(const lldb_private::ArchSpec &arch, const char *flavor);
+  static lldb::DisassemblerSP CreateInstance(const lldb_private::ArchSpec 
&arch,
+ const char *flavor);
 
   size_t DecodeInstructions(const lldb_private::Address &base_addr,
 const lldb_private::DataExtractor &data,
Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp
@@ -1572,16 +1572,14 @@
 
 DisassemblerLLVMC::~DisassemblerLLVMC() = default;
 
-Disassembler *DisassemblerLLVMC::CreateInstance(const ArchSpec &arch,
-const char *flavor) {
+lldb::DisassemblerSP DisassemblerLLVMC::CreateInstance(const ArchSpec &arch,
+   const char *flavor) {
   if (arch.GetTriple().getArch() != llvm::Triple::UnknownArch) {
-std::unique_ptr disasm_up(
-new DisassemblerLLVMC(arch, flavor));
-
-if (disasm_up.get() && disasm_up->IsValid())
-  return disasm_up.release();
+auto disasm_sp = std::make_shared(arch, flavor);
+if (disasm_sp && disasm_sp->IsValid())
+  return disasm_sp;
   }
-  return nullptr;
+  return lldb::DisassemblerSP();
 }
 
 size_t DisassemblerLLVMC::DecodeInstructions(const Address &base_addr,
Index: lldb/source/Core/Disassembler.cpp
===
--- lldb/source/Core/Disassembler.cpp
+++ lldb/source/Core/Disassembler.cpp
@@ -67,20 +67,16 @@
 create_callback =
 PluginManager::GetDisassemblerCreateCallbackForPluginName(plugin_name);
 if (create_callback) {
-  DisassemblerSP disassembler_sp(create_callback(arch, flavor));
-
-  if (disassembler_sp)
-return disassembler_sp;
+  if (auto disasm_sp = create_callback(arch, flavor))
+return disasm_sp;
 }
   } else {
 for (uint32_t idx = 0;
  (create_callback = 
PluginManager::GetDisassemblerCreateCallbackAtIndex(
   idx)) != nullptr;
  ++idx) {
-  DisassemblerSP disassembler_sp(create_callback(arch, flavor));
-
-  if (disassembler_sp)
-return disassembler_sp;
+  if (auto disasm_sp = create_callback(arch, flavor))
+return disasm_sp;
 }
   }
   return DisassemblerSP();
Index: lldb/include/lldb/lldb-private-interfaces.h
===
--- lldb/include/lldb/lldb-private-interfaces.h
+++ lldb/include/lldb/lldb-private-interfaces.h
@@ -30,8 +30,8 @@
  const ArchSpec &arch);
 typedef std::unique_ptr (*ArchitectureCreateInstance)(
 const ArchSpec &arch);
-typedef Disassembler *(*DisassemblerCreateInstance)(const ArchSpec &arch,
-const char *flavor);
+typedef lldb::DisassemblerSP (*DisassemblerCreateInstance)(const ArchSpec 
&arch,
+   const char *flavor);
 typedef DynamicLoader *(*DynamicLoaderCreateInstance)(Process *process,
   bool force);
 typedef lldb::JITLoaderSP (*JITLoaderCreateInstance)(Process *process,


Index: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
===
--- lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
+++ lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.h
@@ -34,8 +34,8 @@
 
   static llvm::StringRef GetPluginNameStatic() { return "llvm-mc"; }
 
-  static lldb_private::Disassembler *
-  CreateInstance(const lldb_private::ArchSpec &arch,

[Lldb-commits] [PATCH] D150236: Thread::GetStackFrameCount should forestall interruption

[Jim Ingham via Phabricator via lldb-commits]

jingham created this revision.
jingham added reviewers: JDevlieghere, mib, bulbazord.
Herald added a project: All.
jingham requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

We can't let GetStackFrameCount get interrupted or it will give the wrong 
answer.  Plus, it's useful in some places to have a way to force the full stack 
to be created even in the face of interruption.  Moreover, most of the time 
when you're just getting frames, you don't need to know the number of frames in 
the stack to start with.  You just keep calling 
`Thread::GetStackFrameAtIndex(index++)` and when you get a null StackFrameSP 
back, you're done.  That's also more amenable to interruption if you are doing 
some work frame by frame.

So this patch makes GetStackFrameCount always return  the full count, 
suspending interruption.  I  also went through all the places that use 
GetStackFrameCount to make sure that they really needed the full stack walk.  
In many cases, they did not.  For instance `frame select -r 10` was getting the 
number of frames just to check whether `cur_frame_idx + 10` was within the 
stack.  It's better in that case to see if that frame exists first, since that 
doesn't force a full stack walk, and only deal with walking off the end of the 
stack if it doesn't...

I also added a test for some of these behaviors.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150236

Files:
  lldb/include/lldb/Target/StackFrameList.h
  lldb/source/Commands/CommandCompletions.cpp
  lldb/source/Plugins/SystemRuntime/MacOSX/SystemRuntimeMacOSX.cpp
  lldb/source/Target/StackFrameList.cpp
  lldb/test/API/functionalities/bt-interrupt/Makefile
  lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
  lldb/test/API/functionalities/bt-interrupt/main.c

Index: lldb/test/API/functionalities/bt-interrupt/main.c
===
--- /dev/null
+++ lldb/test/API/functionalities/bt-interrupt/main.c
@@ -0,0 +1,23 @@
+#include 
+
+// This example is meant to recurse infinitely.
+// The extra struct is just to make the frame dump
+// more complicated.
+
+struct Foo {
+  int a;
+  int b;
+  char *c;
+};
+
+int
+forgot_termination(int input, struct Foo my_foo) {
+  return forgot_termination(++input, my_foo);
+}
+
+int
+main()
+{
+  struct Foo myFoo = {100, 300, "A string you will print a lot"}; // Set a breakpoint here
+  return forgot_termination(1, myFoo);
+}
Index: lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
===
--- /dev/null
+++ lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py
@@ -0,0 +1,48 @@
+"""
+Ensure that when the interrupt is raised we still make frame 0.
+and make sure "GetNumFrames" isn't interrupted.
+"""
+
+import lldb
+import lldbsuite.test.lldbutil as lldbutil
+from lldbsuite.test.lldbtest import *
+
+
+class TestInterruptingBacktrace(TestBase):
+
+NO_DEBUG_INFO_TESTCASE = True
+
+def test_backtrace_interrupt(self):
+"""There can be many tests in a test case - describe this test here."""
+self.build()
+self.main_source_file = lldb.SBFileSpec("main.c")
+self.bt_interrupt_test()
+
+def bt_interrupt_test(self):
+(target, process, thread, bkpt) = lldbutil.run_to_source_breakpoint(self,
+   "Set a breakpoint here", self.main_source_file)
+
+# Now continue, and when we stop we will have crashed.
+process.Continue()
+self.dbg.RequestInterrupt()
+
+# Be sure to turn this off again:
+def cleanup ():
+if self.dbg.InterruptRequested():
+self.dbg.CancelInterruptRequest()
+self.addTeardownHook(cleanup)
+
+frame_0 = thread.GetFrameAtIndex(0)
+self.assertTrue(frame_0.IsValid(), "Got a good 0th frame")
+# The interrupt flag is up already, so any attempt to backtrace
+# should be cut short:
+frame_1 = thread.GetFrameAtIndex(1)
+self.assertFalse(frame_1.IsValid(), "Prevented from getting more frames")
+# Since GetNumFrames is a contract, we don't interrupt it:
+num_frames = thread.GetNumFrames()
+print(f"Number of frames: {num_frames}")
+self.assertGreater(num_frames, 1, "Got many frames")
+
+self.dbg.CancelInterruptRequest()
+
+
Index: lldb/test/API/functionalities/bt-interrupt/Makefile
===
--- /dev/null
+++ lldb/test/API/functionalities/bt-interrupt/Makefile
@@ -0,0 +1,4 @@
+C_SOURCES := main.c
+CFLAGS_EXTRAS := -std=c99
+
+include Makefile.rules
Index: lldb/source/Target/StackFrameList.cpp
===
--- lldb/source/Target/StackFrameList.cpp
+++ lldb/source/Target/StackFrameList.cpp
@@ -86,7 +86,7 @@
 
   std::lock

[Lldb-commits] [PATCH] D149914: [lldb] Refactor ObjCLanguage::MethodName

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D149914#4331048 , @jingham wrote:

> Most of this is fine.  I wonder about avoiding caching the full name and name 
> w/o category & selector name.  One of the main uses of this class is to take 
> incoming ObjC names from the ConstString pool, chop them up into full name, 
> name w/o category, and selectorName, which we then insert into the lookup 
> names tables for the modules they are found in.  All those lookup table names 
> will also exist in the ConstString pool.

The full name being a ConstString might make sense since we're usually getting 
them from debug info or similar sources. When we chop them up, we can decide if 
it's worth making ConstStrings out of them. At least that's the idea.
That being said, I'll look in more detail about how often we actually try to 
create `ObjCLanguage::MethodName` objects. If we do it in a hot loop then maybe 
ConstString is the right thing to do.

> The other instance where we get these names is when someone passes them to 
> break set.  In that case, we're going to break the name apart and then look 
> it and its parts up in the ConstString lookup tables to find a match.
>
> So it seems like this is a case where you aren't really getting savings in 
> the ConstString pool, you're just causing more copying from std::string to 
> ConstString.

What's the code path for this? I'm not 100% convinced we should be 
automatically storing portions of user input in the ConstString StringPool 
until we have a little more information about what they gave us.




Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:37
+///
+/// \return If the name did parse as a valid Objective-C method name,
+/// returns std::nullopt. Otherwise returns a const MethodName.

jingham wrote:
> 
Good catch, thanks!



Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:100
+///   will give you "my_additions"
+/// \return A StringRef to the category name of this method.
+llvm::StringRef GetCategory() const;

jingham wrote:
> What does this return if there's no category?
> 
> We have two behaviors above:
> 
> GetFullNameWithoutCategory - returns an empty string if the original class 
> had no category
> GetClassNameWithCategory - returns the original class name if it had no 
> category
> GetCategory - returns ??? if it had no category.
> 
> These seem a little skew somehow...
`GetCategory` will return an empty StringRef if there is no category. I will 
explicitly document that.

I agree that there's some skew here though. The way I think about it is 
`GetCategory` and `GetClassNameWithCategory` do what they say they will do. If 
there's no category, both of these will leave out the category. The former will 
therefore be empty and the latter will give you what you want without the 
category. Maybe it could be empty though? I haven't thought about it.





Comment at: lldb/source/Plugins/Language/ObjC/ObjCLanguage.h:117
 
-  protected:
-ConstString
-m_full; // Full name:   "+[NSString(my_additions) 
myStringWithCString:]"
-ConstString m_class; // Class name:  "NSString"
-ConstString
-m_class_category;   // Class with category: "NSString(my_additions)"
-ConstString m_category; // Category:"my_additions"
-ConstString m_selector; // Selector:"myStringWithCString:"
-Type m_type = eTypeUnspecified;
-bool m_category_is_valid = false;
+const std::string m_full;
+Type m_type;

jingham wrote:
> For the most part, we find ObjC method names either from debug info, from the 
> symbol table or from the runtime symbol vendor.  All of those are going to 
> store their names in the ConstString pool.
> 
> If we only make these MethodName entities one by one then we're just doing a 
> string copy every so often, and that shouldn't be noticeable.  But if we make 
> and hold a lot of these, we're doubling our storage requirements for a thing 
> that at least on Darwin there are a lot of...
> 
> This one seems a reasonable candidate to be a ConstString... 
Oh, I suppose that's true. I was looking at the call-sites and a lot of them 
were passing in `const char *`, but those likely come from `ConstString`s at 
some layer. In that case, making `m_full` be a ConstString is probably fine.

BTW, from what I can tell, we only do make these `ObjCLanguage::MethodName` 
entities one-by-one. They are never really stored anywhere, they're mostly used 
to chop up things we think may be ObjCLanguage method names and get the parts 
out we find useful. Aside from the input, I wanted to create an interface where 
the caller can decide if the output of its functions should be stored in a 
ConstString or not instead of preemptively putting it in the StringPool.


Repository:
  rG LLVM Github Monorepo

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

ht

[Lldb-commits] [PATCH] D150239: ObjectFileMachO: Prioritize the TEXT segment as the mach header segment, regardless of the order the segments appear in the file

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda created this revision.
jasonmolenda added a reviewer: jingham.
jasonmolenda added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a project: All.
jasonmolenda requested review of this revision.
Herald added a subscriber: lldb-commits.

`ObjectFileMachO::GetMachHeaderSection()` is used to find the vmaddr of the 
mach-o header of a binary.  It first scans all of the Mach-O segments looking 
for one at file offset 0.  Then it finds no segments at file offset 0 (e.g. 
binaries in a shared cache), it looks for a segment called __TEXT.

I have an unusual situation where I need to work with a binary where it is laid 
out in virtual memory the traditional order, TEXT, DATA, LINKEDIT.  But the 
Mach-O binary is ordered DATA, LINKEDIT, TEXT.  I would like to argue that 
while technically allowed, no one does this, but there is no part of the Mach-O 
standard that forbids such a binary, and lldb needs to roll with it.

I'm moving the search for a segment called "TEXT" to be the first thing we try, 
and failing that, look for a segment with file offset 0.  I don't have any such 
binary I can include as a test case publicly, so I can't add a test for this 
specific case right now.

Another possible approach would be to sort the segments by vmaddr order and 
take the one with the lowest vmaddr as being the mach-o header.  It would be a 
valid approach too, but I haven't stress tested this idea and am reluctant to 
try something novel when the existing shared cache style search works correctly 
if we prioritize it ahead of fileoff==0 search.

rdar://109128418


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D150239

Files:
  lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6059,6 +6059,16 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return nullptr;
+
+  // Some binaries can have a TEXT segment with a non-zero file offset.
+  // Binaries in the shared cache are one example.  Some hand-generated
+  // binaries may not be laid out in the normal TEXT,DATA,LC_SYMTAB order
+  // in the file, even though they're laid out correctly in vmaddr terms.
+  SectionSP text_segment_sp =
+  section_list->FindSectionByName(GetSegmentNameTEXT());
+  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
+return text_segment_sp.get();
+
   const size_t num_sections = section_list->GetSize();
   for (size_t sect_idx = 0; sect_idx < num_sections; ++sect_idx) {
 Section *section = section_list->GetSectionAtIndex(sect_idx).get();
@@ -6066,14 +6076,6 @@
   return section;
   }
 
-  // We may have a binary in the shared cache that has a non-zero
-  // file address for its first segment, traditionally the __TEXT segment.
-  // Search for it by name and return it as our next best guess.
-  SectionSP text_segment_sp =
-  GetSectionList()->FindSectionByName(GetSegmentNameTEXT());
-  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
-return text_segment_sp.get();
-
   return nullptr;
 }
 


Index: lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
===
--- lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
+++ lldb/source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp
@@ -6059,6 +6059,16 @@
   SectionList *section_list = GetSectionList();
   if (!section_list)
 return nullptr;
+
+  // Some binaries can have a TEXT segment with a non-zero file offset.
+  // Binaries in the shared cache are one example.  Some hand-generated
+  // binaries may not be laid out in the normal TEXT,DATA,LC_SYMTAB order
+  // in the file, even though they're laid out correctly in vmaddr terms.
+  SectionSP text_segment_sp =
+  section_list->FindSectionByName(GetSegmentNameTEXT());
+  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
+return text_segment_sp.get();
+
   const size_t num_sections = section_list->GetSize();
   for (size_t sect_idx = 0; sect_idx < num_sections; ++sect_idx) {
 Section *section = section_list->GetSectionAtIndex(sect_idx).get();
@@ -6066,14 +6076,6 @@
   return section;
   }
 
-  // We may have a binary in the shared cache that has a non-zero
-  // file address for its first segment, traditionally the __TEXT segment.
-  // Search for it by name and return it as our next best guess.
-  SectionSP text_segment_sp =
-  GetSectionList()->FindSectionByName(GetSegmentNameTEXT());
-  if (text_segment_sp.get() && SectionIsLoadable(text_segment_sp.get()))
-return text_segment_sp.get();
-
   return nullptr;
 }
 
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.

[Lldb-commits] [PATCH] D150236: Thread::GetStackFrameCount should forestall interruption

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.



Comment at: lldb/include/lldb/Target/StackFrameList.h:103-104
 
-  void GetFramesUpTo(uint32_t end_idx);
+  /// Gets frames up to end_idx (which can be greater than the actual number of
+  /// frames.)  Returns true if the function was interrupted, false otherwise.
+  bool GetFramesUpTo(uint32_t end_idx, bool allow_interrupt = true);

Is the range inclusive of `end_idx`? as in, is it [0, end_idx) or [0, end_idx]?



Comment at: lldb/source/Target/StackFrameList.cpp:89
 
-  GetFramesUpTo(0);
+  GetFramesUpTo(0, false);
   if (m_frames.empty())

nit:
```
GetFramesUpTo(/*end_idx = */0, /*allow_interrupt = */false);
```

Or something like this... A little easier to understand IMO.



Comment at: lldb/source/Target/StackFrameList.cpp:640-641
   if (can_create)
-GetFramesUpTo(UINT32_MAX);
+GetFramesUpTo(UINT32_MAX, false); // Don't allow interrupt or we might not
+  // return the correct count
 

Same here I think.



Comment at: lldb/source/Target/StackFrameList.cpp:683-684
   // there are that many.  If there weren't then you asked for too many frames.
-  GetFramesUpTo(idx);
+  bool interrupted = GetFramesUpTo(idx);
+  if (interrupted) {
+Log *log = GetLog(LLDBLog::Thread);

nit: Since `interrupted` isn't used anywhere else, you could do something like:
```
if (bool interrupted = GetFramesUpTo(idx)) {
 // Code here
}
```
You could also just do `if (GetFramesUpTo(idx))` but I think the name of the 
function isn't descriptive enough to do that and stay readable.



Comment at: 
lldb/test/API/functionalities/bt-interrupt/TestInterruptBacktrace.py:16
+def test_backtrace_interrupt(self):
+"""There can be many tests in a test case - describe this test here."""
+self.build()

I have a feeling this docstring was supposed to be different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150236

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