[Lldb-commits] [PATCH] D47492: DWARFUnit::m_die_array swap()->shrink_to_fit()

dblaikie added a comment. In https://reviews.llvm.org/D47492#1120599, @jankratochvil wrote: > FYI I have filed it for libstdc++ but I did not really understand their > reaction: Bug 86013 - std::vector::shrink_to_fit() could sometimes use > realloc()

[Lldb-commits] [PATCH] D51208: [DWARF] Fix dwarf5-index-is-used.cpp

dblaikie added a comment. Herald added a subscriber: teemperor. In https://reviews.llvm.org/D51208#1212320, @labath wrote: > As far as the strict intention of this test goes, the change is fine, as it > is meant to check that the accelerator tables get used *when* they are > generated. How do

[Lldb-commits] [PATCH] D51208: [DWARF] Fix dwarf5-index-is-used.cpp

dblaikie added a comment. >> But if LLDB has different performance characteristics, or the default should >> be different for other reasons - I'm fine with that. I think I left it on >> for Apple so as not to mess with their stuff because of the MachO/dsym sort >> of thing that's a bit

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

dblaikie added a comment. In https://reviews.llvm.org/D49411#1164680, @labath wrote: > Normally this would be clearly a good thing, but the added complication here > is that this function is part of a class hierarchy, and so this way you are > forcing every implementation to take a

[Lldb-commits] [PATCH] D49309: No longer pass a StringRef to the Python API

dblaikie added subscribers: teemperor, dblaikie. dblaikie added a comment. If the implementation of a function requires a string - it should probably accept string (not a StringRef) as a parameter - to avoid back-and-forth conversions in cases where the caller already has a string to use.

