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

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

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

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

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

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

[Lldb-commits] [PATCH] D35740: Fix PR33875 by distinguishing between DWO and clang modules

dblaikie added a comment. (Presumably this could use a test case?) Repository: rL LLVM https://reviews.llvm.org/D35740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

dblaikie added a comment. In D154907#4487335 , @jasonmolenda wrote: > This looks good to me, thanks for digging in Caroline! Is there a naughty > compiler emitting this, or are we mis-parsing somehow? In D154907#4487523

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

dblaikie added inline comments. Comment at: lldb/source/Expression/DWARFExpression.cpp:1082-1089 void *src = (void *)stack.back().GetScalar().ULongLong(); intptr_t ptr; ::memcpy(&ptr, src, sizeof(void *)); // I can't decide whether the size ope

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

dblaikie added a subscriber: jmorse. dblaikie added a comment. Simple clang example that produces this invalid DWARF: void b(double); void c(); void e(double e) { c(); b(e); } $ clang-tot x.ii -g -c -o - -O3 | llvm-dwarfdump-tot - | grep DW_OP_deref_size

[Lldb-commits] [PATCH] D154907: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate (2nd attempt)

dblaikie added a comment. Filed https://github.com/llvm/llvm-project/issues/64093 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154907/new/ https://reviews.llvm.org/D154907 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

dblaikie added a comment. Any chance we could simplify this situation and have dwo searches use exactly the same/shared logic as source file searches? It seems like the dwarf spec intent was for that to be the case. I don't mind adding more ways to find things, but think it'd be useful if we o

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

dblaikie added a comment. In D157609#4645298 , @clayborg wrote: > In D157609#4644194 , @DavidSpickett > wrote: > >>> Any chance we could simplify this situation and have dwo searches use >>> exactly the same/sha

[Lldb-commits] [PATCH] D115313: [lldb/Target] Slide source display for artificial location at function start

