[Lldb-commits] [PATCH] D104525: [lldb] Assert that CommandResultObject error messages are not empty

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good, if this already passes all tests/etc (ie: there actually aren't any callers passing empty strings/non-failed errors/etc) Comment at: lldb/source/Interpreter

[Lldb-commits] [PATCH] D104380: [lldb] Set return object failed status even if error string is empty

dblaikie added a comment. In D104380#2826465 , @DavidSpickett wrote: > This change would have been part of https://reviews.llvm.org/D103701 if I had > realised that this was in fact how it worked. I separated it from the follow > ons because of that an

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

dblaikie added a comment. Might be worth checking who wrote some of this/nearby code and whether they're still active in the community and see if they can review this - not sure if Pavel is around/has the bandwidth to review this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105732/ne

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie added a comment. In D106084#2886659 , @jmorse wrote: > This is going to be excellent for linux targets and similar, > > In D106084#2882970 , @probinson > wrote: > >> + @jmorse who is better placed than I

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie added a comment. In D106084#2890469 , @probinson wrote: >> (hence the renaming of "limited" a long time ago to "standalone-debug" to >> create a policy/philosophy around what goes in each category). > > Sorry, what? I thought -fstandalone-debug

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie accepted this revision. dblaikie added subscribers: probinson, aprantl. dblaikie added a comment. This revision is now accepted and ready to land. Please wait for sign-off from @aprantl (or another appropriate Apple representative) & @probinson (or another appropriate Sony representative

[Lldb-commits] [PATCH] D103172: [lldb][NFC] Allow range-based for loops over DWARFDIE's children

dblaikie added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h:97-105 + template + llvm::iterator_range children() const { +return llvm::make_range(T(*this), T()); + } +}; + +class DWARFDIE::child_iterator rather than a temepl

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie added a comment. In D106084#2897515 , @jmorse wrote: > David wrote: > >> think what I'm missing here: If -fno-standalone-debug is already in use/the >> default and is causing missing types because parts of the program are bulit >> without debug

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

dblaikie accepted this revision. dblaikie added a comment. Sounds like this is good for Google and Sony, and Apple doesn't use `-fno-standalone-debug`/`-flimit-debug-info` anyway, so probably about ready to move forward, then? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

dblaikie added a comment. In D105732#2942355 , @clayborg wrote: > Sorry for the delay. This seems like it should work just fine. Where is this > actually used? I thought most clients should be using posix_spawn() as this > is the preferred launching mec

[Lldb-commits] [PATCH] D105732: [lldb] Update logic to close inherited file descriptors.

dblaikie added a comment. In D105732#2942716 , @jingham wrote: > Do modern Linux's not have posix_spawn? If it exists that's a better > interface, and lets the system handle a lot of the complicated machinations > you have to do by hand if you roll it

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

dblaikie added a comment. I assume there's already test coverage for rnglistx in debug_info.dwo/split unit? (because in that case there's no rnglists_base, but rnglistx is usable) - could you explain how this code avoids treating the split unit rnglists_base == 0 case as "there is no rnglists_b

[Lldb-commits] [PATCH] D106466: [llvm+lldb] Fix#2 of DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D106466#2950268 , @jankratochvil wrote: > In D106466#2947710 , @dblaikie > wrote: > >> I assume ther

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

dblaikie added inline comments. Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:101 S.assign(L"!"); // Set break point at this line. +std::string *not_a_string = (std::string *) 0x0; +touch_string(*not_a_string)

[Lldb-commits] [PATCH] D108228: Fix error handling in the libcxx std::string formatter

dblaikie added inline comments. Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/string/main.cpp:102 +std::string *not_a_string = (std::string *) 0x0; +touch_string(*not_a_string); return 0; teemperor wrote: > dblai

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

dblaikie added a comment. In D97786#2972153 , @pfaffe wrote: > This change breaks all existing uses of relative comp_dirs that don't > accidentally make all of them relative to the executable's directory already. > > It's easy to construct broken use case

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

