[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] D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)

teemperor added a comment. +1. Especially since our assumption that everything related to C++ is of non-zero size is also not entirely correct. Empty structs for example are actually size 0 for Clang until we reach code gen where we set their size to 1. That's why we have this bug where we can'

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

clayborg added inline comments. Comment at: source/Plugins/Process/Windows/Common/ProcessWindows.cpp:858 +Status ProcessWindows::GetFileLoadAddress(const FileSpec &file, bool &is_loaded, + lldb::addr_t &load_addr) { asm

[Lldb-commits] [PATCH] D55122: [PDB] Fix location retrieval for function local variables and arguments that are stored relative to VFRAME

leonid.mashinskiy updated this revision to Diff 181792. leonid.mashinskiy set the repository for this revision to rLLDB LLDB. leonid.mashinskiy added a comment. Herald added a subscriber: arphaman. - Ported implementation to NativePDB plugin. - Implemented GetVariableLocationInfo for local variabl

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

clayborg added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFUnit.cpp:769-774 +FileSpec::Style DWARFUnit::GetPathStyle() { + if (!m_comp_dir) +ComputeCompDirAndGuessPathStyle(); + return m_comp_dir->GetPathStyle(); +} + Remove? ===

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

clayborg added inline comments. Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:29-48 +Token breakpad::toToken(llvm::StringRef str) { return llvm::StringSwitch(str) .Case("MODULE", Token::Module) .Case("INFO", Token::Info) .Case("FILE

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

clayborg added inline comments. Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:232 + }; + for (llvm::StringRef line: lines(*m_obj_file, Token::Func)) { +// Here we can get either FUNC records (starting with FUNC), or line records Sho

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

clayborg added a comment. So LLDB treats compile units special depending on the inline strategy. Because of this, I outlined some examples of how and why we should create a compile unit per "FUNC" token. Let me know if anything above was unclear Comment at: source/Plugins/Sym

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

davide accepted this revision. davide added a comment. This revision is now accepted and ready to land. I hit an incarnation of this problem, and I was able to get away with a localized fix in: [lldb] r344982 - [ValueObject] Stop assuming types are non-zero sized. Of course, your approach is pr

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

clayborg accepted this revision. clayborg added a comment. Looks good to me CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56688/new/ https://reviews.llvm.org/D56688 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm

[Lldb-commits] [lldb] r351215 - Silence compiler warnings

Author: adrian Date: Tue Jan 15 10:07:54 2019 New Revision: 351215 URL: http://llvm.org/viewvc/llvm-project?rev=351215&view=rev Log: Silence compiler warnings Modified: lldb/trunk/source/Plugins/Platform/MacOSX/PlatformAppleTVSimulator.cpp lldb/trunk/source/Plugins/Platform/MacOSX/Platfor

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

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

[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] D56688: Make CompilerType::getBitSize() / getByteSize() return an optional result. (NFC)

zturner added a comment. I had to stop commenting because there's too many occurrences, but please remove `auto` pretty much throughout the patch. A reader of the code will not necessarily be aware or remember that the function returns an `Optional`, and they will read the code as "if the size

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

zturner added inline comments. Comment at: include/lldb/Target/ProcessStructReader.h:70 } -size_t total_size = struct_type.GetByteSize(nullptr); -lldb::DataBufferSP buffer_sp(new DataBufferHeap(total_size, 0)); +auto total_size = struct_type.GetByteSize(nullptr);

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

Hui added a comment. > - 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` to add one more quote level around > that and escape the quotes added by yourse

[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] D56737: [SymbolFile] Split ParseVariablesForContext into two functions.

zturner created this revision. zturner added reviewers: clayborg, labath, jingham. This function was being used in 2 separate use cases. The first is parsing variables in a function, and the second is parsing global variables. To clarify the intent, I've split this into two functions, ParseLo

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

zturner added a comment. In D56230#1358328 , @labath wrote: > 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 correc

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

shafik added a comment. Sorry for the late review Comment at: lldb/trunk/include/lldb/Target/ProcessStructReader.h:70 } -size_t total_size = struct_type.GetByteSize(nullptr); -lldb::DataBufferSP buffer_sp(new DataBufferHeap(total_size, 0)); +auto total_size = s

[Lldb-commits] [lldb] r351237 - Replace auto -> llvm::Optional

Author: adrian Date: Tue Jan 15 12:33:58 2019 New Revision: 351237 URL: http://llvm.org/viewvc/llvm-project?rev=351237&view=rev Log: Replace auto -> llvm::Optional This addresses post-commit feedback for https://reviews.llvm.org/D56688 Modified: lldb/trunk/source/API/SBType.cpp lldb/trun

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

xiaobai created this revision. xiaobai added reviewers: zturner, compnerd, sgraenitz. Herald added subscribers: mgorny, ki.stfu. When building LLDB standalone on windows, lldb-mi fails to build because it doesn't link against User32. This patch adds an explicit dependency of lldb-mi on User32. h

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

aprantl marked an inline comment as done. aprantl added a comment. I changed all the autos back to full types in r351237. Comment at: lldb/trunk/source/Plugins/ABI/MacOSX-i386/ABIMacOSX_i386.cpp:827 CompilerType compiler_type(value->GetCompilerType()); -if (compiler_ty

[Lldb-commits] [lldb] r351243 - Add Doxygen comments.

Author: adrian Date: Tue Jan 15 13:04:18 2019 New Revision: 351243 URL: http://llvm.org/viewvc/llvm-project?rev=351243&view=rev Log: Add Doxygen comments. Modified: lldb/trunk/include/lldb/Symbol/CompilerType.h Modified: lldb/trunk/include/lldb/Symbol/CompilerType.h URL: http://llvm.org/vie

[Lldb-commits] [lldb] r351244 - Simplify code

Author: adrian Date: Tue Jan 15 13:04:19 2019 New Revision: 351244 URL: http://llvm.org/viewvc/llvm-project?rev=351244&view=rev Log: Simplify code Modified: lldb/trunk/source/Symbol/ClangASTContext.cpp Modified: lldb/trunk/source/Symbol/ClangASTContext.cpp URL: http://llvm.org/viewvc/llvm-p

[Lldb-commits] [PATCH] D56322: SBReproducer prototype

JDevlieghere updated this revision to Diff 181868. JDevlieghere added a comment. Herald added a subscriber: lldb-commits. Prototype 2.0 Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56322/new/ https://reviews.llvm.org/D56322 Files: include/lldb/API/SBReprodu

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

clayborg added a comment. Might be good to have a test where we have a relative DW_AT_name and not DW_AT_comp_dir? Just in case? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56543/new/ https://reviews.llvm.org/D56543 ___ lldb-commits mail

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

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Ok, thanks for explaining. Your reasoning makes sense. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56543/new/ https://reviews.llvm.org/D56543

[Lldb-commits] [lldb] r351250 - Simplify Value::GetValueByteSize()

Author: adrian Date: Tue Jan 15 13:26:03 2019 New Revision: 351250 URL: http://llvm.org/viewvc/llvm-project?rev=351250&view=rev Log: Simplify Value::GetValueByteSize() Modified: lldb/trunk/source/Core/Value.cpp Modified: lldb/trunk/source/Core/Value.cpp URL: http://llvm.org/viewvc/llvm-proj

[Lldb-commits] [lldb] r351263 - [debugserver][CMake] Remove commented out line

Author: xiaobai Date: Tue Jan 15 14:27:59 2019 New Revision: 351263 URL: http://llvm.org/viewvc/llvm-project?rev=351263&view=rev Log: [debugserver][CMake] Remove commented out line This has been commented out since rL300111 (commit d742d081f3a1e7412cc609765139ba32d597ac15). Looks like it was comm

[Lldb-commits] [lldb] r351264 - Simplify code by using Optional::getValueOr()

Author: adrian Date: Tue Jan 15 14:30:01 2019 New Revision: 351264 URL: http://llvm.org/viewvc/llvm-project?rev=351264&view=rev Log: Simplify code by using Optional::getValueOr() Modified: lldb/trunk/source/Core/ValueObjectVariable.cpp lldb/trunk/source/Plugins/ABI/SysV-ppc64/ABISysV_ppc6

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

compnerd accepted this revision. compnerd added a comment. This revision is now accepted and ready to land. I personally prefer the use of `target_link_libraries` rather than the custom stuff added in the `add_lldb_tool`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56741/new/ https:/

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

JDevlieghere added a comment. With the LLVM changes now checked-in this is ready for review. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D54617/new/ https://reviews.llvm.org/D54617 ___ lldb-commits mailing list ll

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

zturner added a comment. Why do we need to link against user32? What kind of unresolved external are you getting? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56741/new/ https://reviews.llvm.org/D56741 ___ lldb-commits mailing list lldb-

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

JDevlieghere accepted this revision as: JDevlieghere. JDevlieghere added a comment. This revision is now accepted and ready to land. Although I don't want to condone checking things in without a test case, given that the fix is obvious and that testing it is non-trivial, I think it's fine to che

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

xiaobai added a comment. @zturner: `undefined symbol: __imp_MessageBoxA`. Looks like MessageBoxA is used in the file `tools/lldb-mi/MIUtilDebug.cpp`, specifically in the function `CMIUtilDebug::ShowDlgWaitForDbgAttach`. In D56741#1358733 , @compnerd wrot

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

zturner added a comment. It seems like a pretty bad idea to have lldb-mi show a dialog box :-/ I don't actually work on or have anything to do with lldb-mi though, but I don't know how well-maintained or supported it is these days anyway. But if nobody chimes in and says why this is important

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

xiaobai added a comment. In D56741#1358804 , @zturner wrote: > It seems like a pretty bad idea to have lldb-mi show a dialog box :-/ > > I don't actually work on or have anything to do with lldb-mi though, but I > don't know how well-maintained or support

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

jingham added a comment. As far as I can tell, this dialog is only used when you build the lldb-mi with MICONFIG_DEBUG_SHOW_ATTACH_DBG_DLG, which you would only do if you wanted to debug lldb-mi. The point is to stall the lldb-mi launch at an early point till you've gotten a debugger attached

[Lldb-commits] [lldb] r351274 - Remove redundant check.

Author: adrian Date: Tue Jan 15 15:33:26 2019 New Revision: 351274 URL: http://llvm.org/viewvc/llvm-project?rev=351274&view=rev Log: Remove redundant check. Modified: lldb/trunk/source/Core/Value.cpp Modified: lldb/trunk/source/Core/Value.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/tr

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

xiaobai added a comment. In D56741#1358882 , @jingham wrote: > As far as I can tell, this dialog is only used when you build the lldb-mi > with MICONFIG_DEBUG_SHOW_ATTACH_DBG_DLG, which you would only do if you > wanted to debug lldb-mi. The point is to

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

jingham added a comment. That seems right. , IME on a decently fast Linux or macOS machine "attach -w" will get lldb attached to the process way before it would have gotten this far, so I'm not sure how useful this convenience is altogether. But maybe "attach -w" is slower on Windows. C

[Lldb-commits] [PATCH] D56741: [CMake] Explicitly list User32 as dependency of lldb-mi

xiaobai abandoned this revision. xiaobai added a comment. Abandoned in favor of D56755 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56741/new/ https://reviews.llvm.org/D56741 ___ lldb-commits mailing list

[Lldb-commits] [lldb] r351276 - [lldb-mi] Remove use of dialog box

Author: xiaobai Date: Tue Jan 15 16:09:50 2019 New Revision: 351276 URL: http://llvm.org/viewvc/llvm-project?rev=351276&view=rev Log: [lldb-mi] Remove use of dialog box Summary: This really is only implemented on Windows, and it requires us to pull in User32. This was only useful when debugging o

[Lldb-commits] [PATCH] D56763: [CMake] Prevent lldbDebugserverCommon from building if you disable debugserver builds

xiaobai created this revision. xiaobai added reviewers: sgraenitz, davide, JDevlieghere, beanz, vsk, aprantl, labath. Herald added subscribers: jfb, mgorny. The flags `LLDB_USE_SYSTEM_DEBUGSERVER` and `LLDB_NO_DEBUGSERVER` were introduced to the debugserver build. If one of these two flags are se