[Lldb-commits] [PATCH] D73921: Assert that a subprogram should have a name when parsing DWARF

shafik abandoned this revision. shafik added a comment. I am going to abandon this change b/c the consensus seems to be that we want a different solution and I don't know how much work would require ATM but I may revisit another time, I will note that we do currently have a lot of these and the

[Lldb-commits] [PATCH] D74607: [lldb][NFC] Make all CompilerDeclContext parameters references instead of pointers

shafik accepted this revision. shafik added a comment. This is a great change, it makes the code more consistent. LGTM besides the few comments I made. Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:1133 -if (parent_decl_ctx && GetDeclContextContainingU

[Lldb-commits] [PATCH] D74957: [lldb] Disable auto fix-its when evaluating expressions in the test suite

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM outside of my comments Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:2404 +# Disable fix-its that tests don't pass by accident. +options.Se

[Lldb-commits] [PATCH] D74951: [lldb] Remove all the 'current_id' logging counters from the lookup code.

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. I have struggled to understand the real use of those counters, they may have been helpful to someone at one point but the rationale is lost in the sands of time. LGTM but lets see if someone

[Lldb-commits] [PATCH] D75330: [lldb] Remove checks behind LLDB_CONFIGURATION_DEBUG from TypeSystemClang

shafik added a comment. Nice fix! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75330/new/ https://reviews.llvm.org/D75330 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/ma

[Lldb-commits] [PATCH] D75488: Preserve the owning module information from DWARF in the synthesized AST

shafik added inline comments. Comment at: lldb/include/lldb/Symbol/CompilerType.h:236 // type is valid and the type system supports typedefs, else return an - // invalid type. + // invalid type. The payload argument is the typesystem-specific Type payload. CompilerType C

[Lldb-commits] [PATCH] D75761: Fix to get the AST we generate for function templates to be closer to what clang generates and expects

shafik created this revision. shafik added reviewers: teemperor, aprantl. Fix to get the AST we generate for function templates closer to what clang generates and expects. We fix which `FuntionDecl` we are passing to `CreateFunctionTemplateSpecializationInfo` and we strip template parameters fr

[Lldb-commits] [PATCH] D75761: Fix to get the AST we generate for function templates to be closer to what clang generates and expects

shafik updated this revision to Diff 249247. shafik marked 11 inline comments as done. shafik added a comment. Moving to using `ItaniumPartialDemangler` for now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75761/new/ https://reviews.llvm.org/D75761 Files: lldb/source/Plugins/Symbol

[Lldb-commits] [PATCH] D75761: Fix to get the AST we generate for function templates to be closer to what clang generates and expects

shafik added a comment. In D75761#1912038 , @labath wrote: > It's a pity that the clang's DW_AT_name value is so ambiguous. For example, > gcc would output the name in the commit message as `operator< `, which > is a lot more parsable (for computers and

[Lldb-commits] [PATCH] D75761: Fix to get the AST we generate for function templates to be closer to what clang generates and expects

shafik updated this revision to Diff 249430. shafik added a comment. Move to using `expect_expr` in the test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75761/new/ https://reviews.llvm.org/D75761 Files: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp lldb/test/API/l

[Lldb-commits] [PATCH] D75715: Switch TypeSystemClang over to CreateDeserialized() (NFC)

shafik added inline comments. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:1265 + CXXRecordDecl *decl = CXXRecordDecl::CreateDeserialized(ast, 0); + decl->setTagKind((TagDecl::TagKind)kind); + decl->setDeclContext(decl_ctx); `static_cas

[Lldb-commits] [PATCH] D75715: Switch TypeSystemClang over to CreateDeserialized() (NFC)

shafik added a comment. A bunch of small comments but a few more serious ones as well. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp:1394 + func_tmpl_decl->setDeclName(func_decl->getDeclName()); + func_tmpl_decl->init(func_decl, template_param_list);

[Lldb-commits] [PATCH] D75626: Add support for owning module information to TypeSystemClang.

shafik added inline comments. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:73 + unsigned GetOwningModuleID() { return Flags(m_payload).Clear(ObjCClassBit); } + void SetOwningModuleID(unsigned id) { +assert(id < ObjCClassBit); Why not u

[Lldb-commits] [PATCH] D75626: Add support for owning module information to TypeSystemClang.

shafik added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionDeclMap.cpp:996 m_clang_ast_context->GetUniqueNamespaceDeclaration( - g_lldb_local_vars_namespace_cstr, nullptr); + g_lldb_local_vars_namespace_cstr, null

[Lldb-commits] [PATCH] D75562: Add an opque payload field to lldb::Type (NFC).

shafik added a comment. I want to see how you end up resolving the comments on payload being a plain integer type in D75626 before looking at this again. Maybe it makes more sense to use a type, the use is pretty clever but perhaps makes for opaque code in some

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik added a comment. I am open to suggestions on alternative approaches, for some context I ran into this trying to add a failing test to D75761 as was suggested. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76080/new/ https://reviews.llvm.org/D76

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik created this revision. shafik added reviewers: teemperor, labath, aprantl. shafik added a comment. I am open to suggestions on alternative approaches, for some context I ran into this trying to add a failing test to D75761 as was suggested. I was trying

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik added a comment. In D76080#1919862 , @jingham wrote: > > I'm on the fence about using a "find" not a strict string compare. The only > reason you'd be passing in an error_msg is that you want to test that you got > the error string you were e

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik updated this revision to Diff 250021. shafik added a comment. Incorporate feedback on how to verify the results. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76080/new/ https://reviews.llvm.org/D76080 Files: lldb/packages/Python/lldbsuite/test/lldbtest.py Index: lldb/packag

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik updated this revision to Diff 250083. shafik added a comment. Moving to using `assertIn` as suggest in comment. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76080/new/ https://reviews.llvm.org/D76080 Files: lldb/packages/Python/lldbsuite/test/lldbtest.py Index: lldb/package

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik added a comment. In D76080#1920483 , @teemperor wrote: > - All of the asserts should print a useful error when failing (i.e., one that > allows us to directly write a fix). You could do assertIn which is clearer > than `find(...)` and automaticall

[Lldb-commits] [PATCH] D76080: Adjust error_msg handling for expect_expr in lldbtest.py

shafik abandoned this revision. shafik added a comment. After discussing this @teemperor in some detail it looks like getting the `error_msg` case to work well is not a trivial task. So for these cases we should revert to just using `expect`. I think the plan is that he will remove the feature

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

shafik added a comment. My fix in `ConsumeOperator()` is not proper but if everyone feels this is correct approach I will create member functions to deal with this cleanly. Other approaches could be modifying `ExtractTokens()` to detect this case and generate two `tok::less`in place of `tok::le

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

shafik created this revision. shafik added reviewers: aprantl, labath, jingham. shafik added a comment. My fix in `ConsumeOperator()` is not proper but if everyone feels this is correct approach I will create member functions to deal with this cleanly. Other approaches could be modifying `Extrac

[Lldb-commits] [PATCH] D75761: [lldb] Fix to get the AST we generate for function templates to be closer to what clang generates and expects

shafik updated this revision to Diff 250327. shafik marked 8 inline comments as done. shafik added a comment. Addressing comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75761/new/ https://reviews.llvm.org/D75761 Files: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.c

[Lldb-commits] [PATCH] D76163: [lldb/Reproducers] Decode run-length encoding in GDB replay server.

shafik added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunication.cpp:1374 + // Number of time the previous character is repeated. + int repeat_count = *++c + 3 - ' '; + // We have the char_to_repeat and repeat_count. Now push

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

shafik added a comment. In D76168#1924539 , @hubert.reinterpretcast wrote: > I am not sure what the usage scenario is that this is meant to support. Is it > user input that tries to name a specialization of a template `operator<` > without separation to

[Lldb-commits] [PATCH] D75761: [lldb] Remove template parameters from FunctionTemplateDecl names

shafik updated this revision to Diff 250644. shafik marked 2 inline comments as done. shafik added a comment. Addressing minor comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75761/new/ https://reviews.llvm.org/D75761 Files: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserC

[Lldb-commits] [PATCH] D75761: [lldb] Remove template parameters from FunctionTemplateDecl names

This revision was automatically updated to reflect the committed changes. Closed by commit rG9e2715aaacaa: [lldb] Remove template parameters from FunctionTemplateDecl names (authored by shafik). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75761/new

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

shafik added a comment. In D76168#1929211 , @labath wrote: > In D76168#1925176 , @shafik wrote: > > > Long-term I would like to modify clang to stop doing that for LLDB, but > > LLDB will still have to support olde

[Lldb-commits] [PATCH] D76476: Add formatter for libc++ std::unique_ptr

shafik created this revision. shafik added reviewers: vsk, jingham. This adds a formatter for libc++ `std::unique_ptr`. We currently already have formatters for `std::shared_ptr` and `std::weak_ptr`. I also refactored `GetValueOfCompressedPair(...)` out of `LibCxxList.cpp` since I need the same

[Lldb-commits] [PATCH] D76471: Remap the target SDK directory to the host SDK directory

shafik added inline comments. Comment at: lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp:1318 + +static llvm::VersionTuple ParseSDKVersion(llvm::StringRef &name) { + unsigned i = 0; We should document what we expect the format to be. I mean I can deduce

[Lldb-commits] [PATCH] D76476: Add formatter for libc++ std::unique_ptr

shafik updated this revision to Diff 251707. shafik marked 4 inline comments as done. shafik added a comment. Addressing comments on naming and not duplicating the regex CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76476/new/ https://reviews.llvm.org/D76476 Files: lldb/include/lldb/

[Lldb-commits] [PATCH] D76476: Add formatter for libc++ std::unique_ptr

shafik added inline comments. Comment at: lldb/source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:618 + "unique_ptr synthetic children", + ConstString("^(std::__[[:alnum:]]+::)unique_ptr<.+>(( )?&)?$"), + stl_synth_flags, true); vsk wrote: > D

[Lldb-commits] [PATCH] D76476: Add formatter for libc++ std::unique_ptr

shafik updated this revision to Diff 251783. shafik marked an inline comment as done. shafik added a comment. Adding addition tests for references CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76476/new/ https://reviews.llvm.org/D76476 Files: lldb/include/lldb/DataFormatters/Formatte

[Lldb-commits] [PATCH] D76476: Add formatter for libc++ std::unique_ptr

This revision was automatically updated to reflect the committed changes. Closed by commit rGa567d6809e15: [DataFormatters] Add formatter for libc++ std::unique_ptr (authored by shafik). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D76476?vs=251783&id=252112#t

[Lldb-commits] [PATCH] D76736: [LLDB] Fix parsing of IPv6 host:port inside brackets

shafik added a reviewer: labath. shafik added a comment. We need tests it looks like `UriParserTest.cpp` is the right place. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76736/new/ https://reviews.llvm.org/D76736 ___

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

shafik created this revision. shafik added reviewers: aprantl, labath. shafik added a reviewer: vsk. When parsing DWARF and laying out bit-fields we currently don't take into account whether we have a base class or not. Currently if the first field is a bit-field but the bit offset is due a fiel

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

shafik updated this revision to Diff 252941. shafik marked 4 inline comments as done. shafik added a comment. Addressing comments: - Adding more detailed comments - Adding test for cases that currently fail b/c we don't enable C++20 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76168/ne

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

shafik added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2671 // If we have a gap between the last_field_end and the current // field we have an unnamed bit-field if (this_field_info.bit_of

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

shafik updated this revision to Diff 252984. shafik marked 4 inline comments as done. shafik added a comment. - Expanded comments - Adjusted conditionals CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76808/new/ https://reviews.llvm.org/D76808 Files: lldb/source/Plugins/SymbolFile/DWA

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

shafik updated this revision to Diff 253142. shafik marked 6 inline comments as done. shafik added a comment. Fixing up loose language in the comments and adding periods. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76168/new/ https://reviews.llvm.org/D76168 Files: lldb/source/Plugi

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

shafik updated this revision to Diff 253147. shafik marked 2 inline comments as done. shafik added a comment. Minor fixes CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76808/new/ https://reviews.llvm.org/D76808 Files: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp lld

[Lldb-commits] [PATCH] D76808: Fix handling of bit-fields when there is a base class when parsing DWARF

This revision was automatically updated to reflect the committed changes. Closed by commit rG00c8120acbac: [LLDB] Fix handling of bit-fields when there is a base class when parsing DWARF (authored by shafik). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D76168: CPlusPlusNameParser does not handles templated operator< properly

This revision was automatically updated to reflect the committed changes. Closed by commit rG8016d61e3cf4: [LLDB] CPlusPlusNameParser does not handles templated operator< properly (authored by shafik). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D76964: Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.

shafik added a comment. You also mentioned the `id` case failing as well. We should add that case to the test as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76964/new/ https://reviews.llvm.org/D76964

[Lldb-commits] [PATCH] D77153: [lldb/DataFormatters] Display null C++ pointers as nullptr

shafik added inline comments. Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libstdcpp/string/main.cpp:13 std::basic_string uchar(5, 'a'); +std::string *null = nullptr; S.assign(L"!"); // Set break point at this line. --

[Lldb-commits] [PATCH] D77326: 1/2: [nfc] [lldb] Unindent code

shafik added a comment. I feel like this should have been split in two. It feels like there are straight forward obviously correct changes and a several others that require some thinking about but look correct. Comment at: lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.

[Lldb-commits] [PATCH] D77327: [nfc] [lldb] 2/2: Introduce DWARF callbacks

shafik added inline comments. Comment at: lldb/include/lldb/Core/UniqueCStringMap.h:103 + // Helper iterator for the 'equal_range' method. + class CString_iterator { + public: `CStringIterator`? Comment at: lldb/include/lldb/Core/UniqueCStr

[Lldb-commits] [PATCH] D77326: 1/2: [nfc] [lldb] Unindent code

shafik added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/HashedNameToDIE.cpp:70 const dw_tag_t die_tag = die_info_array[i].tag; -if (die_tag != 0 && die_tag != DW_TAG_class_type && -die_tag != DW_TAG_structure_type) +if (!(die_tag == 0 |

[Lldb-commits] [PATCH] D77582: [CommandInterpreter] Implement UserCommandExists

shafik added a comment. I agree, if we don't have an immediate use then I don't think we should add it now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77582/new/ https://reviews.llvm.org/D77582 ___

[Lldb-commits] [PATCH] D96550: Fix LLDB_LOG calls to use correct formatting

shafik created this revision. shafik added a reviewer: teemperor. shafik requested review of this revision. It looks like a previous change switched these from `LLDB_LOGF` but did not update the format strings. https://reviews.llvm.org/D96550 Files: lldb/source/Plugins/ExpressionParser/Clang

[Lldb-commits] [PATCH] D96537: Make the error condition in Value::ValueType explicit (NFC)

shafik added inline comments. Comment at: lldb/source/Core/Value.cpp:321 + case ValueType::Invalid: +break; + case ValueType::Scalar: { Would it make sense to also do an `error.SetErrorString(...`? Comment at: lldb/source/Core/Value.cpp:

[Lldb-commits] [PATCH] D96550: Fix LLDB_LOG calls to use correct formatting

This revision was automatically updated to reflect the committed changes. Closed by commit rG9f175998debc: [LLDB] Fix LLDB_LOG calls to use correct formatting (authored by shafik). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llv

[Lldb-commits] [PATCH] D96537: Make the error condition in Value::ValueType explicit (NFC)

shafik accepted this revision. shafik added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96537/new/ https://reviews.llvm.org/D96537 ___ lldb-commits mailing list lldb-commits@l

[Lldb-commits] [PATCH] D96537: Make the error condition in Value::ValueType explicit (NFC)

shafik added inline comments. Comment at: lldb/source/Core/Value.cpp:574 switch (m_value_type) { -case eValueTypeScalar: // raw scalar value +case ValueType::Invalid: +case ValueType::Scalar: // raw scalar value aprantl wrote: > shafik wrote: > >

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik created this revision. shafik added reviewers: aaron.ballman, rsmith, teemperor. Herald added a subscriber: arphaman. shafik requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. Currently `TypePrinter` lumps anonymous classes and unname

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik added a comment. Note: I am not fixing how we treat anonymous and unnamed enums, I could not find a way to figure out if an enum was anonymous or not. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm.org/D96807 __

[Lldb-commits] [PATCH] D96861: [lldb][NFC] Delete deleted const char* overloads of SetValueFromString

shafik added a comment. Nice catch! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96861/new/ https://reviews.llvm.org/D96861 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.o

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik updated this revision to Diff 324431. shafik added a comment. - Went with unnamed enums Vs anonymous enums CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm.org/D96807 Files: clang/lib/AST/TypePrinter.cpp clang/test/AST/ast-dump-decl-json.c clan

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik marked an inline comment as done. shafik added inline comments. Comment at: clang/lib/AST/TypePrinter.cpp:1308 +} else if ((isa(D) && cast(D)->isAnonymousStructOrUnion()) || +isa(D)) { OS << "anonymous"; aaron.ballman wrote: > I think `

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

This revision was automatically updated to reflect the committed changes. shafik marked an inline comment as done. Closed by commit rGecb90b55454e: Modify TypePrinter to differentiate between anonymous struct and unnamed struct (authored by shafik). Herald added projects: clang, LLDB. Repository:

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik added a comment. Herald added a subscriber: JDevlieghere. Reverted the changes because I missed the clangd test suite and don't know how long it will take to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm

[Lldb-commits] [PATCH] D96807: Modify TypePrinter to differentiate between anonymous struct and unnamed struct

shafik updated this revision to Diff 325100. shafik added a comment. Herald added subscribers: usaxena95, kadircet. Fixing tests that I missed before. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96807/new/ https://reviews.llvm.org/D96807 Files: clang-tools-extra/clangd/unittests/Fi

[Lldb-commits] [PATCH] D97205: [lldb][NFC] Don't inherit from UserID in ValueObject

shafik added inline comments. Comment at: lldb/include/lldb/Core/ValueObject.h:888 + /// Unique identifier for every value object. + UserID m_id; + This makes more sense, `ValueObject` is not a `UserID` but contains one. CHANGES SINCE LAST ACTION https://re

[Lldb-commits] [PATCH] D97249: [lldb] Support debugging utility functions

shafik added inline comments. Comment at: lldb/source/Expression/FunctionCaller.cpp:324 EvaluateExpressionOptions real_options = options; real_options.SetDebug(false); + real_options.SetGenerateDebugInfo(debug); It feels a little weird you are using the na

[Lldb-commits] [PATCH] D97586: [mlir][lldb] Fix several gcc warnings in mlir and lldb

shafik added a comment. lldb change LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97586/new/ https://reviews.llvm.org/D97586 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.l

[Lldb-commits] [PATCH] D97739: Add a progress class that can track and report long running operations that happen in LLDB.

shafik added inline comments. Comment at: lldb/include/lldb/Core/Progress.h:33 + std::atomic m_completed; + const uint64_t m_total; +}; Maybe I am misunderstanding but if we are going to have a total are we also going to variable to keep track of the expected

[Lldb-commits] [PATCH] D98653: [lldb] Refactor variable paths to support languages with non-pointer "this" (NFC)

shafik added inline comments. Comment at: lldb/include/lldb/Symbol/CompilerDeclContext.h:77 /// in a struct, union or class. - bool IsClassMethod(lldb::LanguageType *language_ptr, - bool *is_instance_method_ptr, - ConstString *langu

[Lldb-commits] [PATCH] D98879: [lldb/PlatformPOSIX] Change LoadImage default to RTLD_LAZY

shafik added a comment. Thank you for adding the commentary on what `RTLD_LAZY` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98879/new/ https://reviews.llvm.org/D98879 ___ lldb-commits mailing list lldb

[Lldb-commits] [PATCH] D99315: [lldb] Support lazily named classes in the Objective-C classes

shafik added inline comments. Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp:199 + +ClassInfo *class_infos = (ClassInfo *)class_infos_ptr; + Is this pointer and `realized_class_list` always non-NULL? ==

[Lldb-commits] [PATCH] D99571: Update ProcessMachCore::DoLoadCore to handle binary hints with and without addresses

shafik added inline comments. Comment at: lldb/test/API/macosx/lc-note/firmware-corefile/TestFirmwareCorefiles.py:142 +main_addr = main_sym.GetStartAddress() +self.assertGreater(main_addr.GetLoadAddress(self.target), 0x700) +self.assertNotEqual(m

[Lldb-commits] [PATCH] D99571: Update ProcessMachCore::DoLoadCore to handle binary hints with and without addresses

shafik added inline comments. Comment at: lldb/source/Plugins/Process/mach-core/ProcessMachCore.cpp:214 + if (FileSystem::Instance().Exists(module_spec.GetFileSpec())) { +module_sp.reset(new Module(module_spec)); + } We can use `make_shared` her

[Lldb-commits] [PATCH] D99694: Add support for getting signed ObjC tagged pointer values

shafik added inline comments. Comment at: lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp:2494 + m_objc_debug_taggedpointer_payload_rshift); + return ClassDescriptorSP(new ClassDescriptorV2Tagged( + actual_class_descriptor_sp, data_pa

[Lldb-commits] [PATCH] D99827: Clarifying the documentation for variable formatting wrt to qualifiers and adding a test that demonstrates this

shafik created this revision. shafik added reviewers: jingham, jasonmolenda, aprantl. shafik requested review of this revision. When looking up user specified formatters qualifiers are removed from types before matching, I have added a clarifying example to the document and added an example to a

[Lldb-commits] [PATCH] D99827: Clarifying the documentation for variable formatting wrt to qualifiers and adding a test that demonstrates this

shafik updated this revision to Diff 335929. shafik added a comment. Updating test to use frame var test. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99827/new/ https://reviews.llvm.org/D99827 Files: lldb/docs/use/variable.rst lldb/test/API/functionalities/data-formatter/data-fo

[Lldb-commits] [PATCH] D99827: Clarifying the documentation for variable formatting wrt to qualifiers and adding a test that demonstrates this

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG79ac5bbb96c4: [LLDB] Clarifying the documentation for variable formatting wrt to qualifiers… (authored by shafik). Herald added a project: LLDB. Cha

[Lldb-commits] [PATCH] D100212: [lldb] Delete dead StackFrameList::Merge

shafik added a comment. I think it is pretty safe to remove it after 10+ years is not being used. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100212/new/ https://reviews.llvm.org/D100212 ___ lldb-commits mailing list lldb-commits@lists.llvm

[Lldb-commits] [PATCH] D100795: [lldb] Fix RichManglingContext::FromCxxMethodName() leak

shafik added inline comments. Comment at: lldb/source/Core/RichManglingContext.cpp:24 + std::free(m_ipd_buf); + if (m_cxx_method_parser.hasValue()) { +assert(m_provider == PluginCxxLanguage); This code is duplicated from `ResetProvider(...)` we should facto

[Lldb-commits] [PATCH] D100800: [lldb] Fix demangler leaks in the DWARF AST parser

shafik added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1249 attrs.is_inline); +free(buf); MaskRay wrote: > teemperor wrote: > > `std::free` ? > `std::` for C library functions is uncommon. >

[Lldb-commits] [PATCH] D100795: [lldb] Fix RichManglingContext::FromCxxMethodName() leak

shafik added a comment. Thank you for reforactoring into `ResetCxxMethodParser`. Comment at: lldb/source/Core/RichManglingContext.cpp:23 +RichManglingContext::~RichManglingContext() { + std::free(m_ipd_buf); + ResetCxxMethodParser(); rupprecht wrote: > JDevli

[Lldb-commits] [PATCH] D101094: lldb/Instrumentation: NFC-ish use GetFrameCodeAddressForSymbolication()

shafik added inline comments. Comment at: lldb/source/Plugins/InstrumentationRuntime/MainThreadChecker/InstrumentationRuntimeMainThreadChecker.cpp:273 + ThreadSP new_thread_sp = std::make_shared( + *process_sp, tid, PCs, pcs_are_call_addresses); A nitpic

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

shafik added a comment. Thank you, this is awesome. Comment at: lldb/docs/resources/test.rst:206 + +**Don't unnecessarily launch the test executable.** +Launching a process and running to a breakpoint can often be the most While I agree with this, it also f

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

shafik added inline comments. Comment at: lldb/source/API/SBThread.cpp:852 +auto location_spec = SourceLocationSpec::Create( +step_file_spec, line, column, check_inlines, exact); +lldbassert(location_spec && "Invalid SourceLocationSpec."); ``` ste

[Lldb-commits] [PATCH] D101237: [lldb] Fix [[no_unique_address]] and libstdc++ 11's std::unique_ptr

shafik added a comment. I think @dblaikie original idea of adding a DWARF attribute for this case is the right way to go here. AFAICT this will change the answer to basic questions such as what size a `struct` is and this will likely lead to confusion from our users who will expect the answers

[Lldb-commits] [PATCH] D101390: Change Target::ReadMemory to ensure the amount of memory read from the file-cache is the amount requested.

shafik added inline comments. Comment at: lldb/source/Target/Target.cpp:1778 +else if (file_cache_bytes_read > 0) { + file_cache_read_buffer.reset(malloc(file_cache_bytes_read)); + std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read); -

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

shafik added inline comments. Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:925 lldb::SBDeclaration declaration = v.GetDeclaration(); -std::string file_name(declaration.GetFileSpec().GetFilename()); const uint32_t line = declaration.GetLine(); wa

[Lldb-commits] [PATCH] D101390: Change Target::ReadMemory to ensure the amount of memory read from the file-cache is the amount requested.

shafik added inline comments. Comment at: lldb/source/Target/Target.cpp:1778 +else if (file_cache_bytes_read > 0) { + file_cache_read_buffer.reset(malloc(file_cache_bytes_read)); + std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read); -

[Lldb-commits] [PATCH] D101390: Change Target::ReadMemory to ensure the amount of memory read from the file-cache is the amount requested.

shafik added inline comments. Comment at: lldb/source/Target/Target.cpp:1778 +else if (file_cache_bytes_read > 0) { + file_cache_read_buffer.reset(malloc(file_cache_bytes_read)); + std::memcpy(file_cache_read_buffer.get(), dst, file_cache_bytes_read); -

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

shafik accepted this revision as: shafik. shafik added a comment. This revision is now accepted and ready to land. I don't have any other feedback, LGTM. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 _

[Lldb-commits] [PATCH] D101585: [lldb] Handle missing SBStructuredData copy assignment cases

shafik added a comment. Herald added a subscriber: JDevlieghere. LGTM, it seems like there a few places where we are not consistently checking `m_impl_up` is valid and other places we are using the conditional operator to check if it is set or not. Repository: rG LLVM Github Monorepo CHANGE

[Lldb-commits] [PATCH] D101627: [lldb] More tests for DumpDataExtractor

shafik added a comment. +1 In D101627#2729594 , @jingham wrote: > I would resist this change. It's unnecessarily disruptive, would again break > git archeology, and really have no significant benefit. I also think the > lldb conventions for naming th

[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo

shafik added inline comments. Comment at: lldb/source/Plugins/Disassembler/LLVMC/DisassemblerLLVMC.cpp:909 + new llvm::MCContext(llvm::Triple(triple), asm_info_up.get(), + reg_info_up.get(), nullptr, subtarget_info_up.get())); if (!context_up) --

[Lldb-commits] [PATCH] D101462: Make it possible for targets to define their own MCObjectFileInfo

shafik added inline comments. Comment at: clang/lib/Parse/ParseStmtAsm.cpp:589 + TheTarget->createMCObjectFileInfo( + /*PIC*/ false, Ctx)); + Ctx.setObjectFileInfo(MOFI.get()); `/*PIC=*/false` Comment at: llvm/tools/llvm-mca/llvm

[Lldb-commits] [PATCH] D102085: Add an "interrupt timeout" to Process, fix a bug in handling interrupt timeouts in

shafik added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:3129 uint8_t error_no = m_gdb_comm.SendGDBStoppointTypePacket( -eBreakpointSoftware, true, addr, bp_op_size); +eBreakpointSoftware, true, addr, bp_op_size, G

[Lldb-commits] [PATCH] D102445: Switch from using member_clang_type.GetByteSize() to member_type->GetByteSize() in ParseSingleMember

shafik created this revision. shafik added reviewers: aprantl, teemperor, labath. shafik requested review of this revision. We have a bug in which using `member_clang_type.GetByteSize()` triggers record layout and during this process since the record was not yet complete we ended up reaching a r

[Lldb-commits] [PATCH] D102445: Switch from using member_clang_type.GetByteSize() to member_type->GetByteSize() in ParseSingleMember

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2182eda30624: [LLDB] Switch from using member_clang_type.GetByteSize() to member_type… (authored by shafik). Herald added a project: LLDB. Repositor

[Lldb-commits] [PATCH] D84267: Thread ExecutionContextScope through GetByteSize where possible (NFC-ish)

shafik added inline comments. Comment at: lldb/source/Plugins/ABI/ARC/ABISysV_arc.cpp:461 if (type_flags & eTypeIsInteger) { -const size_t byte_size = compiler_type.GetByteSize(nullptr).getValueOr(0); +const size_t byte_size = compiler_type.GetByteSize(thread).getValue

[Lldb-commits] [PATCH] D84267: Thread ExecutionContextScope through GetByteSize where possible (NFC-ish)

shafik added inline comments. Comment at: lldb/source/Plugins/ExpressionParser/Clang/IRForTarget.cpp:308 lldb::TargetSP target_sp(m_execution_unit.GetTarget()); - lldb_private::ExecutionContext exe_ctx(target_sp, true); - llvm::Optional bit_size = - m_result_type.GetBit

[Lldb-commits] [PATCH] D84272: Add checks for ValueObjectSP in Cocoa summary providers

shafik created this revision. shafik added a reviewer: jingham. We saw a crash recently that looks related to we had good `ValueObjectSP` for some Cocoa summary providers. This adds checks before we use them when calling `NSStringSummaryProvider`. https://reviews.llvm.org/D84272 Files: lldb

<    1   2   3   4   5   6   7   8   >