dblaikie added a comment. Side note: The src_file is not to be trusted/used either - once line 0 is specified, nothing else in that line entry is valid. LLVM lets the previous line entries file persist because this reduces encoding size (by not having to switch all the fields in the line table

[Lldb-commits] [PATCH] D113604: [lldb][NFC] Format lldb/include/lldb/Symbol/Type.h

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks alright CHANGES SINCE LAST ACTION https://reviews.llvm.org/D113604/new/ https://reviews.llvm.org/D113604 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D118812: [lldb] Add a setting to skip long mangled names

dblaikie added a comment. Any chance you might want a limit on the size of the demangled name too? (might be worth considering what the most densely encoded mangled name is (ie: what's the longest name that could be produced by a 10k long mangled name? and see if that's worth having another cut

[Lldb-commits] [PATCH] D118812: [lldb] Add a setting to skip long mangled names

dblaikie added a subscriber: rsmith. dblaikie added a comment. In D118812#3291303 , @jingham wrote: > In D118812#3291109 , @dblaikie > wrote: > >> Any chance you might want a limit on the size of the demangled na

[Lldb-commits] [PATCH] D129166: [lldb] Make sure we use the libc++ from the build dir

dblaikie added a comment. > The overall logic and the include and library paths make sense to me. The > rpath thingy will not work on windows as it (afaik) has no equivalent feature > (it has the equivalent of (DY)LD_LIBRARY_PATH though). Any idea what the libc++ tests do on Windows then? (on l

[Lldb-commits] [PATCH] D123580: [libc++] Use bit field for checking if string is in long or short mode

dblaikie added inline comments. Comment at: libcxx/utils/gdb/libcxx/printers.py:192 class StdStringPrinter(object): """Print a std::string.""" philnik wrote: > jgorbe wrote: > > Mordante wrote: > > > philnik wrote: > > > > Mordante wrote: > > > > > philni

[Lldb-commits] [PATCH] D124370: [lldb] Fix PR54761

dblaikie added a comment. I take it no tests fail upon removing this condition? Any hints in version history about its purpose? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124370/new/ https://reviews.llvm.org/D124370 ___

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. (sorry, this slipping through somehow - it's on my radar now) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D122974/new/ https://reviews.llvm.org/D122974 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. In D122974#3424654 , @JDevlieghere wrote: > The ConstString/StringPool is pretty hot so I'm very excited about making it > faster. > > I'm slightly concerned about the two hash values (the "full" hash vs the > single byte one)

[Lldb-commits] [PATCH] D124370: [lldb/DWARF] Fix linking direction in CopyUniqueClassMethodTypes

dblaikie added a comment. In D124370#3474824 , @labath wrote: > In D124370#3472394 , @clayborg > wrote: > >> So I believe the reason we need to be able to add methods to a class is for >> templates. Templates in

[Lldb-commits] [PATCH] D122988: [BOLT][DWARF] Add version 5 split dwarf support

dblaikie added inline comments. Comment at: bolt/include/bolt/Core/DebugData.h:375-377 +if (Optional DWOId = Unit.getDWOId()) + return *DWOId; +return Unit.getOffset(); ayermolo wrote: > ayermolo wrote: > > dblaikie wrote: > > > That seems like a som

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. In D122974#3481269 , @llunak wrote: > In D122974#3480686 , @dblaikie > wrote: > >> In D122974#3424654 , @JDevlieghere >> wrote: >> >>> > > ...

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. > It doesn't make sense to require a stable hash algorithm for an internal > cache file. It's at least a stronger stability requirement than may be provided before - like: stable across process boundaries, at least, by the sounds of it? yeah? & then still raises the qu

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. In D122974#3536342 , @llunak wrote: > In D122974#3535669 , @dblaikie > wrote: > >>> It doesn't make sense to require a stable hash algorithm for an internal >>> cache file. >> >> It's a

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. >>> If the theory is that this should keep working even with the library >>> changing without LLDB rebuild, then as I wrote above that theory needs >>> double-checking first. And additionally a good question to ask would be if >>> it's really a good idea to do incompat

[Lldb-commits] [PATCH] D122974: prevent ConstString from calling djbHash() more than once

dblaikie added a comment. >>> Finally, there have been already attempts to change the hash function to >>> use the better non-zero seed (D97396 ), >>> and they were reverted because something depended on the hash function not >>> changing, so I don't see it that

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

dblaikie added a comment. Perhaps the change to use accessors could be removed, now that you've used it to find all the places that needed to be fixed up? (like just using it for cleanup/temporary purposes, without needing to commit that API change?) Comment at: llvm/include/

[Lldb-commits] [PATCH] D139649: [lldb] Make ParseTemplateParameterInfos return false if there are no template params

dblaikie added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1251 + if (has_template_params && + ParseTemplateParameterInfos(die, template_param_infos)) { +template_function_decl = m_ast.CreateFunctionD

[Lldb-commits] [PATCH] D137838: [Support] Move TargetParsers to new component

dblaikie added a comment. This has introduced a circular dependency due to the forwarding header (forwarding header depends on new lib, new lib depends on support, where the forwarding header is). Generally this wouldn't be acceptable (& I'd suggest the patch be reverted on that basis) though I

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

dblaikie added a comment. In D139379#3972876 , @ayermolo wrote: > In D139379#3972871 , @dblaikie > wrote: > >> Perhaps the change to use accessors could be removed, now that you've used >> it to find all the pla

[Lldb-commits] [PATCH] D139379: [llvm][dwwarf] Change CU/TU index to 64-bit

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. In D139379#4015624 , @ayermolo wrote: > In D139379#4015569 , @dblaikie > wrote: > >> In D139379#397287

[Lldb-commits] [PATCH] D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData.

dblaikie added inline comments. Comment at: lldb/source/API/SBSection.cpp:187-189 +DataExtractorSP result_data_sp = +std::make_shared(section_data, offset, size); +sb_data.SetOpaque(result_data_sp); Probably either use `std::move` when passing `re

[Lldb-commits] [PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

dblaikie added a comment. In D143501#4110302 , @Michael137 wrote: > The alternative would be attaching the preferred name as a new attribute or > an existing attribute like `DW_AT_description`. I'd recommend a possible long-term solution would be simpl

[Lldb-commits] [PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

dblaikie added a subscriber: probinson. dblaikie added a comment. In D143501#4116200 , @Michael137 wrote: >> I'd recommend a possible long-term solution would be simplified template >> names (so we don't have to worry about encoding this in the `DW_AT_n

[Lldb-commits] [PATCH] D143501: [clang][DebugInfo] lldb: Use preferred name's type when emitting DW_AT_names

dblaikie added a comment. > I think /maybe/ we had some design principle that DWARF features wouldn't be > solely controlled by debugger tuning, the tuning only indicates defaults but > tehy can be controlled by flags? Not sure if I'm remembering that quite > right, though - maybe @probinson re

[Lldb-commits] [PATCH] D145609: [lldb] Change dwim-print to default to disabled persistent results

dblaikie added a comment. > The rationale is: dwim-print doesn't always use expression evaluation, it > prefers to use frame variable where possible. In the future it could be > expanded, for example to print register as well. Because dwim-print doesn't > always use expression, there isn't alwa

[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

dblaikie added inline comments. Comment at: clang/test/CodeGen/preferred_name.cpp:49 + +Foo> varFooInt; + Michael137 wrote: > probinson wrote: > > This doesn't become `Foo` ? > The name stays as `Foo>` but the type of the template parameter > becomes `BarInt

[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

dblaikie added inline comments. Comment at: clang/test/CodeGen/preferred_name.cpp:49 + +Foo> varFooInt; + Michael137 wrote: > dblaikie wrote: > > Michael137 wrote: > > > probinson wrote: > > > > This doesn't become `Foo` ? > > > The name stays as `Foo>` but t

[Lldb-commits] [PATCH] D147292: [lldb] Add support for the DW_AT_trampoline attribute with a boolean

dblaikie added a comment. In D147292#4238834 , @augusto2112 wrote: > In D147292#4238820 , @JDevlieghere > wrote: > >> There seems to be overlap in the code added in this patch and D146679 >>

[Lldb-commits] [PATCH] D148776: [Modules] Move modulemaps to header search directories. NFC intended.

dblaikie added a comment. Got a link to a design discussion motivating this change? I'd have thought it made sense to put modulemaps in subdirectories - since they cover the whole directory, putting them in the root of an include path would be problematic if there are multiple distinct projects

[Lldb-commits] [PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

dblaikie added a comment. Really appreciate you providing this prototype. > This solves one of the current weaknesses in DWARF debugging, which is the > inability to set breakpoints or step onto inlined callsites. However, there > are other proposals (such as Location View Numbering) that could

[Lldb-commits] [PATCH] D152708: [RFC][Draft] Enable primitive support for Two-Level Line Tables in LLVM

dblaikie added a comment. Are there any known (or vague/unknown) limitations on the implementation with respect to the actual output/on-disk representation? (like, would doing some kind of size analysis of the output when using this patch/feature lead to incorrect conclusions about the cost of

[Lldb-commits] [PATCH] D153840: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate.

dblaikie added a comment. I'm not sure if this is the right fix - these reads are for implementing DW_OP_deref_size, by the looks of it - so I think it does make sense that the size read is not the size of the address, but the size specified in the DW_OP_deref_size. There is a requirement that

[Lldb-commits] [PATCH] D153840: [LLDB] Fix buffer overflow problem in DWARFExpression::Evaluate.

dblaikie added a comment. In D153840#4474213 , @cmtice wrote: > Hi Jason, > > I had been talking more with David, and yes, I had come to the conclusion > that you are both right and that this was not the right fix. I am planning > on reverting this, bu

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. In D109345#2986297 , @thopre wrote: > Is there no way to split this patch further? It's going to be hard finding > someone who can review something so big. If there's no way to split it in > incremental changes, you could perha

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. In D109345#2987565 , @dexonsmith wrote: > This seems like the right direction to me! Especially like the > look-through-the-ErrorInfo change for `FileError` -- I hit this before and > found it annoying. Thanks for taking a lo

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. In D109345#2990577 , @dexonsmith wrote: > In D109345#2990527 , @dblaikie > wrote: > >> (were there other regressions I mentioned/should think about?) > > I don't have specific concerns;

[Lldb-commits] [PATCH] D109345: MemoryBuffer: Migrate to Expected/llvm::Error from ErrorOr/std::error_code

dblaikie added a comment. Thanks for the suggestions/details, @dexonsmith - I've posted to llvm-dev here: https://groups.google.com/g/llvm-dev/c/m9UVRhzJvh4/m/qdd_SyPuCQAJ and will wait for some follow-up (or dead silence) before starting along probably your latter suggestion. Repository:

[Lldb-commits] [PATCH] D110571: [lldb] Add omitted abstract formal parameters in DWARF symbol files

dblaikie added a comment. >> - We could supply a test written in C, but it needs -O1 and is fairly >> sensitive to the meaning of -O1 (e.g., clang started inlining and omitting >> unsued inlined parameters only recently, so changes to -O1 could make a C >> test easily meaningless). Any concerns

[Lldb-commits] [PATCH] D110455: DebugInfo: Use clang's preferred names for integer types

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf6a561c4d675: DebugInfo: Use clang's preferred names for integer types (authored by dblaikie). Herald added subscribers: llvm-commits, lldb-commits,

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

dblaikie added a comment. So this'll add the right test dependency, if the libcxx project is enabled in the build? (& if the build hasn't enabled libcxx, the tests will run, but against the system compiler - and if that system compiler happens to default to libc++ ( https://github.com/llvm/llv

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

dblaikie added a comment. In D111981#3071378 , @teemperor wrote: > In D111981#3071349 , @dblaikie > wrote: > >> So this'll add the right test dependency, if the libcxx project is enabled >> in the build? (& if t

[Lldb-commits] [PATCH] D111981: [lldb] Fix missing dependency on libc++ from LLDB test suite on non-Darwin platforms

dblaikie added a comment. Yeah, applying that patch gets the expected `cannot open shared object file` issue for `libc++.so.1`: == FAIL: test_with_run_command_dwo (TestDataFormatterLibcxxSet.LibcxxSetDataFormatterTestCase)

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

dblaikie created this revision. dblaikie added reviewers: teemperor, labath. dblaikie requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D112163 Files: lldb/test/API/functiona

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

dblaikie added a comment. Herald added a subscriber: JDevlieghere. Looks like this test, when it was introduced/substantially refactored in D9426 - what was missing compared to all the other tests updated to run on linux, was the Makefile change to use libc++, so

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

dblaikie created this revision. dblaikie added a reviewer: aprantl. dblaikie requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Hopefully these were also fixed bi r343545 / 7fd4513920d2fed533ad420976529ef43eb42a35 Repository: rG LLVM Githu

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

dblaikie added a comment. In D112163#3077112 , @teemperor wrote: > LGTM, thanks! > > Tests with libc++ as a category are not run on Windows or when GCC is the > test compiler, so those decorators are redundant. Removing the Clang XFAIL > also seems fine

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

dblaikie added a comment. Does this sort of thing itself get tested? (like this one had a test: https://reviews.llvm.org/D111978 but not sure how much that generalizes/whether there are different parts of the infrastructure are more or less testable) Repository: rG LLVM Github Monorepo CHAN

[Lldb-commits] [PATCH] D112163: Enable libc++ in the build for libcxx initializerlist pretty printers

This revision was automatically updated to reflect the committed changes. Closed by commit rGd723ad5bcf71: Enable libc++ in the build for libcxx initializerlist pretty printers (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112

[Lldb-commits] [PATCH] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

dblaikie added a comment. Sorry I missed this - are these tested anywhere/should I have been able to discover if these needed to be changed before I made the change? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112340/new/ https://reviews.llvm.or

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

dblaikie added a comment. In D112212#3081828 , @JDevlieghere wrote: > In D112212#3080491 , @teemperor > wrote: > >> This LGTM, but `shlex.join` is actually Py3 exclusive and I don't think >> there is a good Py2

[Lldb-commits] [PATCH] D112212: [lldb/test] Print build commands in trace mode

dblaikie added a comment. In D112212#3082352 , @JDevlieghere wrote: > In D112212#3081935 , @dblaikie > wrote: > >> In D112212#3081828 , @JDevlieghere >> wrote: >> >>> I

[Lldb-commits] [PATCH] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

dblaikie added a comment. In D112340#3081540 , @teemperor wrote: > In D112340#3081532 , @dblaikie > wrote: > >> Sorry I missed this - are these tested anywhere/should I have been able to >> discover if these nee

[Lldb-commits] [PATCH] D112340: [lldb/Formatters] Remove space from vector type string summaries (NFCI)

dblaikie added a comment. In D112340#3084953 , @teemperor wrote: > Not actually sure when/how Green Dragon is mailing external people, > but usually someone just watches the build bot and emails/comments on > commits that are breaking the buildbot. Green

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

This revision was automatically updated to reflect the committed changes. Closed by commit rGe6b323379e31: Cleanup a few more PR36048 skips (authored by dblaikie). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112165/new/ https://reviews.llvm.org/D1

[Lldb-commits] [PATCH] D112863: [lldb][NFC] avoid unnecessary roundtrips between different string types

dblaikie added inline comments. Comment at: lldb/source/API/SBTarget.cpp:1589 - const ConstString csFrom(from), csTo(to); - if (!csFrom) + llvm::StringRef srFrom(from), srTo(to); + if (srFrom.empty()) Aside (since the old code did this too): Generally code

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

dblaikie added a comment. In D112165#3100608 , @teemperor wrote: > Small note: gmodules test are never run on Linux, so you actually have to run > them on macOS (or I think FreeBSD) to know whether the tests work. Yeah, I'll admit I didn't test this, bu

[Lldb-commits] [PATCH] D112165: Cleanup a few more PR36048 skips

dblaikie added a comment. In D112165#3102055 , @teemperor wrote: > In D112165#3100929 , @dblaikie > wrote: > >> In D112165#3100608 , @teemperor >> wrote: >> >>> Small no

[Lldb-commits] [PATCH] D113314: [lldb] Use std::string instead of llvm::Twine

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looks good to me. Alternatively the string concatenation could be inlined, then the Twine usage would be correct - but would still need an explicit "str()", like this: SendPacketAndWait

[Lldb-commits] [PATCH] D110578: [lldb] Add support for D programming language

dblaikie added a comment. In D110578#3123164 , @ljmf00 wrote: > @dblaikie Maybe you can land this? Nah, might need someone with more lldb context than me - I don't seem to have a clean check-lldb build right now (so don't have a good baseline to validat

[Lldb-commits] [PATCH] D113604: [lldb][NFC] Format lldb/include/lldb/Symbol/Type.h

dblaikie added inline comments. Comment at: lldb/include/lldb/Symbol/Type.h:205-206 + static bool GetTypeScopeAndBasename(const llvm::StringRef name, + llvm::StringRef scope, + llvm::StringRef basename,

[Lldb-commits] [PATCH] D113604: [lldb][NFC] Format lldb/include/lldb/Symbol/Type.h

dblaikie requested changes to this revision. dblaikie added inline comments. This revision now requires changes to proceed. Comment at: lldb/include/lldb/Symbol/Type.h:204-206 + static bool GetTypeScopeAndBasename(const llvm::StringRef name, +

[Lldb-commits] [PATCH] D126668: LLDB: Fix resolving nested template parameters

dblaikie added a comment. FWIW, clang has support for including template parameter DIEs for template declarations (-Xclang -debug-forward-template-params). We could enable that by default when tuning for lldb (or maybe we're at the tipping point and we should enable it by default in general - I

[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

dblaikie added inline comments. Herald added a subscriber: JDevlieghere. Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:230-231 -DEBUG_INFO_FLAG ?= -g +# DO NOT SUBMIT +DEBUG_INFO_FLAG ?= -g -gsimple-template-names Oh, nifty place to test

[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

dblaikie added a comment. Looking pretty good to me FWIW Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1561 +std::string +DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) { + if (!die.IsValid()) Sorry, when I gave

[Lldb-commits] [PATCH] D115324: Added the ability to cache the finalized symbol tables subsequent debug sessions to start faster.

dblaikie added a comment. Been experimenting with this recently and I noticed that loading in the cached indexes seems to do a lot of loading - specifically interning a lot of strings from the index and the symtab. Does this happen when reading a built-in index (apple_names/debug_names) (I don'

[Lldb-commits] [PATCH] D135979: [lldb][NFCish] Move DWARFDebugInfoEntry::GetQualifiedName() into DWARFASTParserClang

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Looked like this was a fairly uncontroversial part of the other patch, so I feel confident approving it - but welcome to wait for a second opinion from more lldb-affiliated developers. R

[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

dblaikie added a comment. I think the place where this will go wrong is in terms of how lldb renders `char` values on non-default-char-signedness programs (it'll render them as the default-char-signedness, which might be confusing to a user - since they'll be looking at literals, etc, that are

[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

dblaikie added a comment. In D136011#3862150 , @labath wrote: > In D136011#3860637 , @dblaikie > wrote: > >> I think the place where this will go wrong is in terms of how lldb renders >> `char` values on non-def

[Lldb-commits] [PATCH] D136011: [lldb] Don't check environment default char signedness when creating clang type for "char"

dblaikie added a comment. In D136011#3870677 , @labath wrote: > In D136011#3866854 , @aeubanks > wrote: > >> In D136011#3862150 , @labath wrote: >> >>> >> >> Maybe looki

[Lldb-commits] [PATCH] D137098: [lldb] Support simplified template names in the manual index

dblaikie added a comment. In D137098#3897289 , @aeubanks wrote: > updated description with why this doesn't produce false positives with > breakpoints > > this doesn't support regex function name lookup, not sure if we care enough > about the interactio

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

dblaikie added a subscriber: aaron.ballman. dblaikie added a comment. @aaron.ballman does this seem OK? (this was based on my suggestion in the related review linked in the description) It probably needs tests in clang too - not sure if there's an opportunity to use a unit test to simplify acce

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

dblaikie added a comment. In D137583#3917706 , @aaron.ballman wrote: >> ...we expect template params to be fully qualified when comparing them for >> simple template names > > So lldb is not inspecting the AST, they're doing reflection (of a sort) on >

[Lldb-commits] [PATCH] D137983: [lldb] Disable looking at pointee types to find synthetic value for non-ObjC

dblaikie added inline comments. Comment at: lldb/source/Core/ValueObject.cpp:2676-2677 if (!m_deref_valobj) { - if (HasSyntheticValue()) { + // FIXME: C++ stdlib formatters break with incomplete types (e.g. + // `std::vector &`). Remove ObjC restriction once t

[Lldb-commits] [PATCH] D137583: [lldb] Fix simple template names and template params with scope qualifiers

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

[Lldb-commits] [PATCH] D138539: Use std::nullopt_t instead of NoneType (NFC)

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. Sounds good to me, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138539/new/ https://reviews.llvm.org/D138539 _

[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

dblaikie added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:747-749 + if (ParseTemplateParameterInfos(die, template_param_infos) && + (!template_param_infos.args.empty() || + template_param_infos.packed_args)) { ---

[Lldb-commits] [PATCH] D138834: [lldb] Fix simple template names interaction with debug info declarations

dblaikie accepted this revision. dblaikie added a comment. This revision is now accepted and ready to land. I think this is good - welcome to wait for a second opinion if you prefer, or folks can provide feedback post-commit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION ht

[Lldb-commits] [PATCH] D138939: [WIP][clang] adds a way to provide user-oriented reasons

dblaikie added a comment. Sorry if this is derailing, but I wonder/worry about a few things here: 1. Compounding structured output with phraseology - it seems like it might be worthwhile for these to be separate issues (doesn't mean the terminal output has to say exactly the same thing - as you

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

[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

[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: > https://buildkite.com/llvm-project/libcxx-ci/builds/3530#8679608a-200b-4a48-beb4-a9e9924a88

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

[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 https://reviews.llvm.org/

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

  1   2   3   >