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

probinson added subscribers: wolfgangp, probinson. probinson added a comment. This is pretty different from the "always desugar to the canonical type" habit that has always been in place. Sony has done some downstream things to try to work around that in the past. @wolfgangp will remember it

[Lldb-commits] [PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

probinson added a comment. Herald added subscribers: jsetoain, JDevlieghere, ormris. Worth trying on MSVC, which is the other more likely place to fail. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140332/new/ https://reviews.llvm.org/D140332

[Lldb-commits] [PATCH] D139734: [LLDB] Remove redundant XFAIL

This revision was automatically updated to reflect the committed changes. Closed by commit rG5e6467bb3fd2: [lldb] Remove redundant XFAIL (authored by probinson). Herald added a subscriber: lldb-commits. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

probinson accepted this revision. probinson added a comment. Yes, LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131919/new/ https://reviews.llvm.org/D131919 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D137287: [Test] Fix CHECK typo.

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM, but you'll want to be ready to jump on any bot failures. That's just the nature of these kinds of changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D137287: [Test] Fix CHECK typo.

probinson added a comment. Nice work! have you verified that all the modified tests pass? naively it looks like you'd need at least 3 different targets to run them all (windows, webassembly, powerpc) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

probinson added a comment. In D131919#3749850 , @Meinersbur wrote: > Semi-OT: `polly\lib\External` has 3 more third-party libraries. Two of them > have been heavily modified in-tree, the third has just a custom > CMakeLists.txt. > Should these

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

probinson added inline comments. Comment at: clang/CMakeLists.txt:106 AND EXISTS ${UNITTEST_DIR}/CMakeLists.txt) add_subdirectory(${UNITTEST_DIR} utils/unittest) endif() Should this be `third-party/unittest` ? Looking at how the lldb

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

probinson added inline comments. Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1636 +if (Opts.getDebugInfo() == codegenoptions::DebugInfoConstructor) + Opts.setDebugInfo(codegenoptions::LimitedDebugInfo); No... you want to check both options

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