[Lldb-commits] [PATCH] D49411: Move from StringRef to std::string in the ScriptInterpreter API

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Seems good to me - though I haven't looked too closely/don't know this code terribly well (so you're welcome to wait if you know someone else more knowledgable might take a look too - or

[Lldb-commits] [PATCH] D45628: [LLDB] Support compressed debug info sections (.zdebug*)

dblaikie added a comment. In https://reviews.llvm.org/D45628#1067375, @aprantl wrote: > Which compilers / platforms generate / support this? Is this an ELF-only > feature? Clang/LLVM do (-Wa,--compress-debug-sections). Yeah, it's ELF only so far as I know. There's a couple of variations

[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

[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

[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

[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

[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

[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] 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

[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] 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

[Lldb-commits] [PATCH] D71003: [lldb/DWARF] Switch to llvm location list parser

dblaikie added inline comments. Comment at: lldb/source/Expression/DWARFExpression.cpp:72 + } + llvm_unreachable("Fully covered switch!"); +} I think usually the unreachable comment that's used in this sort of case is "Invalid LocationListFormat!" or similar,

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

dblaikie added a comment. In D70840#1765876 , @mstorsjo wrote: > In D70840#1765219 , @labath wrote: > > > debug_aranges is a sort of a lookup table for speeding up address->compile > > unit searches. llvm does not

[Lldb-commits] [PATCH] D71003: [lldb/DWARF] Switch to llvm location list parser

dblaikie added inline comments. Comment at: lldb/source/Expression/DWARFExpression.cpp:2829 +if (!loc) + LLDB_LOG_ERROR(log, loc.takeError(), "{0}"); +if (loc->Range) { labath wrote: > labath wrote: > > dblaikie wrote: > > > Does LLDB_LOG_ERROR stop

[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

[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] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

dblaikie added a subscriber: rnk. dblaikie added a comment. In D70840#1765219 , @labath wrote: > Throwing some additional dwarf people in here... > > In D70840#1763750 , @mstorsjo wrote: > > > In D70840#1763705

[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

[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] D72489: [DWARF] Emit DW_AT_call_return_pc as an address

dblaikie added a comment. I ran some size analysis (& hadn't realized that this special case was only done in the DWARFv5 codepath... that leads me to wonder about size impact in DWARFv4, so maybe I'll need to go back and run some general size analysis cost of these call sites). The

[Lldb-commits] [PATCH] D72489: [DWARF] Emit DW_AT_call_return_pc as an address

dblaikie accepted this revision. dblaikie added a comment. While the related design discussions continue - the patch itself is good/correct & there's nothing much to be done about the address pool size/relocations increase for now, for GDB at least, which is what I care about. Perhaps it's

[Lldb-commits] [PATCH] D72624: [WIP] TargetMachine Hook for Module Metadata

dblaikie added a comment. (just a general comment that this code review should be only in service of the design discussion happening on llvm-dev - please don't approve/commit this without closing out the design discussion there if there are actionable conclusions from this review)

[Lldb-commits] [PATCH] D72489: [DWARF] Emit DW_AT_call_return_pc as an address

dblaikie added a comment. Thanks for looking into this! Could you measure the size of the object files of, for example, the clang binary before/after this change - and, if possible, on Linux (where relocations are required to fixup these addresses)? I'm concerned this might increase the size

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

dblaikie added a comment. If I had to guess what this might've related to is the fact that LLVM puts a DW_AT_low_pc on the CU even if the CU uses discontiguous ranges - and in that case the low_pc has the constant value 0. So that all address values are resolved "relative" to that zero, making

[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] 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

[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

[Lldb-commits] [PATCH] D78882: [IR] Replace all uses of CallBase::getCalledValue() with getCalledOperand().

dblaikie accepted this revision. dblaikie added a comment. Looks great - thanks for the pointer type fixup(s) along the way too! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78882/new/ https://reviews.llvm.org/D78882

[Lldb-commits] [PATCH] D78489: [lldb/DWARF] Trust CU DW_AT_low/high_pc information when building address tables

dblaikie added a comment. In D78489#1993806 , @clayborg wrote: > One thing to think about that my flawed example above did show: dead stripped > functions cause problems for debug info. Both in DW_AT_ranges on the compile > unit and in all of the DWARF

[Lldb-commits] [PATCH] D79491: [lldb] Revive TestBasicEntryValuesX86_64

dblaikie added a comment. @vsk (assuming you're the original author of this test - couldn't quite figure out the revision history, sorry) - could you check if some of this could be simplified a bit to make it more clear what's being tested/what's "interesting" here? (I've provided some inline

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

dblaikie added a comment. In D79811#2036112 , @labath wrote: > Here's another interesting use of aritificial functions: inherited > constructors. > > struct A { A(int); }; > struct B:A { using A::A; }; > B b(2); > > > The constructor B::B(int) will

[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 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 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] 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

[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 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

[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

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

dblaikie added a comment. FWIW I agree that based on the current DWARF LLVM and GCC produce there's no way to allow users to default construct structs in C++11 (where non-static data member initializers were added) or above in the absence of a user-provided constructor - I don't think it'd be

[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

dblaikie added a subscriber: aprantl. dblaikie added inline comments. Comment at: lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8 + // FROM-FUNC1-NEXT: func1 + // FROM-FUNC1-SAME: [artificial] + // FROM-FUNC1-NEXT: main

[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

dblaikie added inline comments. Comment at: lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8 + // FROM-FUNC1-NEXT: func1 + // FROM-FUNC1-SAME: [artificial] + // FROM-FUNC1-NEXT: main vsk wrote: > labath wrote: > >

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

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Could you include some comparative data in the description/commit message demonstrating this generally ends up emitting all the same types for a clang build before/after (& explanations

[Lldb-commits] [PATCH] D80519: [lldb/DWARF] Add support for pre-standard GNU call site attributes

dblaikie added inline comments. Comment at: lldb/test/API/functionalities/tail_call_frames/disambiguate_paths_to_common_sink/main.cpp:8 + // FROM-FUNC1-NEXT: func1 + // FROM-FUNC1-SAME: [artificial] + // FROM-FUNC1-NEXT: main vsk wrote: > dblaikie wrote: > >

[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 >

[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 : line_table->Sequences) { +if (!list.ContainsFileAddressRange(seq.LowPC, seq.HighPC - seq.LowPC)) + continue;

[Lldb-commits] [PATCH] D78978: [LLDB] Add support for WebAssembly debugging

dblaikie added a comment. In D78978#2483327 , @paolosev wrote: > In D78978#2481358 , @vwzm228 wrote: > >> Is there any progress about such patch and D78801 >> ？ >> >> I have

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

dblaikie updated this revision to Diff 315035. dblaikie added a comment. Update test to avoid running the program Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94063/new/ https://reviews.llvm.org/D94063 Files:

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

dblaikie added a comment. In D94063#2481867 , @labath wrote: > The fix is good, but the test could be improved. Yeah - haven't written lldb patches before so totally open to suggestions on the testing front for sure. Thanks! > Combining assembly input

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

dblaikie added a comment. In D94064#2481925 , @labath wrote: > I don't think that simply setting func_lo_pc to zero will be sufficient to > make this work. I would expect this to break in more complicated scenarios > (like, even if we just have two of

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

dblaikie updated this revision to Diff 315036. dblaikie added a comment. Use image lookup to make test runnable without executing the test code Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94064/new/ https://reviews.llvm.org/D94064 Files:

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG274afac9a17f: lldb: Add support for DW_AT_ranges on DW_TAG_subprograms (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE

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

dblaikie added a comment. In D94063#2485271 , @labath wrote: > In D94063#2483546 , @dblaikie wrote: > >> If it's better to write it using C++ source and custom clang flags I can do >> that instead (it'll be an

[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 @@

[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

[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

[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:

[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

[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] 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] [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 =

[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] 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] 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

[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

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

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me with a few minor cleanups. Comment at: lldb/include/lldb/Core/ModuleList.h:73-74 +public: + explicit ModuleIterator(const ModuleList *module_list,

[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

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

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D91734#2480474 , @probinson wrote: > This version of the patch avoids the weirdness I was seeing with prolog > instructions in certain cases. >

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

dblaikie added a comment. In D91734#2482280 , @probinson wrote: > In D91734#2480930 , @dblaikie wrote: > >> I haven't looked closely, but I take it this one test modification seems >> reasonable to you? I guess

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

dblaikie added a comment. In D94064#2485222 , @labath wrote: > In D94064#2483578 , @dblaikie wrote: > >> In D94064#2481925 , @labath wrote: >> >>> I don't think that simply

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

dblaikie updated this revision to Diff 318654. dblaikie added a comment. Retrieve the lowest address in the address ranges, rather than just hardcoding 0 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94064/new/ https://reviews.llvm.org/D94064

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

dblaikie updated this revision to Diff 318711. dblaikie added a comment. Include some more details about/in the test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94064/new/ https://reviews.llvm.org/D94064 Files:

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

dblaikie added inline comments. Comment at: lldb/test/Shell/SymbolFile/DWARF/subprogram_ranges.test:22 +# initialization/no prologue instructions, so the location of "var" is valid +# at the start of the function, so 'image lookup -v -s main' will include it. +#

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG78d41a1295d9: lldb: Add support for printing variables with DW_AT_ranges on DW_TAG_subprograms (authored by dblaikie). Repository: rG LLVM Github

[Lldb-commits] [PATCH] D94888: [lldb] Add -Wl, -rpath to make tests run with fresh built libc++

dblaikie added a comment. (Works for me, FWIW - takes my local run down from 42 failures in check-lldb-api to 1 (a timeout)) Out of curiousity did you find a way to get lldb-test to print out the stdout/stderr of the underlying lldb process to see what error messages it was producing that

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

dblaikie added a comment. In D94063#2492450 , @DavidSpickett wrote: > Do you have python enabled in the build? > (https://lldb.llvm.org/resources/build.html#preliminaries) > > The cmake option LLDB_ENABLE_PYTHON defaults to Auto which has tripped me up

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

dblaikie created this revision. dblaikie added reviewers: hokein, aaron.ballman, jyknight, mehdi_amini, kuhnel. Herald added subscribers: dcaballe, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, lucyrfox, mgester, arpith-jacob,

[Lldb-commits] [PATCH] D103842: NFC: .clang-tidy: Inherit configs from parents to improve maintainability

This revision was automatically updated to reflect the committed changes. Closed by commit rGc5d56fec502f: NFC: .clang-tidy: Inherit configs from parents to improve maintainability (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

dblaikie added a comment. This might be a case of an overly reduced patch - this patch alone I'd expect to be observable in some way, and tested. If the "instring.empty()" condition never fires, then I'd expect to change that to an assert, rather than changing the semantics of it in an

[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-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

[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:

[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

[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

[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] 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"

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sure, all sounds good - if you can, please reach out to the authors of any of the semantics changing changes (the ones related to the `Changed` values in transformations) to see if they

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

dblaikie added a comment. In D101921#2786245 , @MaskRay wrote: > Because of `new MCObjectFileInfo`, we cannot use a forward declaration > (incomplete class) to replace `#include "llvm/MC/MCObjectFileInfo.h"` in > `TargetRegistry.h`. > > I thought about

[Lldb-commits] [PATCH] D100581: [Clang] -Wunused-but-set-parameter and -Wunused-but-set-variable

dblaikie added a comment. In D100581#2793775 , @ldionne wrote: > Hello! There are still some false positives, for example this one is breaking > the libc++ CI: >

[Lldb-commits] [PATCH] D102942: Remove or use variables which are unused but set.

dblaikie added a comment. In D102942#2791204 , @mbenfield wrote: > I also heard via email from Amara Emerson that the change is fine for similar > reasons. Great, thanks for checking! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

  1   2   3   >