[Lldb-commits] [PATCH] D95992: Print the "no plugin" warning only when there is no plugin

shafik added a comment. Can we test this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95992/new/ https://reviews.llvm.org/D95992 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-

[Lldb-commits] [PATCH] D96366: Remove uneeded CopyType from BlockPointerSyntheticFrontEnd

shafik created this revision. shafik added reviewers: teemperor, aprantl. shafik requested review of this revision. `BlockPointerSyntheticFrontEnd` does a `CopyType` which results in it copying the type back into its own context. This will result in a call to `ASTImporterDelegate::setOrigin` wit

[Lldb-commits] [PATCH] D96366: Remove uneeded CopyType from BlockPointerSyntheticFrontEnd

This revision was automatically updated to reflect the committed changes. Closed by commit rG4f14c17df709: [LLDB] Remove uneeded CopyType from BlockPointerSyntheticFrontEnd (authored by shafik). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik added a comment. I am open to different approaches to this problem, this is opening attempt at fixing it but I may be missing other interactions. AFAICT starting the definition of `ToRecord` the way `CompleteDecl(…)` does when we know `FromRecord` is not defined is just broken for the

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik created this revision. shafik added reviewers: martong, balazske, teemperor, a_sidorin. Herald added a subscriber: rnkovacs. Herald added a reviewer: a.sidorin. shafik added a comment. I am open to different approaches to this problem, this is opening attempt at fixing it but I may be miss

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:50 + StringPrinterBufferPointer(StringPrinterBufferPointer &&rhs) + : m_data(rhs.m_data), m_size(rhs.m_size), m_deleter(rhs.m_deleter) { +rhs.m_data = nullptr; `m_del

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik added inline comments. Comment at: lldb/unittests/DataFormatter/StringPrinterTests.cpp:109 + EXPECT_TRUE(matches({"\0", 1}, QUOTE(""))); + EXPECT_TRUE(matches("\a", QUOTE("\\a"))); + EXPECT_TRUE(matches("\b", QUOTE("\\u{8}"))); We have [raw string lite

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 257552. shafik marked 4 inline comments as done. shafik added a comment. Addressing comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 Files: clang/lib/AST/ASTImporter.cpp lldb/test/API/commands/ex

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 258084. shafik added a comment. - Added unit test - Modified condition for defining `FromRecord` to whether it has an `ExternalSource` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 Files: clang/lib/AST/

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/AST/ASTImporter.cpp:8173 + // If a RecordDecl has base classes they won't be imported and we will + // be missing anything that we inherit from those bases. + if (FromRecor

[Lldb-commits] [PATCH] D78329: Allow lldb-test to combine -find with -dump-clang-ast

shafik added inline comments. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:8823 case clang::Type::Typedef: { const clang::TypedefType *typedef_type = Does it make sense to apply these change to non-objective-C cases? ==

[Lldb-commits] [PATCH] D78329: Allow lldb-test to combine -find with -dump-clang-ast

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Thank you! This looks like a nice update to the functionality. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78329/new/ https://reviews.llvm.org/D78329 __

[Lldb-commits] [PATCH] D78396: [lldb/Dataformatter] Add support for CoreFoundation Dictionaries and Sets

shafik added inline comments. Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:8 + +CFBasicHash::CFBasicHash() +: m_ptr_size(UINT32_MAX), m_byte_order(eByteOrderInvalid), Use in class member initializers please then you can use `=default` for the

[Lldb-commits] [PATCH] D78396: [lldb/Dataformatter] Add support for CoreFoundation Dictionaries and Sets

shafik added inline comments. Comment at: lldb/source/Plugins/Language/ObjC/CFBasicHash.cpp:49 + size_t size = 0; + size += sizeof(m_ht->base); + size += sizeof(m_ht->bits); Shouldn't we check that `m_ht` is actually managing an object before attempting to ac

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 259110. shafik marked 2 inline comments as done. shafik added a comment. - Simplifying the changes in `ASTImporter::ImportContext` - Removing `DC->setHasExternalLexicalStorage(true);` from unit test since we are checking `getExternalSource()` CHANGES SINCE L

[Lldb-commits] [PATCH] D78712: [lldb/Host] Improve error messages on unowned read files

shafik added inline comments. Comment at: lldb/source/Host/common/FileSystem.cpp:504 + FileSpec file(file_path.getSingleStringRef()); + return Readable(file_path) && Instance().Open(file, File::eOpenOptionRead); +} Since `file` is a `FileSpec` can't we just use

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

shafik updated this revision to Diff 259703. shafik marked 2 inline comments as done. shafik added a comment. Capitalization fixes. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78000/new/ https://reviews.llvm.org/D78000 Files: clang/lib/AST/ASTImporter.cpp clang/unittests/AST/ASTI

[Lldb-commits] [PATCH] D78000: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...)

This revision was automatically updated to reflect the committed changes. Closed by commit rGdef7c7f60205: [ASTImporter] Fix handling of not defined FromRecord in ImportContext(...) (authored by shafik). Herald added projects: clang, LLDB. Changed prior to commit: https://reviews.llvm.org/D7800

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik added inline comments. Comment at: lldb/source/DataFormatters/StringPrinter.cpp:175 + constexpr unsigned max_buffer_size = 7; + uint8_t *data = new uint8_t[max_buffer_size]; + switch (escape_style) { I really wish we could get ride of the naked `new`. I

[Lldb-commits] [PATCH] D79251: Fix overloaded operator new cases in TestCppOperators.py which currently work by accident

shafik created this revision. shafik added reviewers: teemperor, aprantl, labath. The overloaded `new operator` in `TestCppOperators.py` are working by accident. Currently we have: void *operator new(std::size_t size) { C* r = ::new C; r->custom_new = true; return r; } void *operator new[](

[Lldb-commits] [PATCH] D77843: [lldb/DataFormatters] Delete GetStringPrinterEscapingHelper

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM with minor comments, this is a big set of changes, I would prefer if one of the other reviewers also gave it a second look. Comment at: lldb/source/DataFormatters/Strin

[Lldb-commits] [PATCH] D79251: Fix overloaded operator new cases in TestCppOperators.py which currently work by accident

shafik updated this revision to Diff 261920. shafik marked an inline comment as done. shafik added a comment. Removing `custom_new` since it is now unused. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79251/new/ https://reviews.llvm.org/D79251 Files: lldb/test/API/lang/cpp/operators

[Lldb-commits] [PATCH] D79251: Fix overloaded operator new cases in TestCppOperators.py which currently work by accident

This revision was automatically updated to reflect the committed changes. Closed by commit rG4ad53177db75: [LLDB] Fix overloaded operator new cases in TestCppOperators.py which currently… (authored by shafik). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D79554: [lldb/Dataformatter] Add support to CF{Dictionary, Set}Ref types

shafik added a comment. I have to agree w/ Pavel here that I am not crazy about creating creating an empty struct here, it is not clear why there is no other way. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79554/new/ https://reviews.llvm.org/D7

[Lldb-commits] [PATCH] D79563: [lldb/test] Make "inline" tests handle multiple statements at the same location

shafik added a comment. Thank you! I am pleasantly surprised there was only one hidden broken test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79563/new/ https://reviews.llvm.org/D79563 ___ lldb-com

[Lldb-commits] [PATCH] D79645: [lldb/test] Fix for flakiness in TestNSDictionarySynthetic

shafik added inline comments. Comment at: lldb/test/API/lldbtest.py:123 # In Python 2, string objects can contain Unicode characters. -out = out.decode('utf-8') -err = err.decode('utf-8') +out = out.decode('utf-8', 'replace') +

[Lldb-commits] [PATCH] D79811: WIP: Reenable creation of artificial methods in AddMethodToCXXRecordType(...)

shafik added a comment. I labeled this as WIP in progress since I expect some discussion and maybe a test case for which the change fails. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79811/new/ https://reviews.llvm.org/D79811 ___ lldb-co

[Lldb-commits] [PATCH] D79811: WIP: Reenable creation of artificial methods in AddMethodToCXXRecordType(...)

shafik created this revision. shafik added reviewers: clayborg, jingham, aprantl, teemperor. Herald added subscribers: kbarton, nemanjai. shafik planned changes to this revision. Herald added a subscriber: wuzish. shafik requested review of this revision. shafik retitled this revision from "Reenabl

[Lldb-commits] [PATCH] D79811: WIP: Reenable creation of artificial methods in AddMethodToCXXRecordType(...)

shafik marked an inline comment as done. shafik added a comment. In D79811#2033399 , @labath wrote: > In general, I think this is a good direction to go in. We can run into the > same kinds of problems even with non-artificial functions -- either the > a

[Lldb-commits] [PATCH] D79811: WIP: Reenable creation of artificial methods in AddMethodToCXXRecordType(...)

shafik added a comment. In D79811#2034842 , @clayborg wrote: > The error where two of the same classes conflicted usually only happened in > complex cases that were hard to reduce down. > > If I understand this correctly, the compiler should be able to a

[Lldb-commits] [PATCH] D79789: [lldb] Don't dissasemble large functions by default

shafik added a comment. Pavel, I think this broke the green dragon bots: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/18207/ Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79789/new/ https://reviews.llvm.org/D79789 ___

[Lldb-commits] [PATCH] D79789: [lldb] Don't dissasemble large functions by default

shafik added a comment. I reverted this it breaks `TestFoundationDisassembly.py`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79789/new/ https://reviews.llvm.org/D79789 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

shafik added inline comments. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5215 +uint32_t num_pointee_children = 0; +if (pointee_clang_type.IsAggregateType()) + num_pointee_children = I am curious what cases are pointers aggre

[Lldb-commits] [PATCH] D80254: Prevent GetNumChildren from transitively walking pointer chains

shafik added inline comments. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:5215 +uint32_t num_pointee_children = 0; +if (pointee_clang_type.IsAggregateType()) + num_pointee_children = jarin wrote: > shafik wrote: > > I am curi

[Lldb-commits] [PATCH] D80308: [lldb] Enable C++14 when evaluating expressions in a C++14 frame

shafik added inline comments. Comment at: lldb/test/API/lang/cpp/standards/cpp14/TestCPP14Standard.py:19 +# polymorphic lambdas). +self.expect_expr("[](auto x) { return x; }(1)", result_type="int", result_value="1") It would be worth it to add a

[Lldb-commits] [PATCH] D80308: [lldb] Enable C++14 when evaluating expressions in a C++14 frame

shafik added a comment. In D80308#2051624 , @aprantl wrote: > > It seems C++17 and so on isn't yet in any of the language enums (and the > > DWARF standard it seems), so C++17 support will be a follow up patch. > > Yes and that may point to a problem with

[Lldb-commits] [PATCH] D80308: [lldb] Enable C++14 when evaluating expressions in a C++14 frame

shafik added inline comments. Comment at: lldb/test/API/lang/cpp/standards/cpp14/TestCPP14Standard.py:19 +# polymorphic lambdas). +self.expect_expr("[](auto x) { return x; }(1)", result_type="int", result_value="1") teemperor wrote: > shafik wrot

[Lldb-commits] [PATCH] D79752: [lldb] Move ClangModulesDeclVendor ownership to ClangPersistentVariables

shafik added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:854 - ClangModulesDeclVendor *modules_decl_vendor = - m_target->GetClangModulesDeclVendor(); + auto *persistent_vars = llvm::cast( + m_target->GetPersistentExpres

[Lldb-commits] [PATCH] D80955: Fix UB in EmulateInstructionARM64.cpp

shafik added inline comments. Comment at: lldb/source/Plugins/Instruction/ARM64/EmulateInstructionARM64.cpp:25 +#include + Why? also should be `cstdlib` in C++. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D80955/new/ https://reviews.llvm.org/D80955

[Lldb-commits] [PATCH] D80793: [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

shafik added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h:33-40 +/// Wrapped in a member function of a C++ class. +CppMemberFunction, +/// Wrapped in a instance Objective-C method. +ObjCInstanceMethod, +/

[Lldb-commits] [PATCH] D80793: [lldb][NFC] Make ClangExpressionSourceCode's wrapping logic more consistent

shafik added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionSourceCode.h:41 +/// Note that this is also used for static member functions of a C++ class. +Function + }; I would actually feel better have a separate

[Lldb-commits] [PATCH] D81119: [lldb] Fix SLEB128 decoding

shafik added a comment. I would also like to see a unit test, unless the DWARF test somehow covers as aspect that a unit test would and even in that case the unit test is still important. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81119/new/ h

[Lldb-commits] [PATCH] D82382: [lldb][NFC] Replace most uses of StringConvert in LLDB with LLVM's to_integer

shafik added inline comments. Comment at: lldb/source/Interpreter/OptionValueArray.cpp:209 for (i = 0; i < argc; ++i) { -const size_t idx = -StringConvert::ToSInt32(args.GetArgumentAtIndex(i), INT32_MAX); -if (idx >= size) { +size_t idx;

[Lldb-commits] [PATCH] D82559: [LLDB][Clang Integration][NFC] Remove redundant condition

shafik accepted this revision. shafik 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/D82559/new/ https://reviews.llvm.org/D82559 __

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik created this revision. shafik added reviewers: teemperor, jingham, jasonmolenda, friss. Herald added a subscriber: kristof.beyls. shafik marked an inline comment as done. shafik added a subscriber: rsmith. shafik added inline comments. Comment at: clang/lib/AST/RecordLayou

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik marked an inline comment as done. shafik added a subscriber: rsmith. shafik added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1190 if (UseExternalLayout) { -// FIXME: This appears to be reversed. if (Base->IsVirtual) @rsm

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

shafik added inline comments. Comment at: lldb/include/lldb/Utility/DataExtractor.h:24 +#ifndef __builtin_is_aligned +#define __builtin_is_aligned(POINTER, BYTE_COUNT) \ What platforms are we not expecting to have `__builtin_is_ali

[Lldb-commits] [PATCH] D83256: [lldb] Assert on unaligned load in DataExtractor

shafik added inline comments. Comment at: lldb/include/lldb/Utility/DataExtractor.h:24 +#ifndef __builtin_is_aligned +#define __builtin_is_aligned(POINTER, BYTE_COUNT) \ shafik wrote: > What platforms are we not expecting to have `

[Lldb-commits] [PATCH] D83199: [lldb/DWARF] Add a utility function for (forceful) completion of types

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1339 + TypeSystemClang::CompleteTagDeclarationDefinition(type); + const auto *td = TypeSystemC

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik updated this revision to Diff 275865. shafik added a comment. Adding a second test that is not arm64 specific. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83008/new/ https://reviews.llvm.org/D83008 Files: clang/lib/AST/RecordLayoutBuilder.cpp lldb/test/Shell/Expr/Inputs/la

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik added a comment. In D83008#2131776 , @teemperor wrote: > Thanks for tracking this down, this is a really nasty bug... > > The fix itself is obviously fine, but I think I'm out of the loop regarding > the testing strategy. We talked about adding a C

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik added a comment. In D83008#2136303 , @teemperor wrote: > I think we are talking about different things. My question was why is this a > 'Shell' test (like, a test in the `Shell` directory that uses FileCheck) and > not a test in the `API` director

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik updated this revision to Diff 276229. shafik marked 5 inline comments as done. shafik added a comment. - Removing spurious local variables in test - Simplifying the test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83008/new/ https://reviews.llvm.org/D83008 Files: clang/lib/A

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik added inline comments. Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2197 // least as many of them as possible). return RD->isTrivial() && RD->isCXX11StandardLayout(); } teemperor wrote: > See here for the POD check that we might get wrong

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

shafik updated this revision to Diff 276407. shafik added a comment. Moved from shell test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83008/new/ https://reviews.llvm.org/D83008 Files: clang/lib/AST/RecordLayoutBuilder.cpp lldb/test/API/lang/cpp/alignas_base_class/Makefile lldb

[Lldb-commits] [PATCH] D83008: Fix ItaniumRecordLayoutBuilder so that is grabs the correct bases class offsets from the external source

This revision was automatically updated to reflect the committed changes. Closed by commit rG63b0f8c788d8: [RecordLayout] Fix ItaniumRecordLayoutBuilder so that is grabs the correct… (authored by shafik). Herald added projects: clang, LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAS

[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

shafik created this revision. shafik added reviewers: teemperor, jingham, aprantl. Currently expressions dealing with bit-fields in Objective-C objects is pretty broken. When generating debug-info for Objective-C bit-fields we don't generate offsets because the Objective-C runtime is supposed to

[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

shafik marked an inline comment as done. shafik added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2612 -if ((this_field_info.bit_offset >= parent_bit_size) || -(last_field_info.IsBitfield() && -

[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

shafik added a comment. We can never know the offsets statically b/c of how Objective-C deals with the fragile base class problem the runtime may have to shift fields over and we can not calculate this statically.

[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

shafik added a comment. @aprantl I think this Objective-C Runtime Programming Guide: Bye Encodings entry and this sample program answer the rest of your questions: #in

[Lldb-commits] [PATCH] D83787: [lldb/Test] Always set the cleanupSubprocesses tear down hook

shafik added a comment. Curious, why didn't we do it this way before? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83787/new/ https://reviews.llvm.org/D83787 ___ lldb-commits mailing list lldb-commits@lists.llvm.o

[Lldb-commits] [PATCH] D83796: [ObjC] Wrap namespace-global structure in an anonymous namespace to avoid ODR violations

shafik added a comment. It does indeed solve the immediate problem but if those namespaces aren't really meant to be shared across translation units they should have different names but that is for another PR. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

shafik updated this revision to Diff 278026. shafik marked 6 inline comments as done. shafik added a comment. Added clarifying comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83433/new/ https://reviews.llvm.org/D83433 Files: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserC

[Lldb-commits] [PATCH] D83840: [lldb][test] Prevent infinite loop while looking for use_lldb_suite_root.py.

shafik added a comment. @labath why do we need two copies of `use_lldb_suite.py`? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83840/new/ https://reviews.llvm.org/D83840 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D83972: Modify ImportDefiniton for ObjCInterfaceDecl so that we always the ImportDeclContext one we start the definition

shafik created this revision. shafik added reviewers: teemperor, martong. Herald added a subscriber: rnkovacs. Once we start the definition of an `ObjCInterfaceDecl` we won't attempt to `ImportDeclContext` later on. Unlike `RecordDecl` case which uses `DefinitionCompleter` to force `completeDefi

[Lldb-commits] [PATCH] D84015: [lldb] Remove orphaned modules in a loop

shafik added a comment. Thank you for this fix! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84015/new/ https://reviews.llvm.org/D84015 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https:

[Lldb-commits] [PATCH] D83433: Fix how we handle bit-fields for Objective-C when creating an AST

This revision was automatically updated to reflect the committed changes. Closed by commit rGa54c42df9a72: Fix how we handle bit-fields for Objective-C when creating an AST (authored by shafik). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D83433?vs=278026&id=

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

shafik added subscribers: clayborg, shafik. shafik added a comment. In D70846#1763802 , @teemperor wrote: > This LGTM, but the TODO directly above the change in > ClangExpressionDeclMap.cpp worries me a bit. I am not sure how good our test > coverage is

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

shafik added a comment. In D70846#1763731 , @labath wrote: > There's `lldb-shell :: SymbolFile/DWARF/find-basic-function.cpp`, which > probably didn't get run for you as you didn't have lld enabled (cmake > -DLLVM_ENABLE_PROJECTS=...;lld). You'll need to

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

shafik added a comment. In D70846#1766598 , @labath wrote: > In D70846#1766204 , @shafik wrote: > > > In D70846#1763731 , @labath wrote: > > > > > There's `lldb-shell :: Symb

[Lldb-commits] [PATCH] D70721: [lldb/cpluspluslanguage] Add constructor substitutor

shafik added inline comments. Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:327 + bool trySubstitute(llvm::StringRef From, llvm::StringRef To) { +if (!llvm::StringRef(this->First, this->numLeft()).startswith(From)) `trySubstitute

[Lldb-commits] [PATCH] D70663: [lldb] Remove lldb's own ASTDumper

shafik added a comment. Yeah, there was indeed some ugly code there, nice work! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70663/new/ https://reviews.llvm.org/D70663 ___ lldb-commits mailing list ll

[Lldb-commits] [PATCH] D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit

shafik created this revision. shafik added reviewers: aprantl, teemperor, JDevlieghere. `GetMaxU64Bitfield(...)` uses the `ul` suffix but we require a 64 bit unsigned integer and `ul` could be 32 bit. So this replacing it with a explicit cast and refactors the code around it to use an early exit

[Lldb-commits] [PATCH] D70721: [lldb/cpluspluslanguage] Add constructor substitutor

shafik accepted this revision. shafik added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:352 + void appendUnchangedInput() { +Result += llvm::StringRef(Written, this->First - Writte

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

shafik added a comment. In D70846#1767455 , @labath wrote: > I guess you were building darwin binaries, right? If that's the case, then > you weren't using this code at all, as apple uses AppleIndex by default. The > test in question uses all three index

[Lldb-commits] [PATCH] D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit

shafik updated this revision to Diff 232218. shafik marked 2 inline comments as done. shafik added a comment. Changing `-static_cast(1)` to `std::numeric_limits::max()` CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70992/new/ https://reviews.llvm.org/D70992 Files: lldb/source/Utilit

[Lldb-commits] [PATCH] D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit

shafik added inline comments. Comment at: lldb/source/Utility/DataExtractor.cpp:595 + (bitfield_bit_size == 64 + ? -static_cast(1) + : ((static_cast(1) << bitfield_bit_size) - 1)); teemperor wrote: > How about `std::numeric_limits::max()`

[Lldb-commits] [PATCH] D70992: Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit

This revision was automatically updated to reflect the committed changes. Closed by commit rGfffd70291e12: [LLDB] Replacing use of ul suffix in GetMaxU64Bitfield since it not guarenteed… (authored by shafik). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D70992

[Lldb-commits] [PATCH] D71196: [lldb] Add support for calling objc_direct methods from LLDB's expression evaluator.

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71196/new/ https://reviews.llvm.org/D71196 ___ lldb-commits

[Lldb-commits] [PATCH] D71306: [RFC] Change how we deal with optional dependencies

shafik added a comment. In D71306#1779379 , @labath wrote: > Besides lldb developers, another category we should consider are llvm (or > clang or lld, ...) developers who know nothing about lldb, but they ended up > building it because they specified LLV

[Lldb-commits] [PATCH] D71336: [lldb] Remove ClangASTMetrics

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. Herald added a subscriber: rnkovacs. It is not clear to me we could use these in a test because I don't see a reason for the numbers to stay stable and therefore a useful measure. Repository:

[Lldb-commits] [PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

shafik created this revision. shafik added reviewers: martong, teemperor. Herald added a subscriber: rnkovacs. This fix was motivated by a crashes in expression parsing during code generation in which we had a `RecordDecl` that had incomplete `FieldDecl`. During code generation when computing th

[Lldb-commits] [PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

shafik added a comment. In D71378#1781616 , @teemperor wrote: > I wonder if we have a way to fix this from with LLDB. Having Clang code that > is only tested in LLDB is always a bit weird. > > Otherwise the idea itself LGTM, thanks for working on this (an

[Lldb-commits] [PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

shafik updated this revision to Diff 234619. shafik marked 3 inline comments as done. shafik added a comment. - Fix typo - Adjust error handing - clang-format test CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71378/new/ https://reviews.llvm.org/D71378 Files: clang/lib/AST/ASTImporte

[Lldb-commits] [PATCH] D71630: [lldb] Add a SubsystemRAII that takes care of calling Initialize and Terminate in the unit tests

shafik added inline comments. Comment at: lldb/unittests/TestingSupport/SubsystemRAII.h:57 +/// @endcode +template class SubsystemRAII { + /// RAII for a single subsystem. Should we write a test for this? CHANGES SINCE LAST ACTION https://reviews.llvm.org

[Lldb-commits] [PATCH] D71378: Modifying ImportDeclContext(...) to ensure that we complete each FieldDecl of a RecordDecl when we are importing the definiton

This revision was automatically updated to reflect the committed changes. Closed by commit rG6a7df3a3f940: [ASTImporter][LLDB] Modifying ImportDeclContext(...) to ensure that we complete… (authored by shafik). Herald added projects: clang, LLDB. Repository: rG LLVM Github Monorepo CHANGES SINC

[Lldb-commits] [PATCH] D72086: [lldb] Fix that SBThread.GetStopDescription is returning strings with uninitialized memory at the end.

shafik added inline comments. Comment at: lldb/source/API/SBThread.cpp:337 + } else { // NULL dst passed in, return the length needed to contain the The `else` branch feels inconsistent with the change above. Especially the `+ 1`

[Lldb-commits] [PATCH] D71909: [lldb] Fix crash in AccessDeclContextSanity when copying FunctionTemplateDecl inside a record.

shafik added inline comments. Comment at: lldb/source/Symbol/ClangASTContext.cpp:1347 + if (decl_ctx->isRecord()) +func_tmpl_decl->setAccess(clang::AccessSpecifier::AS_public); Where is the method being added and why are we not setting the access there? A

[Lldb-commits] [PATCH] D71909: [lldb] Fix crash in AccessDeclContextSanity when copying FunctionTemplateDecl inside a record.

shafik added inline comments. Comment at: lldb/source/Symbol/ClangASTContext.cpp:1347 + if (decl_ctx->isRecord()) +func_tmpl_decl->setAccess(clang::AccessSpecifier::AS_public); teemperor wrote: > shafik wrote: > > Where is the method being added and why ar

[Lldb-commits] [PATCH] D72161: [lldb][NFC] Use static_cast instead of reinterpret_cast where possible

shafik added a comment. This is a great change! Moving to stricter casts clarifies intent and reduces the chance that later changes can introduce bugs accidentally. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72161/new/ https://reviews.llvm.org/D72161 ___

[Lldb-commits] [PATCH] D72161: [lldb][NFC] Use static_cast instead of reinterpret_cast where possible

shafik added inline comments. Comment at: lldb/tools/debugserver/source/MacOSX/MachProcess.mm:1745 + "0x%8.8llx, length = %llu) => %p", + (uint64_t)addr, (uint64_t)length, static_cast(bp)); return bp; `static_cast(addr

[Lldb-commits] [PATCH] D72190: Removing C-style casts in favor of explict C++ style casts

shafik created this revision. shafik added reviewers: teemperor, JDevlieghere, labath. C-style casts hide intent and when code changes they can also hide logic errors. This change converts most of the C-style cast into `static_cast` and the icky ones are converted to `reinterpret_cast` and we ca

[Lldb-commits] [PATCH] D72413: Add missing nullptr checks.

shafik added a reviewer: shafik. shafik added a comment. I am curious what prompted this set of changes, they look sensible but I don't see how we can do anything useful if we end up in this state. Do we currently have a way to end up in this state? CHANGES SINCE LAST ACTION https://reviews.

[Lldb-commits] [PATCH] D70846: Expression eval lookup speedup by not returning methods in ManualDWARFIndex::GetFunctions

shafik added a comment. In D70846#1809837 , @labath wrote: > This looks fine to me. @shafik, @teemperor, do you have any more comments? Just a minor comment. Comment at: lldb/test/Shell/SymbolFile/DWARF/find-basic-function.cpp:67 + +/

[Lldb-commits] [PATCH] D72359: [lldb] Fix TestClangASTContext.TestFunctionTemplateInRecordConstruction in Debug builds

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72359/new/ https://reviews.llvm.org/D72359 ___ lldb-commits

[Lldb-commits] [PATCH] D72391: [lldb] Add a display name to ClangASTContext instances

shafik added a comment. Perhaps it makes sense to modify the `dump()` method to also display the new name? Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp:609 + m_ast_context.reset(new ClangASTContext( + "Expression AST for '" + m_filena

[Lldb-commits] [PATCH] D72391: [lldb] Add a display name to ClangASTContext instances

shafik added a comment. In D72391#1809751 , @teemperor wrote: > I don't know if this needs a unit test where we call the constructor and > explicitly check the name is the one we passed in. Let me know if you think > this would make sense. I think we s

[Lldb-commits] [PATCH] D72391: [lldb] Add a display name to ClangASTContext instances

shafik added a comment. In D72391#1811589 , @teemperor wrote: > @clayborg We can easily append the ptr value to the display name. All names > should always be unique as long as there is one target, but in the off-chance > that one isn't unique it might b

[Lldb-commits] [PATCH] D72507: [lldb] Remove FieldDecl stealing hack by rerouting indirect imports to the original AST

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72507/new/ https://reviews.llvm.org/D72507 ___ lldb-commits

[Lldb-commits] [PATCH] D72495: [lldb] Make CompleteTagDeclsScope completion order deterministic

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. This is a good change, we do see bugs that are sometimes not totally deterministic so this should hopefully reduce those. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews

<    2   3   4   5   6   7   8   >