[Lldb-commits] [PATCH] D39515: Remove TestMyFirstWatchpoint and TestStepOverWatchpoint from basic_process category

labath created this revision. Does anyone care about these tests being in the basic_process category? The getCategories function is interfering with the filesystem-based watchpoint category kicking in. If there's any interest in keeping it, then I can certainly make it happen. However, I haven't s

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

labath added a comment. In case there isn't enough people disagreeing, I'd like to say that I also think this does not belong in FileSpec. If for nothing else, then for the FileSpec::Equal implementation: the semantics of the IsEqual method are complicated enough to follow even without having r

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

labath added subscribers: lldb-commits, labath. labath reopened this revision. labath added a comment. This revision is now accepted and ready to land. (Please add lldb-commits to future reviews, so that people are aware of what's going on.) So, the reason why this failed to compile is that andr

[Lldb-commits] [PATCH] D39515: Remove TestMyFirstWatchpoint and TestStepOverWatchpoint from basic_process category

labath added a comment. I agree that we have too many ways of specifying categories. I think that the file-based and getCategories() way are redundant, particularly, as we tend to only have one test file per directory anyway. For very fine grained annotations, we can use the `@add_test_categori

[Lldb-commits] [PATCH] D39515: Remove TestMyFirstWatchpoint and TestStepOverWatchpoint from basic_process category

labath updated this revision to Diff 121287. labath added a comment. Herald added a subscriber: ki.stfu. This removes the getCategories function altogether. I've moved the filesystem-based category computation (which was implemented as a getCategories base case) into the consumer, so that it is i

[Lldb-commits] [PATCH] D39487: Add float/vector registers for ppc64le

labath added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.cpp:534 +Status NativeRegisterContextLinux_ppc64le::ReadVMX() { + void *buf = GetVMXBuffer(); + if (!buf) Isn't this somewhat over-engineered? I mean, Ge

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

labath added inline comments. Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096 +static int readfn(void *ctx, char *buffer, int n) +{ + auto state = PyGILState_Ensure(); + auto *file = (PyObject *) ctx; + int result = -1; + auto p

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

labath added inline comments. Comment at: lldb/trunk/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1075-1096 +static int readfn(void *ctx, char *buffer, int n) +{ + auto state = PyGILState_Ensure(); + auto *file = (PyObject *) ctx; + int result = -1; + auto p

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

labath added a comment. In https://reviews.llvm.org/D38829#914300, @zturner wrote: > Ok, I wasn't aware of the libedit problem. > > I still don't like this approach, because it causes the design of the `File` > class to be influenced by a limitation of a 3rd party library. > > What about adding

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

labath added a comment. In https://reviews.llvm.org/D38829#914305, @lawrence_danna wrote: > @labath > > > Couldn't we just change the File::Read/Write functions to call these > > directly > > Like I said to @zturner , this is possible, but it can't work with the > existing APIs in SBDebugger.h.

[Lldb-commits] [PATCH] D39515: Remove TestMyFirstWatchpoint and TestStepOverWatchpoint from basic_process category

This revision was automatically updated to reflect the committed changes. Closed by commit rL317277: Remove getCategories mechanism of specifying test categories (authored by labath). Repository: rL LLVM https://reviews.llvm.org/D39515 Files: lldb/trunk/packages/Python/lldbsuite/test/exampl

[Lldb-commits] [PATCH] D56461: [NativePDB] Add support for parsing typedefs and make lldb-test work with the native reader.

labath added a comment. Is there any chance we can get someone who knows something about pdbs take a look at this (@amccarth ? @aleksandr.urakov ?) ? Comment at: lldb/lit/SymbolFile/NativePDB/typedefs.cpp:3 + +// REQUIRES: system-windows +// RUN: %build --compiler=clang-cl --n

[Lldb-commits] [PATCH] D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit

labath added a comment. Sounds good to me (not clicking accept to see if anyone else has an opinion here). CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56462/new/ https://reviews.llvm.org/D56462 ___ lldb-commits mailing list lldb-commits@

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

labath added inline comments. Comment at: lit/Minidump/Windows/Sigsegv/sigsegv.test:6 +CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 0x7ff7a13110d9 +CHECK: * frame #0: 0x7ff7a13110d9 sigsegv.exe`crash at sigsegv.cpp:22 + tee

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL350742: ELF: create "container" sections from PT_LOAD segments (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION https://revi

[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows

labath added a comment. Looks very nice. Thanks. Comment at: include/lldb/Host/windows/PipeWindows.h:52 + lldb::pipe_t GetReadPipe() const { return lldb::pipe_t(m_read); } + lldb::pipe_t GetWritePipe() const { return lldb::pipe_t(m_write); } missing `overr

[Lldb-commits] [PATCH] D56234: [lldb-server] Add unnamed pipe support to PipeWindows

labath added inline comments. Comment at: include/lldb/Host/windows/PipeWindows.h:71-74 // PipeWindows specific methods. These allow access to the underlying OS // handle. HANDLE GetReadNativeHandle(); HANDLE GetWriteNativeHandle(); Are these still n

[Lldb-commits] [PATCH] D56531: [CMake] Replace use of llvm-config with LLVM and Clang CMake packages

labath added a comment. Seems like I nice cleanup. I just have a question about the search paths. (Also, I agree with @mgorny about not doing this so close to the branch point.) Comment at: cmake/modules/LLDBStandalone.cmake:9 + find_package(LLVM REQUIRED CONFIG +HINTS "$

[Lldb-commits] [PATCH] D56315: Add a verbose mode to "image dump line-table" and use it to write a .debug_line test

This revision was automatically updated to reflect the committed changes. Closed by commit rL350802: Add a verbose mode to "image dump line-table" and use it to write a .debug_line… (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL350809: PECOFF: Fix section name computation (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D56124?vs=179644&id=181013#to

[Lldb-commits] [PATCH] D56537: ObjectFilePECOFF: Create a "container" section spanning the entire module image

labath created this revision. labath added reviewers: zturner, amccarth, stella.stamenova, clayborg. Herald added subscribers: abidh, JDevlieghere. This is coming from the discussion in D55356 (the most interesting part happened on the mailing list, so it isn't r

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

labath created this revision. labath added reviewers: clayborg, zturner, JDevlieghere. Herald added a subscriber: aprantl. If we opened a file which was produced on system with different path syntax, we would parse the paths from the debug info incorrectly. The reason for that is that we would pa

[Lldb-commits] [PATCH] D55995: [lldb] - Fix compilation with MSVS 2015 update 3

labath added a reviewer: zturner. labath added a comment. BTW, I've been compiling lldb with VS2017 and haven't run into this issue. Also, with the imminent release of VS2019, I expect someone will soon start a thread proposing to drop VS2015, so you might be better of switching to a newer comp

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

labath added a comment. Thanks for the heads-up. This should be fixed in r350834. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55434/new/ https://reviews.llvm.org/D55434 ___ lldb-commits mailing list lldb-commits@lis

[Lldb-commits] [PATCH] D56531: [CMake] Replace use of llvm-config with LLVM and Clang CMake packages

labath added inline comments. Comment at: cmake/modules/LLDBStandalone.cmake:9 + find_package(LLVM REQUIRED CONFIG +HINTS "${LLDB_PATH_TO_LLVM_BUILD}" NO_DEFAULT_PATH NO_CMAKE_FIND_ROOT_PATH) + find_package(Clang REQUIRED CONFIG xiaobai wrote: > labath wrot

lldb-commits@lists.llvm.org

labath accepted this revision. labath added a comment. looks good to me. Comment at: lldb/include/lldb/Symbol/SymbolFile.h:135 + FileSpecList &support_files) = 0; + virtual size_t ParseTypes(CompileUnit &comp_unit) = 0; + virtual bool ParseIsOp

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

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB350924: Introduce SymbolFileBreakpad and use it to fill symtab (authored by labath, committed by ). Herald added a subscriber: abidh. Changed prior to commit: https://reviews.llvm.org/D56173?vs=18024

[Lldb-commits] [PATCH] D56589: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec()

labath created this revision. labath added reviewers: clayborg, zturner, lemo. Adding a breakpad symbol file to an existing MachO module with "target symbols add" currently works only if one's host platform is a mac. This is because SymbolVendorMacOSX (which is the one responsible for loading symb

[Lldb-commits] [PATCH] D56590: breakpad: Add FUNC records to the symtab

labath created this revision. labath added reviewers: zturner, lemo, clayborg, markmentovai. Herald added a subscriber: aprantl. This patch extends SymbolFileBreakpad::AddSymbols to include the symbols from the FUNC records too. These symbols come from the debug info and have a size associated wit

[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

labath created this revision. labath added reviewers: clayborg, zturner, lemo, markmentovai. This patch teaches SymbolFileBreakpad to parse the line information in breakpad files and present it to lldb. The trickiest question here was what kind of "compile units" to present to lldb, as there real

[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

labath marked an inline comment as done. labath added inline comments. Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:150-151 +line_table_up->AppendLineEntryToSequence( +line_seq_up.get(), *next_addr, /*line*/ 0, /*column*/ 0, +/*file_id

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

labath updated this revision to Diff 181261. labath added a comment. - move the comp_dir resolution logic from SymbolFileDWARF to DWARFUnit - this required the addition on an accessor to the "comp_dir_symlinks" setting, which is used in the full comp_dir resolution - add FileSpec::MakeAbsolute()

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

labath marked 4 inline comments as done. labath added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:775-787 +void DWARFUnit::ComputePathStyle() { + m_path_style = FileSpec::Style::native; + const DWARFDebugInfoEntry *die = GetUnitDIEPtrOnly(); + if

[Lldb-commits] [PATCH] D56602: Move FileAction, ProcessInfo and ProcessLaunchInfo from Target to Host

labath created this revision. labath added reviewers: zturner, clayborg, jingham, davide, teemperor. Herald added subscribers: mgorny, emaste. These classes describe the details of the process we are about to launch, and so they are naturally used by the launching code in the Host module. Previous

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

labath added a comment. In D56230#1354829 , @Hui wrote: > I think the key problem here is to make sure the argument will be treated as > a single argument to the process launcher. Can you elaborate on that? I still don't see what is the problem with the

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

labath added a comment. In D56230#1354851 , @zturner wrote: > I almost feel like the `Args` class could be made smarter here. Because all > of the proposed solutions will still not work correctly on Linux. For > example, why is this Windows specific?

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

labath added a comment. In D56230#1355247 , @labath wrote: > For example, for a `Args` vector like `lldb-server`, `gdb-remote`, > `--log-channels=foo\\\ \\\""" '''`, `whatever`, `QuoteForCreateProcess` > would return > `lldb-server gdb-remote "--log

[Lldb-commits] [PATCH] D56609: [CMake] Remove dead code and outdated comments

labath added inline comments. Comment at: lldb/trunk/cmake/modules/LLDBStandalone.cmake:104 include("${LLVM_OBJ_ROOT}/lib${LLVM_LIBDIR_SUFFIX}/cmake/llvm/LLVMConfig.cmake") - # cmake/clang/ClangConfig.cmake is not created when LLVM and Clang are built together. if (EXIST

lldb-commits@lists.llvm.org

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/D56614/new/ https://reviews.llvm.org/D56614 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D56531: [CMake] Replace use of llvm-config with LLVM and Clang CMake packages

labath added a comment. In D56531#1355794 , @sgraenitz wrote: > In D56531#inline-500764 , > @labath wrote: > > > it should be possible to build lldb against an already-installed llvm > > > > > In D56531#inlin

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

labath added a comment. I think this problem ought to be fixed now (since D56545 ). Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55827/new/ https://reviews.llvm.org/D55827 ___ lld

[Lldb-commits] [PATCH] D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)

labath added a comment. Overall, I am pretty sure we will end up with some kind of a special `Optional` class sooner or later. However, it's not clear to me whether this is the place to start. What kind of space this is saving? I would expect that size matters when you store something as a data

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

labath added a comment. In D56230#1358102 , @Hui wrote: > What do you think of the following codes to be added in Args? > > bool Args::GetFlattenQuotedCommandString(std::string &command) const { > std::vector args_ref; > std::vector owner; > >

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

labath marked 5 inline comments as done. labath added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.h:173 + const lldb_private::FileSpec &GetCompilationDirectory(); + lldb_private::FileSpec::Style GetPathStyle(); + clayborg wrote: > Is

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

labath added a comment. In D56230#1358269 , @Hui wrote: > > - drop the `if (m_entries[i].quote)` branch. You don't need it here, and I > > don't believe it will be correct anyway, because all it will do is cause > > `llvm::sys::flattenWindowsCommandLine`

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added a comment. Overall, I think the patch is pretty good. I made a bunch of inline suggestions/questions/comments, but they're all fairly minor. From a high-level view the two comments I have are: - I am slightly concerned about the non-temporality of this approach. Lldb does a fair a

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

labath updated this revision to Diff 182010. labath added a comment. - add a test for the no-DW_AT_comp_dir + relative-DW_AT_name case CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56543/new/ https://reviews.llvm.org/D56543 Files: include/lldb/Utility/FileSpec.h lit/SymbolFile/DWAR

[Lldb-commits] [PATCH] D56543: DWARF: Add some support for non-native directory separators

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB351328: DWARF: Add some support for non-native directory separators (authored by labath, committed by ). Changed prior to commit: https://reviews.llvm.org/D56543?vs=182010&id=182011#toc Repository:

[Lldb-commits] [PATCH] D56589: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec()

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB351330: Teach the default symbol vendor to respect module.GetSymbolFileFileSpec() (authored by labath, committed by ). Changed prior to commit: https://reviews.llvm.org/D56589?vs=181243&id=182013#toc

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added inline comments. Comment at: source/Host/common/FileSystem.cpp:48 -void FileSystem::Initialize() { +llvm::Error FileSystem::Initialize() { lldbassert(!InstanceImpl() && "Already initialized."); Would it be possible to have the overloads which al

[Lldb-commits] [PATCH] D56798: Change TypeSystem::GetBitSize() to return an optional result.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. The new "error" message definitely makes more sense. The rest of the patch seems fine too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56798/new/ https://reviews.llvm.org/D56798

[Lldb-commits] [PATCH] D56814: [Reproducers] Refactor reproducer info

labath added a comment. This looks much better. LGTM, just make sure to do something with the lower_bound search, as I don't think that's right. Comment at: include/lldb/Utility/Reproducer.h:58-59 + virtual const char *GetName() const = 0; + virtual const char *GetFile() c

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added inline comments. Comment at: include/lldb/Utility/Reproducer.h:91-92 + +const char *FileInfo::name = "files"; +const char *FileInfo::file = "files.yaml"; + Are you sure this can be in a header? I would expect this to give you multiply-defined symbol

[Lldb-commits] [PATCH] D56844: Breakpad: Extract parsing code into a separate file

labath created this revision. labath added reviewers: clayborg, lemo, zturner. Herald added subscribers: fedor.sergeev, mgorny. This centralizes parsing of breakpad records, which was previously spread out over ObjectFileBreakpad and SymbolFileBreakpad. For each record type X there is a separate

[Lldb-commits] [PATCH] D56590: breakpad: Add FUNC records to the symtab

labath updated this revision to Diff 182254. labath marked 7 inline comments as done. labath added a comment. Thanks for the review. I've refactored the code to separate (and centralize) the breakpad parsing from the part which does presents the information to lldb. I've done this slightly differ

[Lldb-commits] [PATCH] D56590: breakpad: Add FUNC records to the symtab

labath added inline comments. Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:26 namespace { class LineIterator { public: clayborg wrote: > Move this functionality into llvm::breakpad::Line? I haven't moved this part to the new file becau

[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

labath planned changes to this revision. labath added a comment. I think I understand what you mean. I'll try to refactor this to create a compile unit for each function. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56595/new/ https://reviews.llvm.org/D56595 ___

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

labath added a comment. In D56230#1358356 , @zturner wrote: > I've always disliked this argument and hoped that someday someone would > remove it entirely. My recollection (which may be wrong) is that the only > actual use of it is so that if someone ty

[Lldb-commits] [PATCH] D55718: [ARC] Basic support in gdb-remote process plugin

labath added inline comments. Comment at: source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:4707 +else + // The register info is incorrect, just clear it. + m_register_info.Clear(); tatyana-krasnukha wrote: > clayborg wrote: > > Is

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

labath added a comment. In D56230#1361650 , @Hui wrote: > +#if defined(_WIN32) > +TEST(ArgsTest, GetFlattenWindowsCommandString) { > + Args args; > + args.AppendArgument("D:\\launcher.exe"); > + args.AppendArgument("--log=abc def"); > + > + std::

[Lldb-commits] [PATCH] D56618: [SymbolFile] Remove the SymbolContext parameter from FindTypes

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This makes sense to me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56618/new/ https://reviews.llvm.org/D56618 ___ lldb-commits mailin

[Lldb-commits] [PATCH] D56844: Breakpad: Extract parsing code into a separate file

This revision was automatically updated to reflect the committed changes. Closed by commit rL351541: Breakpad: Extract parsing code into a separate file (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews

[Lldb-commits] [PATCH] D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication

labath added subscribers: beanz, labath. labath added a reviewer: sgraenitz. labath added inline comments. Comment at: include/lldb/Host/Config.h.cmake:32-40 #ifndef HAVE_LIBCOMPRESSION #cmakedefine HAVE_LIBCOMPRESSION #endif +#ifndef HAVE_LIBCOMPRESSION +#ifdef __APPLE__ +#

[Lldb-commits] [PATCH] D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication

labath added inline comments. Comment at: include/lldb/Host/Config.h.cmake:32-40 #ifndef HAVE_LIBCOMPRESSION #cmakedefine HAVE_LIBCOMPRESSION #endif +#ifndef HAVE_LIBCOMPRESSION +#ifdef __APPLE__ +#define HAVE_LIBCOMPRESSION teemperor wrote: > labath wrote:

[Lldb-commits] [PATCH] D57011: Refactor HAVE_LIBCOMPRESSION and related code in GDBRemoteCommunication

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This looks much better. Thanks. (I haven't tested on a mac, but it doesn't seem like a particularly dangerous change to make). Maybe, if this sticks, https://reviews.llvm.org/D48977 could be

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

labath added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string &command) const; + I am sorry for being such a pain, but I don't think this is grammatically correct, as `get` and `flatten` are both v

[Lldb-commits] [PATCH] D56844: Breakpad: Extract parsing code into a separate file

labath marked 7 inline comments as done. labath added a comment. Thanks for the review. I'll create another review with the changes stemming from this. Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:74 + static_assert(sizeof(data) == 20, ""); +

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

labath created this revision. labath added a reviewer: lemo. This addresses the issues raised in D56844 . It removes the accessors from the breakpad record structures by making the fields public. Also, I refactor the UUID parsing code to remove hard-coded constant

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

labath added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string &command) const; + Hui wrote: > labath wrote: > > I am sorry for being such a pain, but I don't think this is grammatically > > correct

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

labath added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string &command) const; + labath wrote: > Hui wrote: > > labath wrote: > > > I am sorry for being such a pain, but I don't think this is gramma

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

labath added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string &command) const; + Hui wrote: > labath wrote: > > labath wrote: > > > Hui wrote: > > > > labath wrote: > > > > > I am sorry for being su

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

labath added inline comments. Comment at: include/lldb/Utility/Args.h:121 + bool GetFlattenWindowsCommandString(std::string &command) const; + Hui wrote: > labath wrote: > > Hui wrote: > > > labath wrote: > > > > labath wrote: > > > > > Hui wrote: > > > > > >

[Lldb-commits] [PATCH] D56590: breakpad: Add FUNC records to the symtab

This revision was automatically updated to reflect the committed changes. Closed by commit rL351781: breakpad: Add FUNC records to the symtab (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

labath updated this revision to Diff 182847. labath added a comment. rebase CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57037/new/ https://reviews.llvm.org/D57037 Files: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

labath marked 2 inline comments as done. labath added inline comments. Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:67 +/// to the endian-specific integer N. Return true on success. +template static bool consume_integer(llvm::StringRef &str, T &N) { + llvm

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

labath marked an inline comment as done. labath added inline comments. Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:87-97 // The textual module id encoding should be between 33 and 40 bytes long, // depending on the size of the age field, which is of v

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

labath marked 5 inline comments as done. labath added a comment. After some internal discussion, it seems that the situation with the all-zero UUIDs is as follows: - breakpad symbol files do not attach a special meaning to a zero UUID - if a file does not have a build-id, the dump_syms tool wil

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

This revision was automatically updated to reflect the committed changes. labath marked an inline comment as done. Closed by commit rLLDB352021: BreakpadRecords: Address post-commit feedback (authored by labath, committed by ). Changed prior to commit: https://reviews.llvm.org/D57037?vs=182847&

[Lldb-commits] [PATCH] D57195: Add UUID::SetFromOptionalStringRef, use it in DynamicLoaderDarwin

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. lgtm Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57195/new/ https://reviews.llvm.org/D57195 ___ lldb-commits

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

labath marked an inline comment as done. labath added inline comments. Comment at: lldb/trunk/source/Target/ProcessLaunchInfo.cpp:220 + if (!m_pty->OpenFirstAvailableMaster(open_flags, nullptr, 0)) { +return llvm::createStringError(llvm::inconvertibleErrorCode(), +

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

labath added a comment. I think there are some architectural issues here in how ProcessWindows works. Normally (i.e., on all non-windows process classes), it is the job of the dynamic loader plugin to create modules and set their load addresses. However, ProcessWindows does things differently,

[Lldb-commits] [PATCH] D56822: [Reproducers] Tool to insert SBReproducer macros

labath added a comment. Is this ready for review now? Comment at: tools/sbrepro/SBRepro.cpp:98 + +static std::string GetRegisterConstructorMacroMacro(StringRef Class, +StringRef Signature) { `MacroMacro`? CH

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath added a comment. I don't have any major issues with this patch. The thing I'd like to see though is one test which uses a path with `..` components and symlinks. If nothing else, it would serve as a good documentation for how these are supposed to be handled. Comment

[Lldb-commits] [PATCH] D57273: Make Type::GetByteSize optional

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Looks fine to me. I guess this would be the place I'd start thinking about an `OptionalSize` class (to replace the two fields in the Type class). But I don't think that's necessary yet... C

[Lldb-commits] [PATCH] D57275: [testsuite] Remove seven dependency

labath added a comment. +1 for keeping seven.py. Anything that moves code out of dotest.py is a good thing. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57275/new/ https://reviews.llvm.org/D57275 ___ lldb-commits

[Lldb-commits] [PATCH] D57273: Make Type::GetByteSize optional

labath added a comment. The attached patch is what it took to get the tests passing for me. It could use a bit of cleanup though. F7864332: size.diff Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57273/new/ https://reviews.ll

[Lldb-commits] [PATCH] D57330: Adjust documentation for git migration.

labath added a comment. I am not sure we should be recommending to people to place the build folder under the llvm-project checkout. Is that how people use the monorepo build nowadays? This is not an full out-of-source build, since the build folder is still a subfolder of the repo root (and wit

[Lldb-commits] [PATCH] D54617: [Reproducers] Add file provider

labath accepted this revision. labath added a comment. Looks good, thanks for bearing with me. I don't want to hold this up further, but ideally, I'd change that last test to use positive assertions instead of negative ones. E.g., right now this doesn't really fulfill my goal of documenting how

[Lldb-commits] [PATCH] D56822: [Reproducers] Tool to insert SBReproducer macros

labath added inline comments. Comment at: tools/sbrepro/SBRepro.cpp:22-31 +static std::string Join(const std::vector &V) { + std::string Str; + llvm::raw_string_ostream OS(Str); + for (std::size_t I = 0, E = V.size(); I < E; ++I) { +OS << V[I]; +if (I != E - 1) +

[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

labath updated this revision to Diff 184103. labath added a comment. It took me a while to get back to this, but I believe this version now does what we want. It creates a compile unit for each function. The compile units are created as soon as the symbol file is initialized (it's needed to get th

[Lldb-commits] [PATCH] D57402: build: remove custom variables

labath added a comment. I think this makes sense, but I don't use the standalone build, so it would be better if one of the standalone users reviewed it. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57402/new/ https://reviews.llvm.org/D57402 __

[Lldb-commits] [PATCH] D55376: Generate LLDB website/documentation from rst with Sphinx

labath accepted this revision. labath added a comment. Let's ship it. :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55376/new/ https://reviews.llvm.org/D55376 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.o

[Lldb-commits] [PATCH] D56822: [Reproducers] Tool to insert SBReproducer macros

labath added a comment. So, if I understand this correctly, a constructor like `SBFoo::SBFoo(){}` will not be considered for macroization because it's empty? That doesn't sound right to me. Even an empty constructor has a side effect of constructing the object, which is something that I would e

[Lldb-commits] [PATCH] D56822: [Reproducers] Tool to insert SBReproducer macros

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. ok, looks good then CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56822/new/ https://reviews.llvm.org/D56822 ___ lldb-commits mailing li

[Lldb-commits] [PATCH] D57475: [Reproducers] Add SBReproducer macros

labath added a comment. BTW, it would be nice to mention somewhere how to use this tool. Do you just run it, or does it needs some special arguments, or something? Comment at: source/API/SBBlock.cpp:52-58 + SB_RECORD_METHOD_CONST_NO_ARGS(bool, SBBlock, IsValid); + return m_o

[Lldb-commits] [PATCH] D57466: [lldb] Relax libc++ ABI version checking

labath added a comment. I was going to say that this has the potential of breaking libstdc++ formatters, but it looks like you found that out already. Since the libc++ inline namespace can really be anything, it makes sense to have the regex match that. Since libstdc++ (AFAIK) doesn't have that

[Lldb-commits] [PATCH] D57475: [Reproducers] Add SBReproducer macros

labath added inline comments. Comment at: source/API/SBBlock.cpp:52-58 + SB_RECORD_METHOD_CONST_NO_ARGS(bool, SBBlock, IsValid); + return m_opaque_ptr != NULL; +} bool SBBlock::IsInlined() const { + SB_RECORD_METHOD_CONST_NO_ARGS(bool, SBBlock, IsInlined); + ---

[Lldb-commits] [PATCH] D56322: [Reproducers] SBReproducer framework

labath added a comment. I have a lot of comments. The two major ones are: - i think the way you link the tests is in the UB territory. I explain this in detail in one of the inline comments. - I believe that your unit tests (not just in this patch) focus too much on testing the behavior of a si

[Lldb-commits] [PATCH] D57466: [lldb] Relax libc++ ABI version checking

labath accepted this revision. labath added a comment. Yes, I suppose it is :) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57466/new/ https://reviews.llvm.org/D57466 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.

[Lldb-commits] [PATCH] D57552: Handle "." in target.source-map in PathMapListing::FindFiles

labath added inline comments. Herald added a subscriber: lldb-commits. Comment at: packages/Python/lldbsuite/test/functionalities/source-map/TestTargetSourceMap.py:32-34 +#bp = target.BreakpointCreateByLocation(src_path, 2) +#self.assertTrue(bp.GetNumLocations()

<    10   11   12   13   14   15   16   17   18   19   >