dblaikie added a comment. In D97786#2974202 , @pfaffe wrote: > In D97786#2973868 , @dblaikie wrote: > >> This doesn't solve all use cases/it's not a terribly general purpose >> situation - using -fdebug-prefix-map/

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

dblaikie added a comment. In D97786#2976549 , @pfaffe wrote: > In D97786#2974879 , @dblaikie wrote: > >> Not fixable? Not sure I follow. In the case of cwd-relative (old behavior) >> you can fix the behavior by cha

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

dblaikie added a comment. In D75929#1915009 , @labath wrote: > I haven't digested the patch yet, but I am wondering if you've seen the > recent discussion (`DWP mixed (DWARFv4/pre-standard + DWARFv5) content`) on > dwarf-discuss (link1 >

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

dblaikie added a comment. > This is almost what you are doing right now -- the only difference is that > the "internal" enum would no longer be internal -- it would actually match > the on-disk format of a v5 index. This v5 enum would contain the official > DWARFv5 constants as well as the new

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

dblaikie added a comment. Side note: This has become complicated enough it might be worth separating these patches now rather than later - changes to dumping should be separate from changes to llvm-dwp, for instance. In D75929#1926834 , @labath wrote: >

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

dblaikie added inline comments. Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:47-48 +// For pre-standard ones, which correspond to sections being deprecated in +// DWARFv5, the values are chosen more or less arbitrary and a tag "_EXT_" +// is added to the names.

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

dblaikie added inline comments. Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFUnitIndex.h:47-48 +// For pre-standard ones, which correspond to sections being deprecated in +// DWARFv5, the values are chosen more or less arbitrary and a tag "_EXT_" +// is added to the names.

[Lldb-commits] [PATCH] D75929: [DebugInfo] Support DWARFv5 index sections.

dblaikie accepted this revision. dblaikie added a comment. In D75929#1954443 , @labath wrote: > Thanks for doing this. This looks fine to me. @dblaikie, @jhenderson, do you > have any additional comments? nah, nothing dire that comes to mind - thanks fo

