[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. In D55142#1320447 , @lemo wrote: > I agree with both comments. The intention is to add some tests but I wanted > to get the review out early to surface concerns, if any. I also needed more > time to investigate a few complex failu

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath created this revision. labath added reviewers: clayborg, zturner, lemo. Herald added subscribers: arichardson, emaste. Herald added a reviewer: espindola. Object files generally have a "base" address, which is their preferred address to be loaded into memory (in the sense that if they are l

[Lldb-commits] [PATCH] D55361: Move Broadcaster+Listener+Event combo from Core into Utility

labath created this revision. labath added reviewers: zturner, jingham, teemperor, clayborg. Herald added a subscriber: mgorny. These are general purpose "utility" classes, whose functionality is not debugger-specific in any way. As such, I believe they belong in the Utility module. This doesn't

[Lldb-commits] [PATCH] D55361: Move Broadcaster+Listener+Event combo from Core into Utility

labath updated this revision to Diff 176948. labath added a comment. That's a good point, thanks for reminding me. Re-clang-formatting the changed lines. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55361/new/ https://reviews.llvm.org/D55361 Files: include/lldb/Breakpoint/Breakpoin

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

labath added a comment. This looks fine to me. I don't know much about frameworks, but I think it makes things cleaner by grouping all the framework-related code together. It's not introduced in this patch, but the part that struck me as a hack has the force-overwriting of LLVM_CODESIGNING_IDEN

[Lldb-commits] [PATCH] D55383: Implement basic DidAttach for DynamicLoaderWindowsDYLD for use with ds2

labath added a comment. I am afraid I know know much about windows, or ds2. The thing I noticed is that this only seems to set the address of the main executable. Don't you also need to set the load address of loaded shared libraries (if any)? Repository: rLLDB LLDB CHANGES SINCE LAST ACTIO

[Lldb-commits] [PATCH] D55384: [NativePDB] Reconstruct FunctionDecl AST nodes from PDB debug info

labath added a comment. In D55384#1321970 , @zturner wrote: > Clang-cl emits `S_LOCAL` symbols while MSVC emits `S_REGREL32` and > `S_REGISTER` symbols. So, to get more test coverage, I added an MSVC test as > well. I don't want to block this patch, b

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

labath created this revision. labath added reviewers: clayborg, jasonmolenda, amccarth, lemo, stella.stamenova. This function was named such because in the case of MachO files, the mach header is located at this address. However all (most?) usages of this function were not interested in that fact

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

labath marked an inline comment as done. labath added a comment. Adding a bunch of people to make sure this makes sense for all object formats (I am particularly oblivious to how COFF works). If this makes sense, I'll implement this function in ObjectFilePECOFF and ELF to return the "image base"

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath planned changes to this revision. labath added a comment. In D55356#1321863 , @clayborg wrote: > This is already available with: > > virtual lldb_private::Address ObjectFile::GetHeaderAddress(); > > > It return a lldb_private::Address which is se

[Lldb-commits] [PATCH] D55430: build.py: Implement "gcc" builder

labath created this revision. labath added a reviewer: zturner. Herald added subscribers: dexonsmith, mehdi_amini. This implements the gcc builder in build.py script to allow it to compile host executables when running on a non-windows host. Where it made sense, I tried to share code with the msvc

[Lldb-commits] [PATCH] D55214: Introduce ObjectFileBreakpad

This revision was automatically updated to reflect the committed changes. labath marked an inline comment as done. Closed by commit rL348592: Introduce ObjectFileBreakpad (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

labath created this revision. labath added reviewers: clayborg, zturner, lemo, markmentovai, amccarth. This patch allows ObjectFileBreakpad to parse the contents of Breakpad files into sections. This sounds slightly odd at first, but in essence its not too different from how other object files han

[Lldb-commits] [PATCH] D55361: Move Broadcaster+Listener+Event combo from Core into Utility

labath updated this revision to Diff 177212. labath marked 3 inline comments as done. labath added a comment. Thanks for the review. Yes the changes were done by clang-format, which even in the diff-only mode doesn't know the difference between a moved file and a newly created one. I hope this ve

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

labath added a comment. Printing the warning would definitely be nice. Making the identity overridable via llvm_codesign arguments also sounds reasonable if we want to have the flexibility of signing things with different identities. If we don't need that flexibility, can't we just have the us

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

labath added a comment. I guess I should elaborate more on the direction where I am going with this. I am trying to model these breakpad files as a debug-info-only object file, like something you would get by running say `strip --only-keep-debug`. This object file will contain a bunch of sectio

[Lldb-commits] [PATCH] D55328: [CMake] Revised LLDB.framework builds

labath added inline comments. Comment at: cmake/modules/LLDBConfig.cmake:64 + set(LLVM_CODESIGNING_IDENTITY lldb_codesign CACHE STRING "" FORCE) +endif() + sgraenitz wrote: > sgraenitz wrote: > > @labath Could this be a way to phase out LLDB_CODESIGN_IDENTITY an

[Lldb-commits] [PATCH] D55013: [CMake] Streamline code signing for debugserver #2

labath accepted this revision. labath added a comment. This looks much better. Thanks. Comment at: tools/debugserver/source/CMakeLists.txt:104 +if(LLDB_CODESIGN_IDENTITY) + set(LLDB_CODESIGN_IDENTITY_USED ${LLDB_CODESIGN_IDENTITY} CACHE STRING "" FORCE) +elseif(LLVM_CODESIGNI

[Lldb-commits] [PATCH] D55472: Speadup memory regions handling and cleanup related code

labath added a comment. Woohoo for speedups. However, I noticed that you are modifying the signatures of SB functions, which I don't believe will be acceptable for lldb. Comment at: include/lldb/API/SBMemoryRegionInfo.h:105 - SBMemoryRegionInfo(const lldb_private::MemoryRe

[Lldb-commits] [PATCH] D55384: [NativePDB] Reconstruct FunctionDecl AST nodes from PDB debug info

labath added a comment. In D55384#1323464 , @zturner wrote: > I considered this, and we actually have a test that does (see > `s_constant.s`). The reason I didn't do it here because my experience > writing `s_constant.s` taught me just how difficult it

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

labath added a comment. In D55434#1323912 , @clayborg wrote: > If you plan on not making the breakpad file ever stand alone, then you will > need to take any addresses and look them up in the module section list and > use the other sections. I don't see

[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

labath added a comment. This patch certainly provides a more comprehensive treatment of memory region information in minidump. However, there might be some value in removing the excessive shared_ptrs in the other patch. I think that should be evaluated separately. I was going to write some add

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

labath added a comment. In D55434#1325782 , @clayborg wrote: > Ok. Check out my changes that parse region info: > https://reviews.llvm.org/D55522 > > It parses the memory region info from the linux maps info if it is available. > In breakpad generated mi

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. This looks fine to me. The test is a bit more coarse-grained than I'd like, though I can't think of a substantially better approach right now. One of the pdb folks should ok this too. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:386

[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

labath added a comment. It just occurred to me that we have another copy of /proc//maps -> MemoryRegionInfo conversion code. It lives in NativeProcessLinux.cpp (ParseMemoryRegionInfoFromProcMapsLine). It would be nice to extract this parser into a some place (`Plugins/Process/Utility` ?), where

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

labath marked 2 inline comments as done. labath added inline comments. Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1677 // Check to see if the module was read from memory? - if (module_sp->GetObjectFile()->GetHeaderAddress().IsValid()) { + if

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

This revision was automatically updated to reflect the committed changes. Closed by commit rL348849: Rename ObjectFile::GetHeaderAddress to GetBaseAddress (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55422?vs=17

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath updated this revision to Diff 177717. labath added a comment. Rebase after comitting D55422 . Now this patch just implements GetBaseAddress for ELF and PECOFF object files, and adds tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55356/new/

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath added a comment. Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print the

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath added a comment. In D55356#1327224 , @clayborg wrote: > In D55356#1327099 , @labath wrote: > > > Actually, this now causes an lldb-mi test to fail, but it's not clear to me > > if the problem is in the test,

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

labath added a comment. Btw, I also wrote something like this in D55430 . (Zachary asked me to do it, but it looks like he missed the review request. :P) I think my version is more complete, though I am missing lto support, because I couldn't be bothered to fig

[Lldb-commits] [PATCH] D55430: build.py: Implement "gcc" builder

labath marked 3 inline comments as done. labath added inline comments. Comment at: lit/BuildScript/toolchain-clang.test:1 +RUN: %build -n --verbose --arch=32 --compiler=clang --mode=compile-and-link -o %t/foo.exe foobar.c \ +RUN:| FileCheck --check-prefix=CHECK --check-prefi

[Lldb-commits] [PATCH] D55430: build.py: Implement "gcc" builder

This revision was automatically updated to reflect the committed changes. labath marked an inline comment as done. Closed by commit rL348918: build.py: Implement "gcc" builder (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

labath added a comment. My main question is why do we need the separate `SBDebugger::RunReplay` API for this. Shouldn't the record/replay be as transparent as possible? I would expect that `SBDebugger::RunCommandInterpreter` will detect that it is running in replay mode and use the recorded inp

[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

labath added a comment. I also find the `static_cast` thingy weird. The rest of the changes seem to be towards the better (based on a pseudo-random sample), but the change is a quite big. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55584/new/ https://reviews.llvm.org/D55584 _

[Lldb-commits] [PATCH] D55575: [NativePDB] Support local variables

labath added inline comments. Comment at: lldb/lit/SymbolFile/NativePDB/local-variables.cpp:31-37 +// CHECK-NEXT:14 } +// CHECK-NEXT:15 +// CHECK-NEXT:16 int main(int argc, char **argv) { +// CHECK-NEXT: -> 17 int SomeLocal = argc * 2; +// CHECK-NEXT:18

[Lldb-commits] [PATCH] D55597: lldb-test: Improve newline handling

labath created this revision. labath added a reviewer: zturner. Previously lldb-test's LinePrinter would output the indentation spaces even on completely empty lines. This is not nice, as trailing spaces get flagged as errors in some tools/editors, and it prevents FileCheck's CHECK-EMPTY from work

[Lldb-commits] [PATCH] D55575: [NativePDB] Support local variables

labath added inline comments. Comment at: lldb/lit/SymbolFile/NativePDB/local-variables.cpp:31-37 +// CHECK-NEXT:14 } +// CHECK-NEXT:15 +// CHECK-NEXT:16 int main(int argc, char **argv) { +// CHECK-NEXT: -> 17 int SomeLocal = argc * 2; +// CHECK-NEXT:18

[Lldb-commits] [PATCH] D55430: build.py: Implement "gcc" builder

labath marked an inline comment as done. labath added inline comments. Comment at: lldb/trunk/lit/helper/build.py:630 +args.append('-nostdinc') +args.append('-static') +args.append('-c') zturner wrote: > Why do we need this? Withou

[Lldb-commits] [PATCH] D55614: Fix MinidumpParser::GetFilteredModuleList() and test it

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

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

labath added a comment. I am very sorry about this, but I am having some second thoughts about the `RunReplay` thingy. I don't think I've correctly expressed what is bothering me about it (and that's probably because I wasn't clear with myself about what is bothering me). I'll try to formulate

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:4246 if (symbol_file) { -ObjectFile *object_file = symbol_file->GetObjectFile(); lemo wrote: > note I had to bypass this check: we don't (yet) have a Object

[Lldb-commits] [PATCH] D55575: [NativePDB] Support local variables

labath added a comment. The test looks really good now. This looks fine to me, though I don't know much about PBDs. I still think the seemingly random change in `BinaryStreamArray` would be better served as a standalone patch. Comment at: lldb/lit/SymbolFile/NativePDB/Inputs

[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good to me. Thanks for moving the function to a more generic place. I can get rid of the copy in NativeProcessLinux once this lands. Comment at: source/Plugins/Process

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

labath added a comment. In D55582#1330715 , @JDevlieghere wrote: > I played around with this today: > > - I'm totally convinced that we should replay by swapping out stdin with a > file from the reproducer. This makes this better overall. > - I'm not con

[Lldb-commits] [PATCH] D55706: ELF: more section creation cleanup

labath created this revision. labath added reviewers: clayborg, krytarowski. Herald added subscribers: arichardson, emaste. Herald added a reviewer: espindola. This patch attempts to move as much code as possible out of the CreateSections function to make room for future improvements there. Some o

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

labath added a comment. In D55582#1331333 , @JDevlieghere wrote: > Yes, this is how SB record/replay will work. I don't disagree with this > approach, it's just that, as I said in my earlier reply, I was hoping to get > the driver working without the ne

[Lldb-commits] [PATCH] D55727: Add "dump" command as a custom "process plugin" subcommand when ProcessMinidump is used.

labath added a comment. +1 for tests. Other than that, the code seems fairly straight-forward, though it could be brought closer to llvm style (e.g., by using more StringRefs in favour of raw c strings). Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:709-714 +

[Lldb-commits] [PATCH] D55736: build.py: inherit environment in the gcc builder

labath created this revision. labath added reviewers: stella.stamenova, zturner. This should enable the compiler to find the system linker for the link step. https://reviews.llvm.org/D55736 Files: lit/helper/build.py Index: lit/helper/build.py ===

[Lldb-commits] [PATCH] D55736: build.py: inherit environment in the gcc builder

labath added a comment. Stella, could you please try whether this solves the problem you reported in D55430 ? I think this should do the trick, but I can't be sure because (for whatever reason), things seem to work for me even without this patch. CHANGES SINCE

[Lldb-commits] [PATCH] D55706: ELF: more section creation cleanup

labath added a comment. Kamil, Joerg: most of this is fairly lldb-specific and boring. I just added you for the .symtab thingy, just in case you know of anyone using SHT_SYMTAB sections which are not called .symtab (or conversely, using the name .symtab for non-symbol-table-related things). C

[Lldb-commits] [PATCH] D55706: ELF: more section creation cleanup

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB349268: ELF: more section creation cleanup (authored by labath, committed by ). Herald added a subscriber: abidh. Changed prior to commit: https://reviews.llvm.org/D55706?vs=178231&id=178360#toc Rep

[Lldb-commits] [PATCH] D55597: lldb-test: Improve newline handling

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB349269: lldb-test: Improve newline handling (authored by labath, committed by ). Herald added a subscriber: abidh. Changed prior to commit: https://reviews.llvm.org/D55597?vs=177841&id=178362#toc Re

[Lldb-commits] [PATCH] D55757: ELF: Don't create sections for section 0

labath created this revision. labath added reviewers: clayborg, krytarowski, joerg. Herald added subscribers: arichardson, emaste. Herald added a reviewer: espindola. The first section header does not define a real section. Instead it is used for various elf extensions. This patch skips creation o

[Lldb-commits] [PATCH] D55761: lldb-test ir-memory-map: Use IntervalMap::contains

labath created this revision. labath added a reviewer: vsk. Simplify the code by using the contains implementation in IntervalMap. There is a slight change of behavior here: We now treat an allocation of size 0, as if it was size 1. This guarantees that the returned addresses will be unique, wher

[Lldb-commits] [PATCH] D55736: build.py: inherit environment in the gcc builder

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB349461: build.py: inherit environment in the gcc builder (authored by labath, committed by ). Changed prior to commit: https://reviews.llvm.org/D55736?vs=178356&id=178620#toc Repository: rLLDB LLD

[Lldb-commits] [PATCH] D55757: ELF: Don't create sections for section header index 0

This revision was automatically updated to reflect the committed changes. Closed by commit rL349498: ELF: Don't create sections for section header index 0 (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55757?vs=17

[Lldb-commits] [PATCH] D55827: Update current working directory to avoid test crashes

labath added a comment. Hm... this seems like an important issue in the `RealFileSystem` (or our usage of it), and I'm not sure it should be papered over like that. I mean, lldb is a library, and requiring every call to `chdir` in the whole process go through "our" implementation is not very fr

[Lldb-commits] [PATCH] D55837: [cmake] Make libcxx(abi) a dependency when building in-tree clang

labath added a comment. On linux libc++ is not a hard dependency for running tests (in fact, clang will not use it even if it is checked out). I guess that has something to do with the fact that libc++ is not the default c++ implementation on that platform. I think this will need to be darwin-

[Lldb-commits] [PATCH] D55854: Show the memory region name if there is one in the output of the "memory region" command

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This looks fine to me. I have one inline suggestion you can consider. Also, I've been wondering whether there is any existing minidump that you could have used for the test purposes. It doesn'

[Lldb-commits] [PATCH] D55858: noexternal 1/2: refactor testsuite spawnLldbMi args->exe+args

labath added inline comments. Comment at: packages/Python/lldbsuite/test/tools/lldb-mi/lldbmi_testcase.py:53 +if exe: +self.runCmd("-file-exec-and-symbols \"%s\"" % exe) Shouldn't you also expect the `^done` response here or something simila

[Lldb-commits] [PATCH] D55858: noexternal 1/2: refactor testsuite spawnLldbMi args->exe+args

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Ok, if that's how things worked previously, then this lgtm. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55858/new/ https://reviews.llvm.org/D55858 _

[Lldb-commits] [PATCH] D55859: noexternal 2/2: symbols.enable-external-lookup=false on all hosts (not just OSX)

labath added reviewers: aprantl, clayborg. labath added a comment. I think this fits well in the spirit in which the `enable-external-lookup` setting was introduced (i.e., to reduce test environment dependencies on mac), but the letter is somewhat questionable. The setting is currently described

[Lldb-commits] [PATCH] D55827: Update current working directory to avoid test crashes

labath added a comment. In D55827#1335207 , @JDevlieghere wrote: > In D55827#1334671 , @labath wrote: > > > Hm... this seems like an important issue in the `RealFileSystem` (or our > > usage of it), and I'm not su

[Lldb-commits] [PATCH] D55859: noexternal 2/2: symbols.enable-external-lookup=false on all hosts (not just OSX)

labath added a comment. > I have regression tested it only on Fedora 29 x86_64. I see now it may > regress OSX but I have no idea what really dsym is. dsym is a apple-specific format for external storage of debug info. I think the closest standard equivalent would be a DWP bundle, but there som

[Lldb-commits] [PATCH] D55859: noexternal 2/2: symbols.enable-external-lookup=false on all hosts (not just OSX)

labath added a comment. In D55859#1336470 , @jankratochvil wrote: > Is the patch OK for check-in now? Thanks. I'd still like the update the description of the setting controlling this. As it stands now, I don't think it accurately describes what's happ

[Lldb-commits] [PATCH] D55835: [dotest] Consider unexpected passes as failures.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks good to me. Modifying this code is not ideal, but this is a pretty small but fundamental change in how unittest works. Changing the treatment of unexpected successes anywhere else would

[Lldb-commits] [PATCH] D55991: DWARF: Fix a bug in array size computation

labath created this revision. labath added reviewers: aprantl, clayborg. Herald added a subscriber: JDevlieghere. r346165 introduced a bug, where we would fail to parse the size of an array if that size happened to match an existing die offset. The logic was: if (DWARFDIE count = die.GetReference

[Lldb-commits] [PATCH] D55859: noexternal 2/2: symbols.enable-external-lookup=false on all hosts (not just OSX)

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: source/Core/ModuleList.cpp:72 {}, "Control the use of external tools or libraries to locate symbol files. " + "Directories listed in target.de

[Lldb-commits] [PATCH] D55998: ELF: create "container" sections from PT_LOAD segments

labath created this revision. labath added reviewers: clayborg, jankratochvil, krytarowski, joerg. Herald added subscribers: arichardson, emaste. Herald added a reviewer: espindola. This is the result of the discussion in D55356 , where it was suggested as a solut

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. Some interesting ideas here. I like some of them, but I have reservations about others. I'm going to go through them one by one. In D55142#1333077 , @zturner wrote: > I thought about what my "ideal" design would look like. I'm no

[Lldb-commits] [PATCH] D55991: DWARF: Fix a bug in array size computation

labath added a comment. Hm.. I can't say I have given much thought about this beforehand, as I just copied this code from llvm. However, now that I did think about it, I believe the current implementation makes sense. Nothing about DWARFFormValue::Reference (well, except the local variable name

[Lldb-commits] [PATCH] D56068: Use reference only for DW_FORM_ref*

labath added a comment. btw, I have a patch for the same bug here: D55991 Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56068/new/ https://reviews.llvm.org/D56068 ___ lldb-commits

[Lldb-commits] [PATCH] D56014: [lldb] - Fix crash when listing the history with the key up.

labath added a comment. I guess it should be possible to trigger this from dotest tests via pexpect. I know we try to avoid it, but given that this is specifically testing terminal interaction, using it here seems appropriate. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56014/new/ h

[Lldb-commits] [PATCH] D56010: [NativePDB] Fix setting breakpoint by file and line

labath accepted this revision. labath added inline comments. Comment at: lldb/lit/SymbolFile/NativePDB/break-by-line.cpp:9-10 + +// This is a separate test from break-by-function.cpp because this test is +// sensitive to edits in the source file. + You could cons

[Lldb-commits] [PATCH] D55998: ELF: create "container" sections from PT_LOAD segments

labath added a comment. In D55998#1340711 , @joerg wrote: > In D55998#1340448 , @jankratochvil > wrote: > > > ELF standard : "//An object file > > segment contains one or

[Lldb-commits] [PATCH] D55991: DWARF: Fix a bug in array size computation

This revision was automatically updated to reflect the committed changes. Closed by commit rL350086: DWARF: Fix a bug in array size computation (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55991?vs=179254&id=179

[Lldb-commits] [PATCH] D55761: lldb-test ir-memory-map: Use IntervalMap::contains

This revision was automatically updated to reflect the committed changes. Closed by commit rL350087: lldb-test ir-memory-map: Use IntervalMap::contains (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D55761?vs=17844

[Lldb-commits] [PATCH] D55998: ELF: create "container" sections from PT_LOAD segments

labath updated this revision to Diff 179541. labath marked 4 inline comments as done. labath added a comment. Address review comments: - move SegmentID() to the cpp file - use lldb_private::Range (not lldb_utility::Range) for segment and sections ranges - use brackets in segment names (PT_LOAD[0

[Lldb-commits] [PATCH] D55998: ELF: create "container" sections from PT_LOAD segments

labath added a comment. In D55998#1341263 , @joerg wrote: > I foundamentally don't understand what you are trying to do. This is stemming from a discussion on a different patch. If you're interested in the full background you can look it up here https:

[Lldb-commits] [PATCH] D56124: PECOFF: Fix section name computation

labath created this revision. labath added reviewers: zturner, stella.stamenova. Herald added a subscriber: abidh. If a section name is exactly 8 bytes long (or has been truncated to 8 bytes), it will not contain the terminating nul character. This means reading the name as a c string will p

[Lldb-commits] [PATCH] D56129: Simplify ObjectFile::GetArchitecture

labath created this revision. labath added reviewers: clayborg, zturner. Herald added subscribers: JDevlieghere, arichardson, emaste. Herald added a reviewer: espindola. instead of returning the architecture through by-ref argument and a boolean value indicating success, we can just return the Arc

[Lldb-commits] [PATCH] D56132: Symtab: Remove one copy of symbol size computation code

labath created this revision. labath added reviewers: clayborg, jasonmolenda, tberghammer. The implementation in CalculateSymbolSizes has been made redundant in D19004 , as this patch added another copy of size computation code into InitAddressIndexes (which is ca

[Lldb-commits] [PATCH] D56124: PECOFF: Fix section name computation

labath updated this revision to Diff 179644. labath added a comment. An excellent idea. Updating to use StringRef. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56124/new/ https://reviews.llvm.org/D56124 Files: lit/Modules/PECOFF/sections-names.yaml source

[Lldb-commits] [PATCH] D56126: [NativePDB] Add basic support of methods recostruction in AST

labath added a comment. In D56126#1342114 , @zturner wrote: > > This info is contained in the Symbols stream, not in the TPI stream. As far > > as I understand, we can't find in a simple way the link between a > > LF_ONEMETHOD record and a corresponding

[Lldb-commits] [PATCH] D56170: RangeMap.h: merge RangeDataArray and RangeDataVector

labath created this revision. labath added reviewers: clayborg, teemperor, tberghammer. Herald added subscribers: JDevlieghere, mgorny. The main difference between the classes was supposed to be the fact that one is backed by llvm::SmallVector, and the other by std::vector. However, over the years

[Lldb-commits] [PATCH] D56173: Introduce SymbolFileBreakpad and use it to fill symtab

labath created this revision. labath added reviewers: clayborg, zturner, lemo, markmentovai, amccarth. Herald added a subscriber: mgorny. This commit adds the glue code necessary to integrate the SymbolFileBreakpad into the plugin system. Most of the methods are stubbed out. The only method implem

[Lldb-commits] [PATCH] D55434: ObjectFileBreakpad: Implement sections

labath added a comment. I've uploaded D56173 to demonstrate how I intend to use the sections created here. The latter patch still requires some changes I only have locally (needed to make base address available to it), but the part about handling the sections

[Lldb-commits] [PATCH] D56174: ProcessLaunchInfo: remove Debugger reference

labath created this revision. labath added reviewers: zturner, jingham, teemperor. The Debuffer object was being used in "GetListenerForProcess" to provide a default listener object if one was not specified in the launch_info object. Since all the callers of this function immediately passed the r

[Lldb-commits] [PATCH] D56173: Introduce SymbolFileBreakpad and use it to fill symtab

labath updated this revision to Diff 179819. labath added a comment. Add some "image lookup" commands to the test. The test now goes through lldb instead of lldb-test, but I've kept the lldb-test changes, as I think they make sense regardless. The tricky part about the new test is the need to spec

[Lldb-commits] [PATCH] D56196: ProcessLaunchInfo: Remove Target reference

labath created this revision. labath added reviewers: zturner, jingham, teemperor. The target was being used in FinalizeFileActions to provide default values for stdin/out/err. Also, most of the logic of this function was very specific to how the lldb's Target class wants to launch processes, so I

[Lldb-commits] [PATCH] D56170: RangeMap.h: merge RangeDataArray and RangeDataVector

labath marked an inline comment as done. labath added inline comments. Comment at: unittests/Core/RangeMapTest.cpp:52-54 + // However, this does not always succeed. + // TODO: This should probably return the range (0, 40) as well. + EXPECT_THAT(Map.FindEntryThatContains(35), n

[Lldb-commits] [PATCH] D56170: RangeMap.h: merge RangeDataArray and RangeDataVector

labath marked 2 inline comments as done. labath added inline comments. Comment at: include/lldb/Core/RangeMap.h:636 -template class RangeDataArray { +template +class RangeDataVector { tberghammer wrote: > Would it make sense to have a default value of 0 for

[Lldb-commits] [PATCH] D56208: Add file-based synchronization to flaky test

labath added inline comments. Comment at: lldb/trunk/packages/Python/lldbsuite/test/macosx/queues/TestQueues.py:132 self.fail("Failed to stop at breakpoint 1.") +self.await_token(exe) Will this actually help? It could be I'm missing someth

[Lldb-commits] [PATCH] D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv

labath added a reviewer: labath. labath added a comment. Forgive my ignorance, but what makes getenv unportable? llvm uses in a bunch of places so it can't be that bad... Is it some wide string thingy? Regardless, using the lldb function for that seems fine, but if I remember correctly, each Ge

[Lldb-commits] [PATCH] D56231: [lldb-server] Improve support on Windows

labath added a reviewer: labath. labath added a comment. Nice to see some action on the lldb-server for windows front. It looks like it should be easy to add a unit test for the File::Write bug you fixed. Can you add something like that? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION ht

[Lldb-commits] [PATCH] D56232: [lldb-server] Add remote platform capabilities for Windows

labath added reviewers: labath, jingham. labath added a comment. All of these functions seem identical to their PlatformPOSIX counterparts. Is that right? And I seem to remember already seeing a lot of code duplication between PlatformPOSIX and PlatformWindows. It sounds to me like we should cr

[Lldb-commits] [PATCH] D56233: [lldb-server] Add initial support for building lldb-server on Windows

labath added a reviewer: labath. labath added inline comments. Comment at: include/lldb/Host/windows/PosixApi.h:79 +#define WNOHANG 1 +#define WUNTRACED 2 + I don't see `WUNTRACED` being used anywhere within lldb. Why did you need this? Comment

[Lldb-commits] [PATCH] D56237: Implement GetFileLoadAddress for the Windows process plugin

labath added a comment. I think there is something wrong here. Normally, it should be the job of the dynamic loader plugin to figure out where a module got loaded (and populate the target section load list). `Process::GetFileLoadAddress` is there to help it with this job. So, the fact that `Pro

[Lldb-commits] [PATCH] D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID()

labath added a comment. I would definitely encourage using something better than the file checksum as UUID, if at all possible. Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:139 +uuid = +UUID::fromOptionalData(llvm::ArrayRef(Resu

<    1   2   3   4   5   6   7   8   9   10   >