probinson added a comment. > (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 meant FullDebugInfo, and AFAICT that's still how the driver handles it?

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

probinson added a comment. > Wouldn't the current "limited" behavior have problems for this shared > libraries situation too? Sounds like in that case -fstandalone-debug should > be used. @jmorse am I remembering correctly, that we require dllimport-style annotations, so "limited" actually

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

probinson added a reviewer: jmorse. probinson added a subscriber: jmorse. probinson added a comment. + @jmorse who is better placed than I am to say whether this is what Sony would prefer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D106084/new/

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

probinson added a comment. > Currently when we have a member function that has an auto return type and the > definition is out of line we generate two DWARF DIE. > One for the declaration and a second one for the definition, the definition > holds the deduced type but does not contain a

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGc161775decdd: [FastISel] Flush local value map on every instruction (authored by probinson). Herald added a subscriber: ormris. Changed prior to commit:

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

probinson added a comment. In D91734#2482318 , @dblaikie wrote: > But, yes, we could possibly do better with more strategic is_stmt, but that's > a big/complex piece of work to tackle. Oh, absolutely. Didn't mean to imply otherwise. And the behavior

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

probinson added a comment. In D91734#2480930 , @dblaikie wrote: > I haven't looked closely, but I take it this one test modification seems > reasonable to you? I guess we're stepping into some subexpressions on a > function call that we previously

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

probinson updated this revision to Diff 314708. probinson added a comment. Change how PHIs are handled; if the operand has a debug location, use it, otherwise don't set a debug location. Then, flushLocalValueMap() will look at the first local-value instruction; if it doesn't already have a

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

probinson added a comment. This version of the patch avoids the weirdness I was seeing with prolog instructions in certain cases. The gdb test suite is largely happy with this; it avoids the new failures I mentioned previously, and one test needed to have some "next" commands updated. Here's

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

probinson added a comment. I've run this through our copy of the GDB suite (8.3, not sure how old that is). There are 10 differences, with and without the patch. FAIL: gdb.base/foll-exec.exp: step through execlp call FAIL: gdb.base/foll-exec.exp: step after execlp call FAIL:

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

probinson requested review of this revision. probinson added a comment. This is still showing as approved; I'm going to try "Request Review" to see if that helps. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91734/new/ https://reviews.llvm.org/D91734

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

probinson updated this revision to Diff 310935. probinson edited the summary of this revision. probinson added a reviewer: dblaikie. probinson added a comment. Rebase and combine cf1cc774d and dc35368c , plus revise handling

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

probinson reopened this revision. probinson added a comment. This revision is now accepted and ready to land. Reopening, will upload a new diff in a moment that modifies how PHIs are handled. I intend to run the gdb suite on this, will post findings when I have them. Repository: rG LLVM

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

probinson added a comment. In D91734#2432988 , @dblaikie wrote: > Yeah, it boils down to something like this: > > void f1(const char*, const char*) { } > int main() { > char prog[1]; > > f1(prog, >prog); > } > > Without the patch,

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

probinson added a comment. > And with these changes together, breaking on the function breaks on line 5 > (presumably because this is creating the constant used by the second '&&' > which is on line 5) instead of line 3. Not the worst thing, but I imagine > given Sony's interest in less

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

probinson added a comment. 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 run the suite myself with both patches applied and I'm basically off tomorrow, so if

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

probinson added a comment. > 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 patch it does. > (a more general fix (that would cover cases where the instruction

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

probinson added a comment. @rnk is correct that the phi's source location is propagating to the xorl. I modified the .ll file to put a different line number on the phi, and the xorl picked it up. This is clearly on the front-end to get right. If we want to get down to cases, there are other

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

probinson added a comment. Just for grins, change the `&&` to `||` and see what happens... The xorl becomes a movb $1 (no surprise there). But, that instruction no longer has line-0, instead it becomes part of the prologue. This tells me that the xorl had an explicit line 0, while the 'movb $1'

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

probinson added a comment. The constant materialization (xorl) obviously does not belong in the prologue; I think the instruction ordering with the patch is preferable. But, clearly we'd rather not have it attributed to line 0, but the line it actually belongs to. I'll start looking into this.

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGcf1c774d6ace: [FastISel] Flush local value map on ever instruction (authored by probinson). Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Changed prior to commit:

[Lldb-commits] [PATCH] D71487: [LLDB] Fix address computation for inline function

probinson added a comment. In D71487#1791824 , @clayborg wrote: > BTW: is used to be that both DW_AT_low_pc and DW_AT_high_pc would be set to > zero when a function was dead stripped. This was back when both the low and > high pc used DW_FORM_addr (a

[Lldb-commits] [PATCH] D69309: Support template instantiation in the expression evaluator

probinson added a comment. In D69309#1787994 , @friss wrote: > In D69309#1787409 , @labath wrote: > > > In D69309#1787297 , @jarin wrote: > > > > > In D69309#1752738

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

probinson added a comment. In D49466#1761156 , @MaskRay wrote: > The ugly path separator pattern `{{(/|)}}` appears in 60+ tests. Can we > teach clang and other tools to > > 1. accept both `/` and `\` input In general they do, AFAIK, although it's

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

probinson added a comment. 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 like GNU's, although AFAIK it hasn't gotten to the DWARF dumping part. Repository:

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

probinson added a comment. Pick whatever mechanism you like, we should debate it in that patch not here. :-) Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591 ___ lldb-commits

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

probinson added a comment. Ah, hadn't considered statefulness. But if you layer another class on top of DataExtractor to handle the error flag, it would have to be replicating all the offset-is-valid checks, because of course DataExtractor itself doesn't return errors. I have a couple more

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

probinson added a comment. Removing that llvm_unreachable is fine, in that case. The idea for error handling for DataExtractor sounds reasonable, looks like adding an error flag wouldn't even increase the size. Repository: rL LLVM CHANGES SINCE LAST ACTION

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

probinson added inline comments. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:220 } - return LL; + llvm_unreachable("Exit from function inside while loop!"); } dblaikie wrote: > Given the loop condition is "while (true)" this unreachable seems a bit

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

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM but give the West Coast folks a chance to look at it. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63591/new/ https://reviews.llvm.org/D63591

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

