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

zturner added a comment. In D59235#1425436 , @clayborg wrote: > My main concern with the LLVM DWARF parser is all of the asserts in the code. > If you attempt to use a DWARFDIE without first checking it for validity, it > will crash on you instead of ret

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

zturner marked an inline comment as done. zturner added a comment. It seems like we're mostly in agreement on the direction of the patch, so if there's no outstanding comments I'll submit at the end of the day. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugRanges.

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

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFFormValue.cpp:170 case DW_FORM_sec_offset: assert(m_cu); + m_value.value.uval = data.GetMaxU64(offset_ptr, 4); jankrato

[Lldb-commits] [PATCH] D59164: [SymbolFileDWARF] Move ElaboratingDIEIterator into implementation file

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB355973: Move ElaboratingDIEIterator into implementation file. (authored by zturner, committed by ). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D59164?vs=189959&id

[Lldb-commits] [PATCH] D59165: Remove DWARFDIECollection

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB355974: Remove DWARFDIECollection. (authored by zturner, committed by ). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D59165?vs=189961&id=190325#toc Repository:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB355975: Remove support for DWARF64. (authored by zturner, committed by ). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D59235?vs=190173&id=190326#toc Repository:

[Lldb-commits] [PATCH] D59276: Delete dead code

zturner created this revision. zturner added reviewers: clayborg, aprantl. Herald added subscribers: jdoerfert, mgorny. Most of these are Dump functions that are never called, but there is one instance of entire unused classes (DWARFDebugMacinfo and DWARFDebugMacinfoEntry) which are also unrefer

[Lldb-commits] [PATCH] D59276: Delete dead code

zturner added a comment. In D59276#1426972 , @aprantl wrote: > One one hand this seems fine to remove, but on the other hand — won't these > functions come in handy to compare and debug differences between the LLDB and > LLVM DWARF parsers, while we are

[Lldb-commits] [PATCH] D59370: Return llvm::Error and llvm::Expected from some DWARF parsing functions

zturner created this revision. zturner added reviewers: labath, clayborg, aprantl, probinson. Herald added a subscriber: jdoerfert. The goal here is to improve our error handling and error recovery while parsing DWARF, while at the same time getting us closer to being able to merge LLDB's DWARF

[Lldb-commits] [PATCH] D59370: Return llvm::Error and llvm::Expected from some DWARF parsing functions

zturner updated this revision to Diff 190654. zturner added a comment. Fix half-written comment and add comments to other extraction function declarations. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59370/new/ https://reviews.llvm.org/D59370 Files: lldb/source/Plugins/SymbolFile/

[Lldb-commits] [PATCH] D59291: [Object] Add basic minidump support

zturner added inline comments. Comment at: include/llvm/Object/Minidump.h:77 + ArrayRef Streams, + std::unordered_map StreamMap) + : Binary(ID_Minidump, Source), Header(Header), Streams(Streams), jhenderson wrote: > Are you delibe

[Lldb-commits] [PATCH] D59370: Return llvm::Error and llvm::Expected from some DWARF parsing functions

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB356190: Return llvm::Error and llvm::Expected from DWARF parsing code. (authored by zturner, committed by ). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D59370?vs=

[Lldb-commits] [PATCH] D59381: Change CompileUnit and ARanges interfaces to propagate errors

zturner created this revision. zturner added reviewers: clayborg, labath, aprantl. Herald added a subscriber: jdoerfert. This is a continuation of D59370 for compile units and aranges. https://reviews.llvm.org/D59381 Files: lldb/source/Plugins/SymbolFile/DWA

[Lldb-commits] [PATCH] D59381: Change CompileUnit and ARanges interfaces to propagate errors

