clayborg (Greg Clayton)
User

Projects

User does not belong to any projects.

User Details

User Since
Sep 23 2014, 10:11 AM (178 w, 2 h)

Recent Activity

Fri, Feb 16

clayborg added a comment to D40199: [dwarfdump] Fix spurious verification errors for DW_AT_location attributes.

I look forward to this being checked in

Fri, Feb 16, 3:14 PM · debug-info
clayborg accepted D43345: [LLDB] Initial version of PPC64 InstEmulation.
Fri, Feb 16, 8:38 AM
clayborg accepted D43333: Add SBDebugger::GetBuildConfiguration and use it to skip an XML test.

Looks good. Self describing and concise.

Fri, Feb 16, 7:06 AM

Thu, Feb 15

clayborg requested changes to D43345: [LLDB] Initial version of PPC64 InstEmulation.

Ok, marking as needs changes.

Thu, Feb 15, 11:52 AM
clayborg accepted D43345: [LLDB] Initial version of PPC64 InstEmulation.

Looks good if you are not concerned with my inline comment about using bitfields and endianess

Thu, Feb 15, 11:43 AM
clayborg added inline comments to D43333: Add SBDebugger::GetBuildConfiguration and use it to skip an XML test.
Thu, Feb 15, 11:11 AM
clayborg accepted D43344: [LLDB][PPC64] Fixed next blocked forever at same line.
Thu, Feb 15, 10:49 AM
clayborg accepted D43333: Add SBDebugger::GetBuildConfiguration and use it to skip an XML test.

Think if we want to cache the configuration in case we start using this a lot more in the test suite. Doesn't need to be done now.

Thu, Feb 15, 8:08 AM

Mon, Feb 12

clayborg accepted D43202: Remove dead code for handling DWARF pubnames.
Mon, Feb 12, 11:17 AM
clayborg added inline comments to D42955: Make Module::GetSectionList output consistent.
Mon, Feb 12, 7:33 AM

Mon, Feb 5

clayborg accepted D42917: Adapt some tests to work with PPC64le architecture.
Mon, Feb 5, 11:37 AM
clayborg accepted D42891: DWZ 01/12: refactor: DWARFCompileUnit::Producer -> DWARFProducer.
Mon, Feb 5, 9:33 AM

Fri, Feb 2

clayborg added a comment to D42870: Correct recognition of NetBSD images.

You would check in the YAML code for the NetBSD ELF file so that the test case can make it into an ELF file, run the test, verify things, and then cleanup the created ELF file.

Fri, Feb 2, 3:27 PM
clayborg added a comment to D42870: Correct recognition of NetBSD images.

Probably take a ELF file that is NetBSD and obj2yaml it. The test would run yaml2obj on it and then test that things are recognized correctly via the SB interfaces (check triple is correct)?

Fri, Feb 2, 3:26 PM
clayborg added a comment to D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line.

Question: if you have a global entry point, is there ever any reason to stop at these? Is there anything you can debug in the global entry point? Does a global entry point always forward on to a local entry point? Does the local entry point always exists and is it the only thing that can be debugged? If so, then it sounds like we should mark the global entry point as a trampoline by setting the symbol type to lldb::eSymbolTypeTrampoline. Then all stepping logic will automatically just go through this code.

Fri, Feb 2, 11:37 AM
clayborg accepted D42853: Resolve binary symlinks before finding its separate .debug file.

Makes sense for me, but make sure Pavel is ok with this as well before committing.