probinson added a comment. Looks pretty good, and thanks especially for the error-case tests! I'll give other folks a chance to chime in if they want to. Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:101 +if (!Data.isValidOffsetForDataOfSize(*Offset, 2 *

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

probinson added a comment. In D62634#1527575 , @dblaikie wrote: > This is intentional behavior in clang (controllable under the > -f[no-]split-dwarf-inlining flag, and modified by the > -f[no-]debug-info-for-profiling flag). > > This extra debug info is

[Lldb-commits] [PATCH] D62302: DWARF: Fix address range support in mixed 4+5 scenario

probinson added a comment. Drive-by: For the "dead code" did you check whether gcc emits DW_AT_start_scope? LLDB should support more than just what Clang emits. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62302/new/ https://reviews.llvm.org/D62302

[Lldb-commits] [PATCH] D61752: Re-enable a test for non-Windows

probinson abandoned this revision. probinson added a comment. @stella.stamenova fixed this using system-windows instead. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61752/new/ https://reviews.llvm.org/D61752

[Lldb-commits] [PATCH] D61752: Re-enable a test for non-Windows

probinson created this revision. probinson added reviewers: stella.stamenova, labath. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Required teaching lit.cfg.py to distinguish Windows the same way other projects do. Repository: rLLDB LLDB