[Lldb-commits] [PATCH] D77141: [DebugInfo] Rename section identifiers which are deprecated in DWARFv5. NFC.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sure - looks good CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77141/new/ https://reviews.llvm.org/D77141 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2565677 , @jankratochvil wrote: > In D96778#2565414 , @werat wrote: > >> I can't claim I fully understand what's the difference here, but this aligns >> with your comment at htt

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added a comment. Perhaps all these clone implementations could be provided via mixin instead? struct base { virtual std::shared_ptr Clone() = 0; ... }; template struct base_clone_helper : IntermediateBase { static_assert(std::is_base_of::value); std::shared_ptr Cl

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added inline comments. Comment at: lldb/unittests/Interpreter/TestOptionValue.cpp:173 + // Trigger the callback second time. + file_list_copy_ptr->SetValueFromString(llvm::StringRef("0 another/path"), + eVarSetOperationReplace);

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2567255 , @jankratochvil wrote: > In D96778#2566208 , @dblaikie wrote: > >> I expect it'd be good to have a test case showing the sort of DWARF that DWZ >> produces for cross-CU

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added a comment. > CRTP was my first implementation, however, I discarded it as more bug-prone. > Virtual Clone function at the interface is so common that, I believe, > everyone knows it must be overridden by a new derived class. The necessity of > inheriting from base_clone_helper is

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2568794 , @werat wrote: > To be honest, I'm not sure how to reproduce this kind of debug info. I've > tried a few examples (e.g. force inline the function from another CU) , but > it didn't work. > > Should we postpone

[Lldb-commits] [PATCH] D96817: Fix deep copying for OptionValue classes

dblaikie added a comment. In D96817#2569852 , @clayborg wrote: > In D96817#2569595 , @dblaikie wrote: > >>> CRTP was my first implementation, however, I discarded it as more >>> bug-prone. Virtual Clone function at

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2572405 , @jankratochvil wrote: > In D96778#2566208 , @dblaikie wrote: > >> I expect it'd be good to have a test case showing the sort of DWARF that DWZ >> produces for cross-CU

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2573076 , @jankratochvil wrote: > In D96778#2572816 , @dblaikie wrote: > >> rolls this into one file with two CUs - bit easier to deal with. > > Then one could not use the `.file

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2576913 , @jankratochvil wrote: > In D96778#2576881 , @dblaikie wrote: > >> In D96778#2573076 , @jankratochvil >> wrote: >> >>> Personal

[Lldb-commits] [PATCH] D96778: [lldb] Fix handling of `DW_AT_decl_file` according to D91014

dblaikie added a comment. In D96778#2576980 , @jankratochvil wrote: > In D96778#2576927 , @dblaikie wrote: > >> What part of DWZ is specified by DWARFv5? > > Only `DW_*_sup` - being used by DWZ with DWARF-4 as `DW_

[Lldb-commits] [PATCH] D98289: [lldb] Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

dblaikie added a comment. In D98289#2618881 , @jankratochvil wrote: > Ah there is already a discussion from it: > http://lists.dwarfstd.org/pipermail/dwarf-discuss-dwarfstd.org/2021-March/004765.html Yeah, happy to discuss/clarify/help with anything in

[Lldb-commits] [PATCH] D98289: [lldb] Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

dblaikie added inline comments. Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h:116 Optional getOffsetEntry(DataExtractor Data, uint32_t Index) const { -if (Index > HeaderData.OffsetEntryCount) +if (Index >= HeaderData.OffsetEntryCount) return Non

[Lldb-commits] [PATCH] D86110: [WIP][DebugInfo] Lazily parse debug_loclist offsets

dblaikie added inline comments. Comment at: llvm/include/llvm/DebugInfo/DWARF/DWARFListTable.h:116 + Optional getOffsetEntry(DataExtractor Data, uint32_t Index) const { +if (Index > HeaderData.OffsetEntryCount) + return None; jankratochvil wrote: > That

[Lldb-commits] [PATCH] D98996: Teach DWARFExpression about DWARF 4+ Location Descriptions

dblaikie added a comment. (idle question: Is this code remotely related to any code in LLVM's libDebugInfoDWARF? Does this patch take us closer to or further away from unifying them? If it takes us further away, any chance of designing it in a direction that points towards each other rather tha

[Lldb-commits] [PATCH] D99120: [lldb] Silence GCC warnings about format not being a string literal in LLDB_SCOPED_TIMER

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Herald added a subscriber: JDevlieghere. Looks right to me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99120/new/ https://reviews.llvm.org

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

dblaikie added inline comments. Comment at: lldb/source/API/SBDebugger.cpp:857 +error = m_opaque_sp->GetTargetList().CreateTarget( +*m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp, +target_sp); jingham wrote: > claybo

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

dblaikie added inline comments. Comment at: lldb/source/API/SBDebugger.cpp:857 +error = m_opaque_sp->GetTargetList().CreateTarget( +*m_opaque_sp, filename, arch, eLoadDependentsYes, platform_sp, +target_sp); clayborg wrote: > jingh

[Lldb-commits] [PATCH] D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB

dblaikie added a comment. (generally makes sense to me - but I'll leave it to some more lldb-focussed reviewers to do more review/approval) Usual caveat/question: Does this take us closer or further away from unifying this code with LLVM's libDebugInfoDWARF? (or neutral) Com

[Lldb-commits] [PATCH] D99401: Fix .debug_aranges parsing issues introduced with llvm error handling in LLDB

dblaikie added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugArangeSet.cpp:56 + if (m_header.length > 0) +m_next_offset = m_offset + *offset_ptr - m_offset + m_header.length; + else clayborg wrote: > dblaikie wrote: > > This ma

[Lldb-commits] [PATCH] D99702: [lldb-vscode] Use LLVM's ScopeExit to ensure we always terminate the debugger

dblaikie added a comment. @wallace - minor inconvenience, but approving a patch in Phabricator without any textual comment does not result in an email to the list, making it look like a patch is being committed without approval - could you include some text in your Phab approvals, as it makes i

[Lldb-commits] [PATCH] D98289: [lldb] 2/2: Fix DW_AT_ranges DW_FORM_sec_offset not using DW_AT_rnglists_base (used by GCC)

dblaikie added inline comments. Comment at: lldb/test/Shell/SymbolFile/DWARF/DW_AT_range-DW_FORM_sec_offset.s:11 +# Failure was the block range 1..2 was not printed plus: +# error: DW_AT_range-DW_FORM_sec_offset.s.tmp {0x003f}: DIE has DW_AT_ranges(0xc) attribute, but range

[Lldb-commits] [PATCH] D100447: [lldb] Silence GCC warnings about control reaching the end of non-void functions. NFC.

dblaikie added a comment. I'm guessing this change will break the build in a different way, triggering clang's -Wcovered-switch-default - the usual way this gcc/clang diagnostic pinch is resolved is by putting the unreachable after the switch/at the end of the function, instead of in a default

[Lldb-commits] [PATCH] D100447: [lldb] Silence GCC warnings about control reaching the end of non-void functions. NFC.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good - thanks! Do you need me to commit this for you? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100447/new/ https://reviews.llvm.o

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

dblaikie added a comment. In D100191#2689543 , @mgorny wrote: > In D100191#2689489 , @labath wrote: > >> We should also start thinking about tests. I suppose the smallest piece of >> functionality that could be u

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

dblaikie added inline comments. Comment at: lldb/tools/driver/Driver.cpp:856 if (const char *reproducer_path = SBReproducer::GetPath()) { -// Leaking the string on purpose. -std::string *finalize_cmd = new std::string(argv0); +static std::string *fi

[Lldb-commits] [PATCH] D100806: [lldb] Fix one leak in reproducer

dblaikie added inline comments. Comment at: lldb/tools/driver/Driver.cpp:856 if (const char *reproducer_path = SBReproducer::GetPath()) { -// Leaking the string on purpose. -std::string *finalize_cmd = new std::string(argv0); +static std::string *fi

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

dblaikie added inline comments. Comment at: lldb/source/Core/RichManglingContext.cpp:23 +RichManglingContext::~RichManglingContext() { + std::free(m_ipd_buf); + ResetCxxMethodParser(); JDevlieghere wrote: > shafik wrote: > > rupprecht wrote: > > > JDevlieghere

[Lldb-commits] [PATCH] D96236: [lldb] DWZ 1/9: Pass main DWARFUnit * along DWARFDIEs

dblaikie added a comment. If this is going to be plumbed through everywhere (ie: a DWARFDIE is mostly useless without both its lexical DWARFUnit and its imported-into DWARFUnit) then maybe it makes more sense to add this DWARFUnit* to the DWARFBaseDie type alongside the lexical DWARFUnit? Rep

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

dblaikie added a subscriber: aprantl. dblaikie added a comment. > As DWARF does not contain the [[no_unique_address]] attribute the patch adds > it to every record member, could it be a problem? Given that no_unique_address changes the layout of data structures ( https://clang.llvm.org/docs/Att

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

dblaikie added a comment. In D101237#2714962 , @jankratochvil wrote: > In D101237#2714953 , @dblaikie > wrote: > >> Given that no_unique_address changes the layout of data structures ( >> https://clang.llvm.org

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

dblaikie added a comment. In D101237#2718456 , @shafik wrote: > 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`

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

dblaikie added a comment. In D101237#2728726 , @teemperor wrote: >> (and it could tell clang exactly how large the structure is too - from the >> DWARF) > > We are actually doing that to my knowledge and return the `DW_AT_byte_size` > value for the reco

[Lldb-commits] [PATCH] D102092: [lldb] Enable -Wmisleading-indentation

dblaikie added a comment. might as well enable it for LLVM more generally? (though probably lldb specifically, and lots of llvm more generally, would need cleanup before this is enabled? Have you checked if lldb or other parts of llvm build cleanly with this warning enabled?) Repository: rG

[Lldb-commits] [PATCH] D102092: [lldb] Enable -Wmisleading-indentation

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good to me if - if the build (of ~everything in the monorepo) is clean - and do please keep an eye on the buildbots when this is committed as there's likely to be cleanup here and t

[Lldb-commits] [PATCH] D102630: [lit] Stop using PATH to lookup clang/lld/lldb unless requested

dblaikie added a comment. @thopre - as an aside: It'd be helpful if you could include some text in the text box when marking something "approved" through phabricator. There's a bug/limitation that approvals without text don't produce email to the mailing list - so it looks like a patch hasn't b

[Lldb-commits] [PATCH] D102851: [lldb] Improve invalid DWARF DW_AT_ranges error reporting

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me - at least includes more values from the file to point to the problem. Though phrasing might be better (I guess some other code prints the "DW_AT_ranges(0x0) attribute" pa

[Lldb-commits] [PATCH] D102851: [lldb] Improve invalid DWARF DW_AT_ranges error reporting

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Yeah, looks pretty good! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102851/new/ https://reviews.llvm.org/D102851 ___

[Lldb-commits] [PATCH] D84402: [lldb/DWARF] Add more line table validation

dblaikie added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1044 for (const llvm::DWARFDebugLine::Sequence &seq : line_table->Sequences) { +if (!list.ContainsFileAddressRange(seq.LowPC, seq.HighPC - seq.LowPC)) + continue;

[Lldb-commits] [PATCH] D79147: Switch to using -debug-info-kind=constructor as default (from =limited)

dblaikie added a subscriber: echristo. dblaikie added a comment. In D79147#2178104 , @russell.gallop wrote: > Hi, > > It looks like is causing one of the debuginfo-tests: > llgdb-tests/nrvo-string.cpp to fail, run on Linux. Failure as below. I don't > t

[Lldb-commits] [PATCH] D86110: [WIP][DebugInfo] Lazily parse debug_loclist offsets

dblaikie created this revision. dblaikie added a reviewer: labath. Herald added subscribers: llvm-commits, lldb-commits, hiraditya, aprantl. Herald added projects: LLDB, LLVM. dblaikie requested review of this revision. Herald added a subscriber: JDevlieghere. Parsing DWARFv5 debug_loclist offsets

[Lldb-commits] [PATCH] D86110: [WIP][DebugInfo] Lazily parse debug_loclist offsets

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D86110#2223905 , @labath wrote: > This sounds perfectly reasonable to me. The offsets are present in memory > already so there's no point in sto

[Lldb-commits] [PATCH] D86110: [WIP][DebugInfo] Lazily parse debug_loclist offsets

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf7a49d2aa691: [WIP][DebugInfo] Lazily parse debug_loclist offsets (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[Lldb-commits] [PATCH] D86110: [WIP][DebugInfo] Lazily parse debug_loclist offsets

dblaikie added a comment. In D86110#2225970 , @labath wrote: > In D86110#2224163 , @dblaikie wrote: > >> In D86110#2223905 , @labath wrote: >> >>> This sounds perfectly reaso

[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

dblaikie added inline comments. Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugLoc.h:93-108 + +/// Return the half-open range of addresses covered by this entry. +/// DW_LLE_offset_pair entries are resolved using the given base address, +/// and the supplied DWARF

[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

dblaikie added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:291-295 + EntryIterator Absolute = + getAbsoluteLocations( + SectionedAddress{BaseAddr, SectionedAddress::UndefSection}, + LookupPooledAddress) + .begin(); -

[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

dblaikie added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:291-295 + EntryIterator Absolute = + getAbsoluteLocations( + SectionedAddress{BaseAddr, SectionedAddress::UndefSection}, + LookupPooledAddress) + .begin(); -

[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

dblaikie added a comment. In D68270#1700108 , @probinson wrote: > Do we care whether llvm-dwarfdump's output bears any similarities to the > output from GNU readelf or objdump? There has been a push lately to get the > LLVM "binutils" to behave more lik

[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

dblaikie added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:291-295 + EntryIterator Absolute = + getAbsoluteLocations( + SectionedAddress{BaseAddr, SectionedAddress::UndefSection}, + LookupPooledAddress) + .begin(); -

[Lldb-commits] [PATCH] D69230: RFC: specialized Optional for T that can represent its own invalid state

dblaikie added a comment. In D69230#1717675 , @serge-sans-paille wrote: > On a personal note, as much as I like the idea (and I really do like it), I'd > rather have llvm::optional sticking to std::optional interface. (short-ish reply, while busy with

[Lldb-commits] [PATCH] D69230: RFC: specialized Optional for T that can represent its own invalid state

dblaikie added a comment. In D69230#1720048 , @labath wrote: > In D69230#1718191 , @lawrence_danna > wrote: > > > Seems like there's a consensus that if we have something like this it > > should be called `DenseOp

[Lldb-commits] [PATCH] D69230: RFC: specialized Optional for T that can represent its own invalid state

dblaikie added a comment. In D69230#1720255 , @labath wrote: > In D69230#1720246 , @dblaikie wrote: > > > In D69230#1720048 , @labath wrote: > > > > > That said, I think you

[Lldb-commits] [PATCH] D69230: RFC: specialized Optional for T that can represent its own invalid state

dblaikie added a comment. In D69230#1727209 , @labath wrote: > BTW, I was bored on the plane, so I created this proof of concept F10589216: > dense.cc . It needs a lot of cleanup of > course, but it demonstrates one wa

[Lldb-commits] [PATCH] D65942: Disallow implicit conversion from pointers to bool in llvm::toStringRef

dblaikie added a comment. Yeah, might be worth changing toStringRef to only accept bools: template typename std::enable_if::value, StringRef>::type toStringRef(T B) { ... } Or something like that? That'd avoid accidental "toStringRef(5)" or the like... CHANGES SINCE LAST ACTION https:/

[Lldb-commits] [PATCH] D65942: Disallow implicit conversion from pointers to bool in llvm::toStringRef

dblaikie added inline comments. Comment at: lldb/source/Symbol/TypeSystem.cpp:331 "TypeSystem for language " + - llvm::toStringRef(Language::GetNameForLanguageType(language)) + + llvm::StringRef(Language::GetNameForLanguageType(language)) +

[Lldb-commits] [PATCH] D65942: Disallow implicit conversion from pointers to bool in llvm::toStringRef

dblaikie added inline comments. Comment at: lldb/source/Symbol/TypeSystem.cpp:331 "TypeSystem for language " + - llvm::toStringRef(Language::GetNameForLanguageType(language)) + + llvm::StringRef(Language::GetNameForLanguageType(language)) +

[Lldb-commits] [PATCH] D65942: Disallow implicit conversion from pointers to bool in llvm::toStringRef

dblaikie added inline comments. Comment at: lldb/source/Symbol/TypeSystem.cpp:331 "TypeSystem for language " + - llvm::toStringRef(Language::GetNameForLanguageType(language)) + + llvm::StringRef(Language::GetNameForLanguageType(language)) +

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

dblaikie added a comment. In D62634#1527533 , @labath wrote: > Actually, there was a subtle change of behavior here. Before this change, if > we tried to parse a DIE using an abbreviation table from a different file, we > would most likely fail immediate

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

dblaikie added a comment. In D62634#1527634 , @probinson wrote: > In D62634#1527575 , @dblaikie wrote: > > > This is intentional behavior in clang (controllable under the > > -f[no-]split-dwarf-inlining flag, and m

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

dblaikie added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:27 +template +static llvm::Error createError(const char *Fmt, Ts &&... Vals) { + return createStringError(inconvertibleErrorCode(), Fmt, Vals...); I guess "Ts &&... Vals" should b

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

dblaikie added a comment. Given figuring out error handling for DataExtractor is perhaps a wider issue - if you want to go ahead with this change (continue with the review & defer error handling improvements for later, leave a FIXME, etc) that seems fine. Repository: rL LLVM CHANGES SINCE L

[Lldb-commits] [PATCH] D90598: [lldb/DWARF] Fix implementation of DW_OP_convert

dblaikie added a comment. In D90598#2368020 , @labath wrote: > Judging by http://lists.llvm.org/pipermail/llvm-dev/2020-October/145990.html, > dsymutil has a similar bug... That's my understanding. It means I'm not sure (though I'm not the expert here -

[Lldb-commits] [PATCH] D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

dblaikie added inline comments. Comment at: lldb/source/Expression/DWARFExpression.cpp:944 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_EXPRESSIONS)); + auto to_generic = [addr_size = opcodes.GetAddressByteSize()](auto v) { +bool is_signed = std::is_signed:

[Lldb-commits] [PATCH] D90840: [lldb/DWARF] Fix sizes of DW_OP_const[1248][us] and DW_OP_litN results

dblaikie accepted this revision. dblaikie added a comment. Sounds good Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90840/new/ https://reviews.llvm.org/D90840 ___ lldb-commits mailing list lldb-commits@

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. This causes several regressions in the gdb test suite we're running internally - the failures are: FAIL: gdb.base/break.exp: breakpoint at start of multi line while conditional FAIL: gdb.base/break.exp: breakpoint info FAIL: gdb.base/foll-exec.exp: step through ex

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. In D91734#2426897 , @rnk wrote: > I see. We should give that constant materialization a location. Yeah, likely - if this patch makes constant materialization local to the IR instruction - if there's a reasonable location the IR

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. Sometihng like this seems plausible to me: $ git diff diff --git clang/lib/CodeGen/CGExprScalar.cpp clang/lib/CodeGen/CGExprScalar.cpp index c906af8a4afa..c85ce46508a6 100644 --- clang/lib/CodeGen/CGExprScalar.cpp +++ clang/lib/CodeGen/CGExprScalar.cpp @@ -4

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. In D91734#2431247 , @probinson wrote: >> Sometihng like this seems plausible to me: > > Yes, I was playing with essentially that exact patch last night. It has no > effect on the final assembly on its own, but combined with my p

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. In D91734#2432188 , @probinson wrote: > See D92606 for a front-end patch to improve > locations in the IR. > That, plus reapplying this patch, should help out GDB. I haven't had a > chance to r

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. Also, FWIW, the other gdb test suite failures we saw were: FAIL: gdb.base/foll-exec.exp: step through execlp call FAIL: gdb.base/foll-exec.exp: step after execlp call FAIL: gdb.base/foll-exec.exp: print execd-program/global_i (after execlp) FAIL: gdb.base/foll-ex

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

dblaikie added a comment. In D91734#2443231 , @probinson wrote: > In D91734#2432988 , @dblaikie wrote: > >> Yeah, it boils down to something like this: >> >> void f1(const char*, const char*) { } >> int main() {

[Lldb-commits] [PATCH] D94063: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms

dblaikie created this revision. dblaikie added a reviewer: labath. dblaikie requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: lldb-commits, sstefan1. Herald added a project: LLDB. gcc already produces debug info with this form -freorder-block-and-part

[Lldb-commits] [PATCH] D94064: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms

dblaikie created this revision. dblaikie added a reviewer: labath. dblaikie requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Finishing out the support (to the best of my knowledge/based on current testing running the whole check-lldb with a

[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

dblaikie added inline comments. Comment at: lldb/include/lldb/Core/ModuleList.h:70 +class ModuleIterator +: public std::iterator { +public: FWIW, std::iterator is deprecated since C++17 - probably best not to add new uses of it. (I think the idea is that the

[Lldb-commits] [PATCH] D94136: [lldb] Make ModuleList iterable (NFC)

dblaikie added inline comments. Comment at: lldb/include/lldb/Core/ModuleList.h:71 +: public llvm::iterator_facade_base< + ModuleIterator, std::bidirectional_iterator_tag, lldb::ModuleSP> { +public: On the fence, but this could be a random access ite

<    1   2   3   >