Fri, Feb 2, 10:23 AM
clayborg accepted D42852: Fix upper->lower case for /usr/lib/debug/.build-id/**.debug.

Many file systems are case insensitive so that is probably why it works on some computers.

Fri, Feb 2, 10:21 AM
clayborg added inline comments to D42828: Fix for read-past-end-of-array buglet in ProcessElfCore.cpp while reading linux notes.
Fri, Feb 2, 8:10 AM · Restricted Project

Thu, Feb 1

clayborg accepted D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line.

Looks good! Not sure if we need the PPC specific comment in Architecture.h, but I won't hold up this patch for that. Might be nice to move the PPC comment into ArchitecturePPC64.h.

Thu, Feb 1, 1:35 PM
clayborg added a comment to D42195: [lldb] Generic base for testing gdb-remote behavior.

I am pretty sure that the ack that gets sent should happen in response to getting an ACK. Note that LLDB send an ACK and then waits for an ACK. This helps us see if anything is there. So yes, this code can always just respond to an unsolicited ACK with an ACK.

Thu, Feb 1, 10:05 AM
clayborg added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

I think we're slowly getting there, but we could cleanup the implementation a bit.

I am also not sure that the WriteObjectFile name really captures what this function does, but I don't have a suggestion for a better name either....

Thu, Feb 1, 8:16 AM
clayborg added a comment to D39307: Fix global data symbol resolution.

It would be nice if we can try and disable the redeclaration lookups from clang when in debugger mode. I can't remember if we already have such a flag in the compiler options. If we do, we should try to just disable this when in debug expression mode and see how the test suite fares. Unless we are able to lookup the name within the AST that is currently being built so we can re-find the exact same variable declaration and hand it back, disabling this seems to be the correct thing to do for debug expression. Comments?

Thu, Feb 1, 8:14 AM

Tue, Jan 30

clayborg added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

This discussion has got me questioning if WriteMemory is even the best place to put the flash commands. It seems like some of the concern here is around the behavior of arbitrary memory writes to flash regions, which isn't really the main objective of this patch (supporting object file loading into flash).

Tue, Jan 30, 11:45 AM
clayborg added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

Thanks. What I'm struggling to reconcile are your statements that users should not have to know how things must happen, but then that we should make ObjectFile::Load smart so it doesn't result in an unnecessary amount of reads/writes. If writing to flash memory effectively requires some smarts on the caller's part, then how have we made things easier for the callers?

Tue, Jan 30, 11:38 AM

Mon, Jan 29

clayborg added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

I'm not envisioning that anything else needs to change to use begin/end or care it's there. I guess the way I look at it, having ObjectFile::LoadInMemory do begin/end is basically the same as what you're saying about having it be more process aware.

If Process is going to introduce new concepts for ObjectFile to use either way, isn't a high level of abstraction (batched writes) preferable to ObjectFile needing to know the fine details of flash memory blocks or that flash is even used? And doesn't keeping the heavy lifting in ProcessGDB make it reusable should another case come along?

Hope you don't mind the pushback. I think we're looking at this from different angles, and I'm genuinely asking to help my understanding of your thinking so hopefully we can converge on the same view.

Mon, Jan 29, 3:48 PM
clayborg added a comment to D42656: [testsuite] Remove flakey test relying on `pexpect`.

There would be no spinning an instance, it would be call the API in python. No extra process, done in process.

Mon, Jan 29, 3:47 PM
clayborg accepted D42317: [Host] Respect LLVM_LIBDIR_SUFFIX when looking for LLDB plugins on Linux.

Looks good.

Mon, Jan 29, 10:20 AM
clayborg added a comment to D42317: [Host] Respect LLVM_LIBDIR_SUFFIX when looking for LLDB plugins on Linux.

See inlined comment

Mon, Jan 29, 8:25 AM
clayborg added inline comments to D42317: [Host] Respect LLVM_LIBDIR_SUFFIX when looking for LLDB plugins on Linux.
Mon, Jan 29, 8:24 AM

Fri, Jan 26

clayborg accepted D42591: Update GSOC 2018 page for the LLDB project.
Fri, Jan 26, 1:15 PM
clayborg added a reviewer for D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line: jingham.
Fri, Jan 26, 10:20 AM
clayborg added a comment to D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line.

Added Jim Ingham so he can chime in on this and see if he agrees with my solution

Fri, Jan 26, 10:20 AM
clayborg requested changes to D42582: [lldb][PPC64] Fixed step-in stopping in the wrong line.

This seems like functionality that should go into an architecture plug-in. The class is currently "lldb_private::Architecture". For an example see the ARM version:

Fri, Jan 26, 10:20 AM

Thu, Jan 25

clayborg accepted D42488: Remove ObjectFile usage from HostLinux::GetProcessInfo.
Thu, Jan 25, 9:42 AM
clayborg accepted D42195: [lldb] Generic base for testing gdb-remote behavior.

Thanks for iterating on this. This will be great.

Thu, Jan 25, 6:39 AM
clayborg added a comment to D31451: New C++ function name parsing logic.

This code is processing demangled names. Since you say (I could not get my demangler to process it either) the symbol demangles to a multi-megabyte name, we can probably make the cutoff even longer then 1000 bytes.

Thu, Jan 25, 6:35 AM

Wed, Jan 24

clayborg added a comment to D31451: New C++ function name parsing logic.

The funny thing is this is only 981 characters long... Not sure what the right cutoff would be...

Wed, Jan 24, 4:53 PM
clayborg added a comment to D31451: New C++ function name parsing logic.

This is part of the problem, but not the entire thing.. We had a mangled name:
_ZNK3shk6detail17CallbackPublisherIZNS_5ThrowERKNSt15__exception_ptr13exception_ptrEEUlOT_E_E9SubscribeINS0_9ConcatMapINS0_18CallbackSubscriberIZNS_6GetAllIiNS1_IZZNS_9ConcatMapIZNS_6ConcatIJNS1_IZZNS_3MapIZZNS_7IfEmptyIS9_EEDaS7_ENKUlS6_E_clINS1_IZZNS_4TakeIiEESI_S7_ENKUlS6_E_clINS1_IZZNS_6FilterIZNS_9ElementAtEmEUlS7_E_EESI_S7_ENKUlS6_E_clINS1_IZZNSL_ImEESI_S7_ENKUlS6_E_clINS1_IZNS_4FromINS0_22InfiniteRangeContainerIiEEEESI_S7_EUlS7_E_EEEESI_S6_EUlS7_E_EEEESI_S6_EUlS7_E_EEEESI_S6_EUlS7_E_EEEESI_S6_EUlS7_E_EESI_S7_ENKUlS6_E_clIS14_EESI_S6_EUlS7_E_EERNS1_IZZNSH_IS9_EESI_S7_ENKSK_IS14_EESI_S6_EUlS7_E0_EEEEESI_DpOT_EUlS7_E_EESI_S7_ENKUlS6_E_clINS1_IZNS_5StartIJZNS_4JustIJS19_S1C_EEESI_S1F_EUlvE_ZNS1K_IJS19_S1C_EEESI_S1F_EUlvE0_EEESI_S1F_EUlS7_E_EEEESI_S6_EUlS7_E_EEEESt6vectorIS6_SaIS6_EERKT0_NS_12ElementCountEbEUlS7_E_ZNSD_IiS1Q_EES1T_S1W_S1X_bEUlOS3_E_ZNSD_IiS1Q_EES1T_S1W_S1X_bEUlvE_EES1G_S1O_E25ConcatMapValuesSubscriberEEEDaS7_

Wed, Jan 24, 3:52 PM
clayborg added a comment to D31451: New C++ function name parsing logic.

This code is making debugging of large C++ apps so slow it is unusable...

Wed, Jan 24, 10:26 AM
clayborg accepted D42488: Remove ObjectFile usage from HostLinux::GetProcessInfo.

Looks fine to me.

Wed, Jan 24, 10:19 AM
clayborg added inline comments to D42468: [lldb][PPC64] Fixed vector and struct return value.
Wed, Jan 24, 10:15 AM

Tue, Jan 23

clayborg added a comment to D39967: Refactoring of MemoryWrite function.

That would be an easy fix for the ObjectFile::Load(), we could see if there are any breakpoints in the range we are trying to write, get a list of them, disable them all, write memory and re-enable. But this doesn't stop a user from trying doing something like:

(lldb) script lldb.process.WriteMemory(...)

And they should expect this to just work no matter where they write memory.

Couldn't WriteMemory do the same thing as well? I would expect that 99% of WriteMemory calls will not intersect any breakpoints so the overhead of disabling and re-enabling should be negligible.

Tue, Jan 23, 8:28 AM
clayborg added a comment to D39967: Refactoring of MemoryWrite function.

I completely agree with your point, but why isn't enough just to return an error about breakpoints in the area user wants to write?

Tue, Jan 23, 8:18 AM
clayborg added a comment to D39967: Refactoring of MemoryWrite function.

Does this functionality really belong in the client? In case of memory reads, it's the server who patches out the breakpoint opcodes (NativeProcessProtocol::ReadMemoryWithoutTrap). I think it would make sense to do this in the same place.

Will not work for gdb-remote mode, other servers treat this just as a block of memory.
I might be wrong, but gdb inserts a breakpoint right before execution of instruction range, containing this breakpoint, and removes right after stop.

Tue, Jan 23, 7:11 AM

Mon, Jan 22

clayborg added a comment to D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF .

Sounds good then.

Mon, Jan 22, 4:25 PM · Restricted Project
clayborg added a comment to D39969: Set error status in ObjectFile::LoadInMemory if it is not set .

And yes, if the write memory can only write fewer byte than requested, it won't be an error if at least some bytes were written

Mon, Jan 22, 11:41 AM
clayborg added a comment to D39969: Set error status in ObjectFile::LoadInMemory if it is not set .

I am not sure we can say for sure that a breakpoint intersected with the write here? I would rather just have an error saying something like "only %u of %u bytes in section %s were written". Extra credit for checking if there are overlapping breakpoints and appending "(try disabling all breakpoints and loading again)" to the error message.

Mon, Jan 22, 11:13 AM
clayborg added a comment to D39967: Refactoring of MemoryWrite function.

This will need a test, preferable covering all of the cases:

  • breakpoint right at start with extra data after
  • breakpoint right at start with no extra data after
  • breakpoint in middle of ranges with data before and after
  • breakpoint at end of range with no data after
  • multiple breakpoints tests with two breakpoints in a row with no data between with no data before first or after last
  • multiple breakpoints tests with two breakpoints in a row with no data between with data before first and after last
  • multiple breakpoints tests with two discontiguous breakpoints data between them with data before first and after last
Mon, Jan 22, 10:58 AM
clayborg added a comment to D40283: lldb: Use the DWARF linkage name when importing C++ methods.

As Greg pointed out C++ (and Swift) mangled names can be enormous, so it would definitely have an impact on the memory footprint (unless we are only passing StringRefs around. Are we?)

Mon, Jan 22, 10:00 AM
clayborg added a comment to D40283: lldb: Use the DWARF linkage name when importing C++ methods.

I am fine with checking this. The only issue on my end is the extra memory that will be needed to store these often huge mangled names in every AST context for the 0.0001% of the time it is actually needed.

Mon, Jan 22, 9:09 AM

Jan 19 2018

clayborg accepted D42280: Wrap all references to build artifacts in the LLDB testsuite in TestBase::getBuildArtifact().

Nice!

Jan 19 2018, 11:46 AM
clayborg added a comment to D42281: Compile the LLDB tests out-of-tree.

Looks like a good start. It might be nice to validate that after "clean" that we have no files that are untracked in the test directory? This could help us to verify that we don't leave artifacts around.

Jan 19 2018, 8:50 AM

Jan 17 2018

clayborg added a comment to D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry.

Are we going to test each unix call that can fail with EINTR? Seems a bit overkill.

Jan 17 2018, 4:22 PM · Restricted Project
clayborg accepted D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry.

This is a very common unix thing.

Jan 17 2018, 3:47 PM · Restricted Project
clayborg accepted D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF .
Jan 17 2018, 3:10 PM · Restricted Project
clayborg added a comment to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

My objection to the BeginWriteMemoryBatch and EndWriteMemoryBatch is users must know to emit these calls when they really just want to call process.WriteMemory() and not worry about how it is done. Much like writing to read only code when setting breakpoints, we don't require the user to know that they must check the memory permissions first, set permissions to be writable and then revert them back after writing to a memory location.

Jan 17 2018, 3:05 PM
clayborg added a comment to D42195: [lldb] Generic base for testing gdb-remote behavior.

Looks fine to me. Wait for Pavel to give it the OK since he did more of the lldb-server testing stuff.

Jan 17 2018, 2:57 PM
clayborg accepted D41702: Add SysV Abi for PPC64le.
Jan 17 2018, 12:06 PM
clayborg added inline comments to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.
Jan 17 2018, 11:04 AM
clayborg added a comment to D41702: Add SysV Abi for PPC64le.

Looks nice. Only nit is we probably don't need the m_endian member variable. See inlined comment.

Jan 17 2018, 10:11 AM
clayborg accepted D42182: Add LLDB_LOG_ERROR (?).
Jan 17 2018, 10:04 AM
clayborg requested changes to D42145: [lldb] Use vFlash commands when writing to target's flash memory regions.

Very nice overall. See inlined comments. Big issues are:

  • GDBRemoteCommunication::WriteEscapedBinary() is not needed as StreamGDBRemote::PutEscapedBytes() does this already
  • Remove the batch start and end functions in process if possible and have ProcessGDBRemote::DoWriteMemory() just "do the right thing"
  • Can we actually cache the results of the qXfer:memory-map for the entire process lifetime?
  • Remove the new ProcessGDBRemote::GetMemoryMapRegion() and fold into GDBRemoteCommunicationClient::GetMemoryRegionInfo() as needed
Jan 17 2018, 10:00 AM

Jan 12 2018

clayborg requested changes to D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF .

Very cool and close. It would be nice to function correctly without asserts, see inlined comment.

Jan 12 2018, 11:08 AM · Restricted Project

Jan 11 2018

clayborg added a comment to D41909: Fix deadlock in dwarf logging.

Yeah, the threading was added later. It started out as single threaded and didn't have this problem.

Jan 11 2018, 2:26 PM
clayborg added a comment to D41909: Fix deadlock in dwarf logging.

We will need to pass the mutex down into any call that needs to let the mutex go. At least that is the only way I could see this working... But then all functions that do anything lazily will need this same treatment. Lot of trouble for the logging.

Jan 11 2018, 2:09 PM
clayborg added a comment to D41909: Fix deadlock in dwarf logging.

Why do we need to lock the Module mutex in SymbolVendor::FindFunctions? I think a better option would be to remove that lock and if it is needed then lock it just for the calls where it necessary. The fact that SymbolVendor locks a mutex inside a Module feels like a pretty bad layering violation for me what can cause many other deadlocks so it would be nice to fix that instead of hacking it around here.

Jan 11 2018, 8:08 AM

Jan 10 2018

clayborg added a comment to D40283: lldb: Use the DWARF linkage name when importing C++ methods.

And I do agree with Jim that we don't want to have to mangle the typename to see if it matches, that is too much work.

Jan 10 2018, 1:55 PM
clayborg added a comment to D40283: lldb: Use the DWARF linkage name when importing C++ methods.

Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there any way that a DIE is marked up with an extra attribute when the asm name is explicitly set? It would be great to know this from the DWARF so we don't have to end up setting the ASM name for every single possibly affected DIE...

Jan 10 2018, 1:54 PM
clayborg added a reviewer for D40283: lldb: Use the DWARF linkage name when importing C++ methods: aprantl.
Jan 10 2018, 1:53 PM
clayborg added a comment to D40283: lldb: Use the DWARF linkage name when importing C++ methods.

Sounds like finding a clang expert to clarify what Jim last asked for is the way forward. Do a source control "blame" command and see who worked on the code in the area of clang and maybe add them as reviewers so they can comment? I agree with Jim that this sounds like a good fix, but we should be careful. Is there no other way to detect this special "asm" name? No extra DWARF attributes?

Jan 10 2018, 1:53 PM
clayborg added a comment to D41909: Fix deadlock in dwarf logging.

Tough to call on this one. Yes the function is dumping simple stuff, but it is using m_arch, m_file and m_objname. It could cause crashes in multi-threaded environments. Is the deadlock caused by an A/B lock issue?

Jan 10 2018, 1:49 PM
clayborg added a comment to D41902: Remove Platform references from the Host module.

As long as:

Jan 10 2018, 1:43 PM
clayborg added a comment to D41533: Advanced guessing of rendezvous breakpoint.

Looks good.

Jan 10 2018, 1:37 PM · Restricted Project
clayborg accepted D41584: Check existence of each required component during construction of LLVMCDisassembler..

Fix comment spacing as mentioned in inline comments and this is good to go.

Jan 10 2018, 1:34 PM

Jan 9 2018

clayborg added a comment to D41584: Check existence of each required component during construction of LLVMCDisassembler..

I like the overall direction this patch is taking, just a few fixes.

Jan 9 2018, 11:08 AM
clayborg requested changes to D41584: Check existence of each required component during construction of LLVMCDisassembler..
Jan 9 2018, 11:08 AM
clayborg accepted D41871: [dotest] Remove crashinfo hook.
Jan 9 2018, 9:29 AM
clayborg added a comment to D41533: Advanced guessing of rendezvous breakpoint.

Looks fine. Set the breakpoint using the list of names and delete the breakpoint if you get no locations and this will be good to go.

Jan 9 2018, 8:25 AM · Restricted Project

Jan 8 2018

clayborg accepted D41745: Handle O reply packets during qRcmd.

Updated patch with new function name suggested by @clayborg. Added unit test and changed to llvm::function_ref as suggested by @labath.

Jan 8 2018, 3:31 PM
clayborg added a comment to D26242: Test for YMMRegisters..

Detecting x86_64h might be tricky on different platforms. Two things I can think of: explicitly compile using x86_64h and when you try to run it it might fail. I believe that will work for Darwin, but not sure about other OSs (linux, Android). Mach-o has the x86_64h CPU type/subtyype, but I am not sure how ELF does this since they only really have the machine type and it isn't enough to represent x86_64h. The other way would be to start up the process (if it launches on all platforms) and look for a register by name that is only available if haswell support is available on the system. I believe all of our targets do correctly detect their AVX regs, so just stopping and looking for the register by name, and end the test as an expected failure or success if the register isn't available.

Jan 8 2018, 8:36 AM

Jan 3 2018

clayborg accepted D41702: Add SysV Abi for PPC64le.
Jan 3 2018, 10:57 AM

Dec 21 2017

clayborg accepted D41352: debugserver: Propagate environment in launch-mode (pr35671).

Thanks for all the revisions. Looks good.

Dec 21 2017, 8:50 AM
clayborg added a comment to D41086: [lldb] Stop searching for a symbol in a pdb by regex.

Looks good!

Dec 21 2017, 8:04 AM

Dec 20 2017

clayborg added a comment to D41086: [lldb] Stop searching for a symbol in a pdb by regex.

If you load a exe file that has a PDB file, people can currently run:

Dec 20 2017, 10:49 AM
clayborg accepted D40079: Make sure DataBufferLLVM contents are writable.

Looks good, thanks for the making the changes. This will make future tweaks to reading of file data in object files just a single change in ObjectFile.cpp.

Dec 20 2017, 10:45 AM
clayborg added inline comments to D41352: debugserver: Propagate environment in launch-mode (pr35671).
Dec 20 2017, 10:44 AM

Dec 19 2017

clayborg added a comment to D41352: debugserver: Propagate environment in launch-mode (pr35671).

This is exposing a bug where if we have options like:

Dec 19 2017, 10:42 AM
clayborg added a comment to D41352: debugserver: Propagate environment in launch-mode (pr35671).

Environment vars might be set by using --env, so those need to go into "inferior_envp" first. If we are launching, we will copy only the host environment vars that haven't been already set manually (they don't already exist in the inferior_envp).

Dec 19 2017, 10:41 AM
clayborg requested changes to D41352: debugserver: Propagate environment in launch-mode (pr35671).

So we can also specify extra environment variable using the "--env" option with debugserver and your current fix will break that.

Dec 19 2017, 10:36 AM
clayborg added a comment to D41352: debugserver: Propagate environment in launch-mode (pr35671).

The existing code for the "--forward-env" does this:

Dec 19 2017, 10:35 AM
clayborg requested changes to D40079: Make sure DataBufferLLVM contents are writable.

See inlined comment. Quick fix and this will be good to go.

Dec 19 2017, 9:04 AM
clayborg added inline comments to D41086: [lldb] Stop searching for a symbol in a pdb by regex.
Dec 19 2017, 8:38 AM
clayborg added a comment to D41352: debugserver: Propagate environment in launch-mode (pr35671).

So the main question is what do we expect to happen by default. I kind of agree that if we launch the inferior directly when launching I would expect the environment to be passed along. Jason: since we always just launch debugserver for Xcode in the mode that doesn't expect environment variables to be passed along, I think changing the default behavior would be a good idea and it would only affect internal Apple customers. What do you think? We might need to add a "--no-forward-env" option in case anyone doesn't want this behavior just in case if we do switch the default.

Dec 19 2017, 8:31 AM
clayborg added a comment to D41359: Add Utility/Environment class for handling... environments.

Thanks for the explanation. Good to go.

Dec 19 2017, 8:17 AM

Dec 18 2017

clayborg added a comment to D41352: debugserver: Propagate environment in launch-mode (pr35671).

I am ok either way, but I do want to make sure Jason gets input before we do anything. He might be out for a few weeks over the break, so there might be a delay.

Dec 18 2017, 10:11 AM
clayborg accepted D41359: Add Utility/Environment class for handling... environments.

ok as long as we don't want to return const reference when returning Environment values in getters and setters.

Dec 18 2017, 10:09 AM
clayborg added a comment to D41352: debugserver: Propagate environment in launch-mode (pr35671).

We already have an option for this named "--forward-env". It might be better to fix this by adding the "--forward-env" argument to the debugserver launch during testing? When we launch through LLDB, it forwards all environment variables manually. Maybe we add a "--forward-env" option to lldb-server too? Not sure what the right thing to do here is. iOS, tvOS and watchOS won't be affected since we never launch directly and all env vars are manually set by LLDB. Jason, any comments on this one?

Dec 18 2017, 9:55 AM
clayborg added inline comments to D41086: [lldb] Stop searching for a symbol in a pdb by regex.
Dec 18 2017, 9:44 AM

Dec 15 2017

clayborg accepted D41245: Reduce x86 register context boilerplate..

Looks good

Dec 15 2017, 2:34 PM

Dec 14 2017

clayborg accepted D40616: ObjectFileELF: Add support for compressed sections.

Move #include of "llvm/Object/Decompressor.h" into CPP file and this is good to go.

Dec 14 2017, 9:19 AM