[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

probinson added a comment. In D61611#1496865 , @stella.stamenova wrote: > In D61611#1496838 , @probinson wrote: > > > @stella.stamenova I'm not familiar with any lit feature that gives a > > special meaning to

[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

probinson added a comment. @stella.stamenova I'm not familiar with any lit feature that gives a special meaning to the prefix "no". The opposite of "REQUIRES: windows" is not "REQUIRES: nowindows" but "UNSUPPORTED: windows" AFAIK. This part of the discussion should probably be taken to

[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

probinson added a comment. In D61611#1496580 , @sgraenitz wrote: > @stella.stamenova Can you have a look at the lit test please? It works on > macOS and Linux, but I didn't test Windows. Should I add something like `# > REQUIRES: nowindows` or is it

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

probinson added a comment. I remember LLVM's DWARFContext being irritating to deal with when I was doing the DWARF 5 stuff. The exact details have been swapped out of my memory, but I suspect it had something to do with needing to know whether I was in DWO mode or normal mode in order to snag

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

probinson added a comment. In D59235#1425443 , @zturner wrote: > In D59235#1425436 , @clayborg wrote: > > > My main concern with the LLVM DWARF parser is all of the asserts in the > > code. If you attempt to use a

[Lldb-commits] [PATCH] D51935: [LLDB] - Improve reporting source lines and variables (improved DWARF5 support).

probinson added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp:442 +ReadDescriptors(debug_line_data, offset_ptr); +uint8_t dirCount = debug_line_data.GetU8(offset_ptr); +for (int i = 0; i < dirCount; ++i) {

[Lldb-commits] [PATCH] D50525: [WIP] Move lldb-mi interpreter tests to LIT

probinson added inline comments. Comment at: lit/tools/lldb-mi/interpreter/cli-support/target-list.test:5 +# We should hardcode executable name since at the moment of running of +# lldb-mi the name must be known. +# RUN: %cxx -o a.exe %p/inputs/main.cpp -g

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

probinson added inline comments. Comment at: unittests/Utility/StreamTest.cpp:38-41 +TEST_F(StreamTest, ChangingByteOrder) { + s.SetByteOrder(lldb::eByteOrderPDP); + EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder()); +} teemperor wrote: > labath wrote: > >

[Lldb-commits] [PATCH] D49192: [FileCheck] Add -allow-deprecated-dag-overlap to failing lldb tests

probinson accepted this revision. probinson added a comment. This revision is now accepted and ready to land. LGTM https://reviews.llvm.org/D49192 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

probinson added a comment. Go for it. It sounds like an abstraction that hides the multiple-section nature of the beast will particularly help LLDB; on the LLVM side the dumper will want to remain a bit more cognizant of the underlying sections. But we can surely leverage ideas from each

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

probinson added a comment. I haven't given Pavel's comments as much thought as they deserve, but here are some of mine. It's a worthwhile point that a class ought to represent some useful and reasonably well defined abstraction. DataExtractor has one, and DWARFDataExtractor builds on that

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

probinson added a comment. I can't coment on the xcode and python parts. The C++ all looks plausible. I think the "sliding" trick will be trickier in the LLVM parser, as it has to handle .o files (which might have multiple instances of .debug_types in v4, and multiple instances of .debug_info

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

probinson added a comment. So, I've been attacking the type-units-in-.debug_info problem in LLVM, and what I've done (so far) in the LLVM parser is to factor out a DWARFUnitHeader class, and made DWARFUnit derive from that. This makes it feasible to parse a unit header before deciding what

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

probinson added a comment. I am looking at making the LLVM parser handle type units across v4/v5, and the trick of pretending .debug_types is pasted onto the end of .debug_info seems like a pretty convenient fiction. It unifies DIE handling, and still allows the dumper to distinguish which

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

probinson added a comment. In https://reviews.llvm.org/D32167#1020032, @labath wrote: > Running the entire dotest test suite in -fdebug-types-section is certainly a > good way to catch problems, but it's not the way to write regression tests. Is there testing in place that is more

[Lldb-commits] [PATCH] D39896: Remove last Host usage from ArchSpec

probinson added a comment. Drive by comment: In https://reviews.llvm.org/D39896#94, @jingham wrote: > You're right, the Triple stuff is in ADT! I was using it as an example of > something you clearly wouldn't do so I didn't actually check to see if it was > done. That's pretty funny. >

[Lldb-commits] [PATCH] D32787: Fix build error: no viable conversion from returned value of type 'int' to function return type 'sigset_t' (aka '__sigset_t')

probinson added a comment. In https://reviews.llvm.org/D32787#745205, @labath wrote: > In https://reviews.llvm.org/D32787#745139, @probinson wrote: > > > In https://reviews.llvm.org/D32787#745099, @labath wrote: > > > > > Could it be that you just have stale cmake cache? > > > > > > That could

[Lldb-commits] [PATCH] D32787: Fix build error: no viable conversion from returned value of type 'int' to function return type 'sigset_t' (aka '__sigset_t')

probinson added a comment. In https://reviews.llvm.org/D32787#745099, @labath wrote: > Could it be that you just have stale cmake cache? That could easily be true. Rerunning cmake didn't fix it; short of deleting the entire build directory, is there a way to refresh the cache? Repository:

[Lldb-commits] [PATCH] D32787: Fix build error: no viable conversion from returned value of type 'int' to function return type 'sigset_t' (aka '__sigset_t')

probinson added a comment. Looking at CMakeError.log, the test program does `#include ` but does not `#define _GNU_SOURCE`. The define has to be there for your example to compile on my Ubuntu. Repository: rL LLVM https://reviews.llvm.org/D32787

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

probinson added a comment. Thanks for helping to decipher the test result! Repository: rL LLVM https://reviews.llvm.org/D27282 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

This revision was automatically updated to reflect the committed changes. Closed by commit rL288282: PR31214: Make a test tolerate "line 0" when stepping by instruction. (authored by probinson). Changed prior to commit: https://reviews.llvm.org/D27282?vs=79813=79824#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

probinson added a comment. Forgot to mention this is for PR31214. https://reviews.llvm.org/D27282 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

probinson created this revision. probinson added a reviewer: jingham. probinson added a subscriber: LLDB. LLVM is about to start emitting "line 0" records more often in the DWARF line table. One test tripped over this; made the test accommodate the new records. https://reviews.llvm.org/D27282