[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR

zturner created this revision. Instead of having the Error class (which is in Utility) know about logging (which is in Core), it seems more appropriate to put all logging code together, and teach Log about errors rather than teaching Error about logs. This patch does so.

[Lldb-commits] [PATCH] D29427: Move some classes from Core -> Utility

zturner added a comment. I didn't do everything needed to get Utility to be standalone if that's what you mean. Some things are trickier than others, so I wanted to isolate this CL to strictly mechanical code move. What Host code were you referring to? BTW, I'll probably submit this tomorrow

[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR

zturner updated this revision to Diff 87036. https://reviews.llvm.org/D29514 Files: lldb/include/lldb/Core/Log.h lldb/include/lldb/Utility/Error.h lldb/source/Core/Communication.cpp lldb/source/Host/common/Host.cpp lldb/source/Plugins/OperatingSystem/Go/OperatingSystemGo.cpp

[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR

zturner added a comment. Fair enough, I can do that. https://reviews.llvm.org/D29514 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR

zturner added a comment. In https://reviews.llvm.org/D29514#666285, @labath wrote: > I'm not opposed to this patch, if people really want it, but I don't really > see the value of this macro. > Why couldn't I write > `LLDB_LOG(log, "foo: {0}", error);` > instead of > `LLDB_LOG_ERROR(log,

[Lldb-commits] [PATCH] D29510: Remove LIBLLDB_LOG_VERBOSE category

zturner added a comment. In https://reviews.llvm.org/D29510#666443, @jingham wrote: > This is sort of a side question, but Pavel's comment brought it up. If I > were new to this stuff, and wanted to know which entities had formatters and > which didn't, how would I find that out easily?

[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

zturner created this revision. Herald added a subscriber: mgorny. The goal here is to make `lldbUtility` a library which depends on no other libraries. It seems like this library already exists in spirit as a place to house very low level code which is commonly used by all parts of LLDB, so it

[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

zturner added a comment. > Move Error from Core -> Utility Also, I almost forgot. Long term: Delete and use `llvm::Error` https://reviews.llvm.org/D29359 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

zturner added inline comments. Comment at: lldb/source/Target/ThreadList.cpp:387 if (log) - log->Printf("ThreadList::%s thread 0x%4.4" PRIx64 - ": voted %s, but lost out because result was %s", - __FUNCTION__,

[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

zturner added a comment. In https://reviews.llvm.org/D29359#662417, @labath wrote: > Looks reasonable. To make sure things stay this way, we should make sure that > the Utility unit test has only this module specified as a dependency (I guess > after @beanz is done with digging through that).

[Lldb-commits] [PATCH] D29333: [CMake] Add accurate dependency specifications

zturner added a comment. Any reason this doesn't use the same strategy as LLVM? I.e. at the top of the `CMakeLists.txt` file, write something like `set(LLVM_LINK_COMPONENTS Foo Bar Baz)`, then just call `add_lldb_library` etc. https://reviews.llvm.org/D29333

[Lldb-commits] [PATCH] D29333: [CMake] Add accurate dependency specifications

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. ok, lgtm with the caveat that if it breaks anything on Windows we might have to revert until we figure it out. https://reviews.llvm.org/D29333

[Lldb-commits] [PATCH] D29333: [CMake] Add accurate dependency specifications

zturner added a comment. As long as you considered the tradeoffs then and you like this better, that's fine then. Is CMake going to complain about circular dependencies? For example, Breakpoint depends on Core and Core depends on Breakpoint. https://reviews.llvm.org/D29333

[Lldb-commits] [PATCH] D29615: Convert Log class to llvm streams

zturner added a comment. Where is the logging callback used? It seems rather odd that we would need to notify someone every time a log message was generated. Comment at: source/Core/StreamCallback.cpp:22 StreamCallback::StreamCallback(lldb::LogOutputCallback callback, void

[Lldb-commits] [PATCH] D29514: Change Error::PutToLog to LLDB_LOG_ERROR

This revision was automatically updated to reflect the committed changes. Closed by commit rL294210: Get rid of Error::PutToLog(). (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D29514?vs=87036=87260#toc Repository: rL LLVM https://reviews.llvm.org/D29514 Files:

[Lldb-commits] [PATCH] D29359: Start breaking some dependencies in lldbUtility

This revision was automatically updated to reflect the committed changes. Closed by commit rL293806: Break some dependencies in lldbUtility. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D29359?vs=86516=86693#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

zturner created this revision. Herald added a subscriber: mgorny. This depends on https://reviews.llvm.org/D30010 going in first, but assuming that's successful, this patch updates LLDB to use LLVM's memory mapping instead of `DataBufferMemoryMap`. Since this also makes `DataBufferMemoryMap`

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

zturner updated this revision to Diff 89547. zturner added a comment. Updated with suggestions from clayborg@. It seems wasteful to me check the pointer on every single call to read when we can check it once on creation. So I've added an assert in the constructor and documented with doxygen

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

zturner updated this revision to Diff 89542. https://reviews.llvm.org/D30054 Files: lldb/include/lldb/Core/DataBufferHeap.h lldb/include/lldb/Core/DataBufferLLVM.h lldb/include/lldb/Core/DataBufferMemoryMap.h lldb/include/lldb/Host/FileSpec.h lldb/source/Core/CMakeLists.txt

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

zturner added a comment. The main concern I have with adding null checks is that I think it actually *increases* your risk of crashing. All of a sudden you've introduced an entirely new branch / code-path that is completely untested. Worse, it only occurs in a situation where you've

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

This revision was automatically updated to reflect the committed changes. Closed by commit rL296159: Delete DataBufferMemoryMap. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30054?vs=89547=89701#toc Repository: rL LLVM https://reviews.llvm.org/D30054 Files:

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

zturner updated this revision to Diff 89453. zturner added a comment. Updated with suggestions from labath@ https://reviews.llvm.org/D30054 Files: lldb/include/lldb/Core/DataBufferHeap.h lldb/include/lldb/Core/DataBufferLLVM.h lldb/include/lldb/Core/DataBufferMemoryMap.h

[Lldb-commits] [PATCH] D30054: Delete DataBufferMemoryMap

zturner added inline comments. Comment at: lldb/include/lldb/Core/DataBufferLLVM.h:43 + uint8_t *GetBytes() override { +llvm_unreachable("Not implemented!"); +return nullptr; labath wrote: > This makes pretty much everything fail. Most of the code base

[Lldb-commits] [PATCH] D29909: Break some more dependencies in lldbUtility

zturner added a comment. I'm not sure, it's a bit of black magic :-/ I use git with mono-repo, and there is a bunch of magic that happens at the git-svn layer. When I run `git status` it shows me that the files are renames / moves, so I hope that means that it preserves the history when it

[Lldb-commits] [PATCH] D29909: Break some more dependencies in lldbUtility

zturner created this revision. Herald added a subscriber: mgorny. This completely removes the dependency from `lldbUtility` -> `lldbCore` and `lldbTarget`. This was done with the following restructure: 1. `ProcessStructReader`: `Utility` -> `Target` 2. `ModuleCache`: `Utility` -> `Target` 3.

[Lldb-commits] [PATCH] D29909: Break some more dependencies in lldbUtility

zturner added a comment. `git log --follow` seems to understand. Is that sufficient to guarantee that it will be retained on the SVN side? D:\src\llvm-mono>git log --follow lldb/source/Target/ModuleCache.cpp commit 07bf36c627f3a4304dbb8ea3f8347110a025bd93 Author: Zachary Turner

[Lldb-commits] [PATCH] D29909: Break some more dependencies in lldbUtility

zturner added a comment. In https://reviews.llvm.org/D29909#675644, @krytarowski wrote: > In https://reviews.llvm.org/D29909#675621, @zturner wrote: > > > `git log --follow` seems to understand. Is that sufficient to guarantee > > that it will be retained on the SVN side? > > > On SVN there is

[Lldb-commits] [PATCH] D29964: Finish breaking the dependency from lldbUtility -> Host

zturner created this revision. Herald added subscribers: mgorny, srhines, danalbert. This patch finally gets `lldbUtility` to be dependency-free. This was done through the following changes: 1. `PseudoTerminal.cpp` : `Utility` -> `Host` 2. Introduce a `vasprintf` / `vsnprintf` wrapper into

[Lldb-commits] [PATCH] D29895: Refactor log channel registration mechanism

zturner added inline comments. Comment at: include/lldb/Core/Log.h:74 + llvm::ArrayRef categories, + uint32_t default_flags, std::atomic _ptr); + static void Unregister(llvm::StringRef name); Not sure I like the idea

[Lldb-commits] [PATCH] D29964: Finish breaking the dependency from lldbUtility -> Host

zturner updated this revision to Diff 88626. https://reviews.llvm.org/D29964 Files: lldb/include/lldb/Host/PseudoTerminal.h lldb/include/lldb/Utility/PseudoTerminal.h lldb/include/lldb/Utility/VASPrintf.h lldb/source/Host/CMakeLists.txt lldb/source/Host/common/PseudoTerminal.cpp

[Lldb-commits] [PATCH] D27459: Add a more succinct logging syntax

zturner added inline comments. Comment at: source/Core/Log.cpp:78 + char *text; + vasprintf(, format, args); + message << text; dancol wrote: > I usually implement printf-into-std::string by using `vsnprintf` to figure > out how many characters we generate,

[Lldb-commits] [PATCH] D30402: Modernize Enable/DisableLogChannel interface a bit

zturner added inline comments. Comment at: include/lldb/Interpreter/Args.h:196-197 + llvm::ArrayRef GetArgumentArrayRef() const { +return {static_cast(m_argv.data()), +m_argv.size() - 1}; + } can this be written `return

[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

zturner updated this revision to Diff 80992. zturner added a comment. Added a format provider for `FileSpec`. Style syntax is documented in this patch. Added some unit tests so you can see the output. To answer Greg's earlier question about printing c-strings, `formatv("'{0}' '{1}' '{2}'",

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

This revision was automatically updated to reflect the committed changes. Closed by commit rL289853: [StringRef] Add enable-if to StringLiteral. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D27780?vs=81487=81624#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner reopened this revision. zturner added a comment. My bad, this revision was not actually closed, I attached the wrong diff to an unrelated commit. I will need to re-upload this one. Repository: rL LLVM https://reviews.llvm.org/D27780

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner removed rL LLVM as the repository for this revision. zturner updated this revision to Diff 81625. zturner added a comment. Re-upload the correct diff. https://reviews.llvm.org/D27780 Files: lldb/include/lldb/Interpreter/Options.h lldb/include/lldb/lldb-private-types.h

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner added a comment. Greg, did the comments about implicit construction of a `StringRef` from a char literal being zero overhead make sense? If so, are there any other concerns? https://reviews.llvm.org/D27780 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

This revision was automatically updated to reflect the committed changes. Closed by commit rL289922: Add methods to enable using formatv syntax in LLDB. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D27632?vs=80992=81713#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D27459: Straw-man proposal for new logging syntax

zturner added a comment. In https://reviews.llvm.org/D27459#614670, @clayborg wrote: > Part of the reason I like the current "if (log) log->Printf()" is that it > doesn't cost you much if logging isn't enabled. So if you have a log line > like: > > if (log) > log->Printf("%s %s %s",

[Lldb-commits] [PATCH] D27459: Straw-man proposal for new logging syntax

zturner added a comment. In https://reviews.llvm.org/D27459#614672, @clayborg wrote: > Does llvm::formatv allow you to specify the base of integers, or just where > they go? Can you give it width, number base, leading characters and other > things like a lot of what printf does? You can

[Lldb-commits] [PATCH] D27459: Straw-man proposal for new logging syntax

zturner added a comment. A couple comments: 1. If we're going to use `formatv` (yay!) then let's standardize. No point mising `formatv` and streaming, especially when the former will almost always be less verbose. 2. One of the concerns that came up last time was that of evaluating the

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner added a comment. In response to all the questions about "Will a StringRef constructor be called on XXX", the answer is usually yes, but the constructor will be inlined and invoke `__builtin_strlen` (which is constexpr on GCC and Clang) when used on a string literal. In other words,

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner created this revision. zturner added reviewers: clayborg, labath. zturner added a subscriber: lldb-commits. The major blocker to this until now has been that we can't have global constructors, no matter how trivial. Recently LLVM obtained the `StringLiteral` class which addresses this.

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner added inline comments. Comment at: lldb/source/Interpreter/Options.cpp:728 +for (auto : range) { + std::string full_name("--"); + full_name.append(def.long_option); clayborg wrote: > Do we still need std::string here for

[Lldb-commits] [PATCH] D27759: Fix build for mingw.

zturner added inline comments. Comment at: cmake/modules/LLDBConfig.cmake:246 +if (MSVC) +add_definitions( /D _UNICODE /D UNICODE ) +elseif (MINGW) labath wrote: > Could you check if it's enough to pass `-DFOO` regardless of the platform. > This is the only

[Lldb-commits] [PATCH] D27780: Make OptionDefinition structure store a StringRef

zturner added a comment. Yea as I mentioned this whole plan might be killed by a blocker I ran into last night. I'm still trying to figure out if there's a workaround. https://reviews.llvm.org/D27780 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

zturner added inline comments. Comment at: source/Host/common/FileSpec.cpp:1394 + assert( + (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) && + "Invalid FileSpec style!"); clayborg wrote: > If we are going to not care about case

[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

zturner added inline comments. Comment at: source/Host/common/FileSpec.cpp:1394 + assert( + (Style.empty() || Style.equals_lower("F") || Style.equals_lower("D")) && + "Invalid FileSpec style!"); clayborg wrote: > zturner wrote: > > clayborg wrote: > >

[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

zturner added a comment. Thanks. Not going to commit this just yet, as I know there was at least one concern over on llvm-dev about adopting this. So I'll give it another few days to see if the complaints remain. https://reviews.llvm.org/D27632

[Lldb-commits] [PATCH] D27632: Add Formatv() versions of all our printf style formatting functions

zturner created this revision. zturner added reviewers: labath, clayborg, jingham. zturner added a subscriber: lldb-commits. We have various functions like `Stream::Printf()`, and `Error::SetErrorStringWithFormat()`, `Log::Printf()`, and various others. I added functions that delegate to

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

zturner added a comment. Technically it's backed by the error instance, so you could return a `StringRef` as long as you said that its lifetime is tied to the lifetime of the `Error` instance. Not super unreasonable, but also not high priority. I would vote for getting rid of c strings

[Lldb-commits] [PATCH] D28519: Add format_provider for the Error class

zturner added inline comments. Comment at: include/lldb/Core/Error.h:308-309 + llvm::StringRef Options) { +llvm::format_provider::format(error.AsCString(), OS, + Options); + } clayborg

[Lldb-commits] [PATCH] D27476: Install lldb Python module on Windows.

This revision was automatically updated to reflect the committed changes. Closed by commit rL291291: Install lldb Python module on Windows. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D27476?vs=80456=83427#toc Repository: rL LLVM https://reviews.llvm.org/D27476

[Lldb-commits] [PATCH] D28028: Fix a couple of incorrect format strings

zturner added a comment. How about re-writing these to use `llvm::formatv()` since the whole point of it is to eliminate this problem entirely and clayborg@ has agreed that it looks good going forward? Individual call-site suggestions inlined. Comment at:

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

zturner added a comment. The architecture help change snuck in, I already removed it locally but didn't re-upload. As for why I'm trying to get this out of `ArchSpec`, it turns out `ArchSpec` is one of the biggest causes of dependency cycles. It's used all over the place, which triggers a

[Lldb-commits] [PATCH] D31086: Remove FileSystem::MakeDirectory

This revision was automatically updated to reflect the committed changes. Closed by commit rL298203: Remove FileSystem::MakeDirectory. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D31086?vs=92162=92259#toc Repository: rL LLVM https://reviews.llvm.org/D31086

[Lldb-commits] [PATCH] D31089: Remove FileSystem::GetFilePermissions and FileSystem::SetFilePermissions

This revision was automatically updated to reflect the committed changes. Closed by commit rL298205: Remove FileSystem::Get/SetFilePermissions (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D31089?vs=92167=92260#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D31129: Remove remaining platform specific code from FileSpec

zturner created this revision. Herald added subscribers: mgorny, srhines. This is the last step before I plan to move `FileSpec` to `Utility`, which should completely eliminate between 3 and 5 dependencies from other targets to `Host`. https://reviews.llvm.org/D31129 Files:

[Lldb-commits] [PATCH] D31129: Remove remaining platform specific code from FileSpec

zturner updated this revision to Diff 92287. zturner added a comment. Turns out `ResolveUsername` was only being called from one place outside of `FileSpec` and `ResolvePartialUsername` was dead code (since callers had already been updated to use `TildeExpressionResolver`. So I just deleted

[Lldb-commits] [PATCH] D31031: Move GetAuxvData from Host to relevant process plugins

zturner added a comment. You mention that this was the cause of dependencies from `Host` to `Target`, but I don't see any `#include "lldb/Target/*.h"` statements removed. Is this an oversight or not possible yet? https://reviews.llvm.org/D31031

[Lldb-commits] [PATCH] D30926: Fix MSVC signed/unsigned conversion and size_t conversion warnings in LLDB

This revision was automatically updated to reflect the committed changes. Closed by commit rL298099: Fix some signed/unsigned comparison warnings. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30926?vs=91670=92156#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D31079: Replace std::ofstream with llvm::raw_fd_ostream

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. In the places where you want to read from an `ifstream` and write to a socket, you might consider using `llvm::sys::fs::copy_file`, declared in `Support/FileSystem.h`. Currently it takes

[Lldb-commits] [PATCH] D30927: Normalize the LLVM cmake path before appending it to the module path

This revision was automatically updated to reflect the committed changes. Closed by commit rL298100: CMake requires normalized paths when appending. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30927?vs=91676=92157#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D31086: Remove FileSystem::MakeDirectory

zturner created this revision. Use the LLVM function instead. There are two subtle behavioral changes here which I want to make clear so someone can determine whether this matters on their platform. 1. Previously all LLDB callers were passing `eFilePermissionsDirectoryDefault` which is equal

[Lldb-commits] [PATCH] D31086: Remove FileSystem::MakeDirectory

zturner added a comment. That one is calling a file static function `MakeDirectory`, not `FileSystem::MakeDirectory`, and the implementation of that function already calls `llvm::sys::fs::create_directories()` to create the whole tree, so it should be fine. https://reviews.llvm.org/D31086

[Lldb-commits] [PATCH] D31089: Remove FileSystem::GetFilePermissions and FileSystem::SetFilePermissions

zturner added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:813 + auto perms = + static_cast(packet.GetHexMaxU32(false, UINT32_MAX)); if (packet.GetChar() == ',') { labath wrote: > This

[Lldb-commits] [PATCH] D31089: Remove FileSystem::GetFilePermissions and FileSystem::SetFilePermissions

zturner added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:813 + auto perms = + static_cast(packet.GetHexMaxU32(false, UINT32_MAX)); if (packet.GetChar() == ',') { zturner wrote: > labath

[Lldb-commits] [PATCH] D31088: Remove FileSystem::GetFilePermissions and SetFilePermissions

zturner abandoned this revision. zturner added a comment. Messed up reviewer / subscriber. https://reviews.llvm.org/D31088 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D31088: Remove FileSystem::GetFilePermissions and SetFilePermissions

zturner created this revision. Use the LLVM functions instead. https://reviews.llvm.org/D31088 Files: lldb/include/lldb/Host/FileSystem.h lldb/source/Host/posix/FileSystem.cpp lldb/source/Host/windows/FileSystem.cpp

[Lldb-commits] [PATCH] D31108: Delete LLDB code for MD5'ing a file. Use LLVM instead

zturner created this revision. https://reviews.llvm.org/D31108 Files: lldb/source/Host/common/FileSystem.cpp lldb/source/Plugins/Platform/MacOSX/PlatformDarwin.cpp lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp lldb/source/Target/Platform.cpp Index:

[Lldb-commits] [PATCH] D31111: Delete various FileSystem functions that are either dead or have direct LLVM equivalents.

zturner created this revision. Herald added a subscriber: emaste. https://reviews.llvm.org/D3 Files: lldb/include/lldb/Host/FileSystem.h lldb/source/Host/common/File.cpp lldb/source/Host/common/Host.cpp lldb/source/Host/macosx/Host.mm lldb/source/Host/posix/DomainSocket.cpp

[Lldb-commits] [PATCH] D31111: Delete various FileSystem functions that are either dead or have direct LLVM equivalents.

zturner updated this revision to Diff 92236. zturner added a comment. Forgot to remove `Stat` declaration from header file. https://reviews.llvm.org/D3 Files: lldb/include/lldb/Host/FileSystem.h lldb/source/Host/common/File.cpp lldb/source/Host/common/Host.cpp

[Lldb-commits] [PATCH] D31129: Remove remaining platform specific code from FileSpec

zturner updated this revision to Diff 92373. zturner added a comment. See what you think about this. I've created a folder called `Mocks` under `Utility`, and created a new target out of it. `UtilityTests` links against it, and so does `InterpreterTests`. To do this I had to add the lldb

[Lldb-commits] [PATCH] D31111: Delete various FileSystem functions that are either dead or have direct LLVM equivalents.

This revision was automatically updated to reflect the committed changes. Closed by commit rL298334: Delete various lldb FileSystem functions. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D3?vs=92236=92426#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D31172: Move stop info override callback code from ArchSpec into Process

zturner created this revision. The only real consumer of this is the `Process` class, so it makes sense (both conceptually and from a layering standpoint) to hide this in the Process plugin. This is intended to be NFC. https://reviews.llvm.org/D31172 Files:

[Lldb-commits] [PATCH] D31108: Delete LLDB code for MD5'ing a file. Use LLVM instead

This revision was automatically updated to reflect the committed changes. Closed by commit rL298325: Delete LLDB's MD5 code. Use LLVM instead. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D31108?vs=9=92405#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D31089: Remove FileSystem::GetFilePermissions and FileSystem::SetFilePermissions

zturner created this revision. Use the LLVM functions instead https://reviews.llvm.org/D31089 Files: lldb/include/lldb/Host/FileSystem.h lldb/source/Host/posix/FileSystem.cpp lldb/source/Host/windows/FileSystem.cpp

[Lldb-commits] [PATCH] D31129: Remove remaining platform specific code from FileSpec

This revision was automatically updated to reflect the committed changes. Closed by commit rL298465: Delete the remainder of platform specific code in FileSpec. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D31129?vs=92373=92577#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D30894: Remove lldb streams from the Log class completely

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. Looks great. Comment at: tools/lldb-server/LLDBServerUtilities.cpp:59-60 if (!success) { - fprintf(stderr, "Unable to open log file '%s' for channel \"%s\"\n", -

[Lldb-commits] [PATCH] D31823: Update LLDB Host to support IPv6 over TCP

zturner added a comment. Networking isn't my area of domain expertise, so these are mostly just general comments. Comment at: include/lldb/Host/common/TCPSocket.h:55 + + std::map m_listen_sockets; }; Any particular reason you're using a

[Lldb-commits] [PATCH] D31822: [NFC] Adding a new wrapper for getaddrinfo

zturner accepted this revision. zturner added inline comments. This revision is now accepted and ready to land. Comment at: include/lldb/Host/SocketAddress.h:44-45 + // + static std::vector

[Lldb-commits] [PATCH] D31969: [CMake] Support generating Config.h

zturner added a comment. How much would it complicate things to move the hand maintained files out of tree? If the Xcode build isn't really a thing we're officially supporting, perhaps we can take that aggressive approach in-tree as well? Comment at:

[Lldb-commits] [PATCH] D31823: Update LLDB Host to support IPv6 over TCP

zturner added inline comments. Comment at: include/lldb/Host/common/TCPSocket.h:55 + + std::map m_listen_sockets; }; jasonmolenda wrote: > zturner wrote: > > Any particular reason you're using a `std::map` instead of a `DenseMap` or > >

[Lldb-commits] [PATCH] D32148: [Utility/StringLexer] Remove dead code.

zturner accepted this revision. zturner added a comment. This revision is now accepted and ready to land. No need to request review when removing dead code. https://reviews.llvm.org/D32148 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D30807: Use LLVM's directory enumeration code

zturner created this revision. `FileSpec::EnumerateDirectory` has a bunch of platform-specific gunk in it for posix and non-posix platforms. We can get rid of all this by using LLVM's easy-to-use directory iterators. Ideally I would like to just remove this entire `EnumerateDirectory`

[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.

zturner created this revision. Herald added a subscriber: mgorny. There were a couple of problems with this function on Windows. Different separators and differences in how tilde expressions are resolved for starters, but in addition there was no clear indication of what the function's inputs

[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.

zturner added a comment. In https://reviews.llvm.org/D30789#696870, @jingham wrote: > What is the motivation behind of the "2" versions of the functions you added? Ahh yea. I can call them something else if you have a better name. But basically in a unit test I don't have an instance of a

[Lldb-commits] [PATCH] D30779: dotest.py: remove the ability to specify different architectures/compilers in a single invocation

zturner added a comment. Agree that it will be nice to see this gone. I suspect there is even more complexity than what you have here that can be removed if we aren't supporting this, but this seems like a good start. Comment at:

[Lldb-commits] [PATCH] D30560: Split DataExtractor into two classes.

This revision was automatically updated to reflect the committed changes. Closed by commit rL296910: Isolate Target-specific functionality of DataExtractor. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30560?vs=90432=90526#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D30698: Resubmit "Use LLVM for all stat related calls"

This revision was automatically updated to reflect the committed changes. Closed by commit rL297300: Resubmit FileSystem changes. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30698?vs=90905=91044#toc Repository: rL LLVM https://reviews.llvm.org/D30698 Files:

[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.

This revision was automatically updated to reflect the committed changes. Closed by commit rL297585: Make file / directory completion work properly on Windows. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30789?vs=91361=91501#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D30807: Use LLVM's directory enumeration code

This revision was automatically updated to reflect the committed changes. Closed by commit rL297598: Use LLVM for file / directory enumeration. (authored by zturner). Changed prior to commit: https://reviews.llvm.org/D30807?vs=91260=91513#toc Repository: rL LLVM

[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.

zturner added inline comments. Comment at: lldb/source/Commands/CommandCompletions.cpp:112 + if (!Resolver) +Resolver = + labath wrote: > amccarth wrote: > > This leaves the caller with no way to say the don't want a tilde resolver. > > I didn't expect

[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12

zturner added a comment. Hmm.. I thought this code was gone? Are you at tip of trunk? If so then I must be crazy. I added `llvm::set_thread_name()` which already handles all this correctly, and I thought I converted all of LLDB's clients over to using the LLVM function.

[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12

zturner added a comment. labath@ will need to comment on this. I wasn't aware that we were getting the thread name anywhere else. Im not sure if this code is even important, but hopefully it is not. The test suite is full of flakiness, but as long as it's the same before and after your

[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12

zturner added a comment. It looks like I just forgot to remove those functions. All the callers have been updated to use the llvm method. Can you just delete this method (and the one in `HostThreadFreeBSD`? https://reviews.llvm.org/D30844 ___

[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12

zturner added a comment. Oh wait, there's nothing else in those files Usually someone on the Apple team picks up the slack when changes are required to the Xcode project. If you use Xcode, you're welcome to remove them yourself. (Goes without saying, but make sure the build still

[Lldb-commits] [PATCH] D30844: pthread_setname_np first appeared in glibc 2.12

zturner added a comment. I don't think you need to delete the entire files. Just the `SetName` function. https://reviews.llvm.org/D30844 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D30918: [debugserver] This is a small cleanup patch to AVX support detection

zturner added inline comments. Comment at: tools/debugserver/source/MacOSX/x86_64/DNBArchImplX86_64.cpp:66 + int error = ::sysctlbyname(feature, , _size, NULL, 0); + return !error & answer; +} jasonmolenda wrote: > I see what you're doing -- this can either

[Lldb-commits] [PATCH] D30702: Fix remaining threading issues in Log.h

zturner accepted this revision. zturner added inline comments. Comment at: source/Utility/Log.cpp:82 + if (mask | flags) { +m_options.store(options, std::memory_order_release); +m_stream_sp = stream_sp; Might as well use `memory_order_relaxed` here.

  1   2   3   4   5   6   7   8   >