zturner marked 2 inline comments as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:25 + lldb::offset_t *offset_ptr) { + assert(debug_info.ValidOffset(*offset_ptr)); + clayborg w

[Lldb-commits] [PATCH] D59400: [lldb-vscode] Fix dangling pointer in request_evaluate.

zturner added a comment. Another option would be to just assign it to a `std::string`, but this is fine too. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59400/new/ https://reviews.llvm.org/D59400 ___ lldb-commit

[Lldb-commits] [PATCH] D59381: Change CompileUnit and ARanges interfaces to propagate errors

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit rL356278: Return Error and Expected from more DWARF interfaces. (authored by zturner, committed by ). Herald added a proje

[Lldb-commits] [PATCH] D59430: Update DwarfDebugInfoEntry to use llvm::Error and llvm::Expected

zturner created this revision. zturner added reviewers: clayborg, aprantl, labath. Herald added a subscriber: jdoerfert. This hits the next batch of classes and adds better error handling and recovery to them. After this, the only remaining case is the line table stuff, at which point we should

[Lldb-commits] [PATCH] D59430: Update DwarfDebugInfoEntry to use llvm::Error and llvm::Expected

zturner marked 3 inline comments as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp:218 + assert(offset < cu->GetNextCompileUnitOffset() && + debug_info_data.ValidOffset(offset)); aprantl wr

[Lldb-commits] [PATCH] D59427: [lldb] [API] Split SBRegistry into smaller files

zturner added a comment. Or better yet, make it a static method on each SB class. E.g. `SBTarget::InitializeReproducerRegistry();` etc, one for each class. Random thought, but this all seems like a very high maintenance strategy, having to manually update this registry when methods are added o

[Lldb-commits] [PATCH] D59433: Fix UUID decoding from minidump files.

zturner added a comment. "MinidumpNew" is a little bit vague. What's "new" about it? Is there a way we could come up with a better name? As an aside, since there's no interactivity in these tests, they seem like a good candidate for lit/Minidump, with 1 file for each test. Something like:

[Lldb-commits] [PATCH] D59430: Update DwarfDebugInfoEntry to use llvm::Error and llvm::Expected

zturner added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59430/new/ https://reviews.llvm.org/D59430 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59276: Delete dead code

zturner added a comment. ping CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59276/new/ https://reviews.llvm.org/D59276 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D59498: [DWARF] Remove a couple of log statements

zturner created this revision. zturner added reviewers: JDevlieghere, aprantl. Herald added a subscriber: jdoerfert. I don't find these specific log statements particularly high value, as they appear more just like simple function tracing calls (i.e. they log low level implementation details, no

[Lldb-commits] [PATCH] D59498: [DWARF] Remove a couple of log statements

zturner marked 4 inline comments as done. zturner added a comment. In all cases, I think the question worth asking is not "could it be used for X", but rather "how often is it actually used for X". Otherwise, it's just technical debt IMO. There's a lot of things that are possible in theory, bu

[Lldb-commits] [PATCH] D59498: [DWARF] Remove a couple of log statements

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB356469: Remove a couple of log statements. (authored by zturner, committed by ). Herald added a subscriber: abidh. Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D594

[Lldb-commits] [PATCH] D59291: [Object] Add basic minidump support

zturner accepted this revision. zturner added a comment. This LGTM, but let's give it a day to see if anyone else chimes in with comments. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59291/new/ https://reviews.llvm.org/D59291

[Lldb-commits] [PATCH] D59276: Delete dead code

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB356490: Delete dead code. (authored by zturner, committed by ). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D59276?vs=190350&id=191356#toc Repository: rLLDB LLD

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

zturner created this revision. zturner added reviewers: JDevlieghere, aprantl, clayborg. Herald added subscribers: jdoerfert, mgorny. LLVM's DWARF parsing library has a class called `DWARFContext` which holds all of the various DWARF data sections and lots of other information. LLDB's on the ot

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

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.h:30 + + const DWARFDataExtractor *maybeLoadArangesData(); + JDevlieghere wrote: > Why do we need these two methods? I presume

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

zturner updated this revision to Diff 191421. zturner added a comment. I went ahead and just removed the const `get` version entirely, while changing the other function name to `getOrLoad` as you suggested. I have another patch while I'll upload after this one that converts all of the rest of t

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

zturner marked 5 inline comments as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:17 +static const DWARFDataExtractor *LoadOrGetSection( +Module &module, const llvm::Optional &mapped_dwarf_data, +SectionType sectio

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

zturner updated this revision to Diff 191510. zturner marked an inline comment as done. zturner added a comment. Updated based on suggestions, including removal of the `m_dwarf_data` member variable which holds the contents of the `__DWARF` segment on MachO. CHANGES SINCE LAST ACTION https://

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

zturner marked 2 inline comments as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFContext.cpp:50 + +const DWARFDataExtractor *DWARFContext::getOrLoadArangesData() { + return LoadOrGetSection(m_module, m_dwarf_data, eSectionTypeDWARFD

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

zturner marked an inline comment as done. zturner added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:552 + data.Clear(); + m_obj_file->ReadSectionData(section_sp.get(), data); } clayborg wrote: > clayborg wrote: > > ```

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL356612: Introduce DWARFContext. (authored by zturner, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D59562?vs=1

[Lldb-commits] [PATCH] D59611: Move the rest of the section loading over to DWARFContext

zturner created this revision. zturner added reviewers: clayborg, JDevlieghere, labath, aprantl. Herald added a subscriber: jdoerfert. This gets all of the remaining sections except those that are DWO-dependent. That will be a bit harder to do since we'll somehow have to get knowledge of DWO-ne

[Lldb-commits] [PATCH] D59611: Move the rest of the section loading over to DWARFContext

zturner added a comment. In D59611#1437044 , @clayborg wrote: > Use reference to DWARFContext instead of pointers? Apply to entire patch and > good to go Done. Since you conditionally LGTM'ed it based on that and I made that change I'll go ahead and s

[Lldb-commits] [PATCH] D59611: Move the rest of the section loading over to DWARFContext

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit rL356682: Move the rest of the sections over to DWARFContext. (authored by zturner, committed by ). Herald added a project

[Lldb-commits] [PATCH] D59651: Move DebugRanges() function from SymbolFileDWARF to DWARFContext

zturner created this revision. zturner added reviewers: JDevlieghere, clayborg, labath, aprantl. This should be the last non-dwo dependent piece to get over to `DWARFContext`. After this there will need to be some careful refactoring to get all knowledge of DWO-ness into the `DWARFContext`, ult

[Lldb-commits] [PATCH] D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary

zturner added a comment. In D59854#1443906 , @jingham wrote: > In D59854#1443904 , @JDevlieghere > wrote: > > > In D59854#1443891 , @jingham wrote: > > > > > It isn't safe t

[Lldb-commits] [PATCH] D59911: Don't abort() in lldb_assert and document why.

zturner added a comment. In D59911#1446942 , @jingham wrote: > In D59911#1446745 , @davide wrote: > > > My (maybe unpopolar) opinion on the subject is that "soft assertions" are a > > way to cleanse your conscience

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

zturner added inline comments. Comment at: lldb/tools/driver/Driver.cpp:492-496 #ifdef _WIN32 - _close(fds[WRITE]); - fds[WRITE] = -1; + _close(fd); #else - close(fds[WRITE]); - fds[WRITE] = -1; + close(fd); #endif labath wrote: >

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

[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", myStr

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

[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 `forma

[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}'", (c

[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] 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 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 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, the

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

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

[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&id=81624#toc Repository: rL LLVM https://reviews.llvm.org

[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/source/

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

[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&id=81713#toc Repository: rL LLVM https://reviews.

[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@list

[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: source/Interpreter/

[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&id=83427#toc Repository: rL LLVM https://reviews.llvm.org/D274

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

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

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

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

[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 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] 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] 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 http://lists.llvm.org/cgi-bin/mailm

[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__, thread_sp-

[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] 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&id=86693#toc Repository: rL LLVM https://reviews.llvm.org/D29

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

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

[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] D29510: Remove LIBLLDB_LOG_VERBOSE category

zturner added inline comments. Comment at: source/Target/SectionLoadList.cpp:70 bool warn_multiple) { - Log *log(lldb_private::GetLogIfAnyCategoriesSet(LIBLLDB_LOG_DYNAMIC_LOADER | - LI

[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/sour

[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? There

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

zturner added a comment. I guess the same way you would know how to use any part of a library or API. The first time you've ever used an API, you don't know how it works, so you don't know it's capabilities. So you fiddle around, read the source code, trudge through some compiler errors, then

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

zturner added a comment. FWIW, long term I would like to get rid of ALL printf style formatting. So right now, yes, they are only used in a few places. But it would be nice if it were the only type of formatting used anywhere. At which point I think most of your questions would be resolved o

[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&id=87260#toc Repository: rL LLVM https://reviews.llvm.org/D29514 Files:

[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] 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 &log_ptr); + static void Unregister(llvm::StringRef name); Not sure I like the id

[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. `R

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

[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 Date:

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

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

[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` o

[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/include/ll

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

[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/sourc

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

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

[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&id=89701#toc Repository: rL LLVM https://reviews.llvm.org/D30054 Files:

[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 makeArrayRef(m_argv).drop

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

zturner updated this revision to Diff 90432. zturner added a comment. I basically turned the two dump methods into free functions and moved the DWARF specific function into `DWARFCallFrameInfo.cpp`. I think this is much better than before, as we don't muck with the inheritance hierarchy at all

<    2   3   4   5   6   7   8   >