Page MenuHomePhabricator

Hui (Hui Huang)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 26 2018, 2:52 PM (81 w, 6 d)

Recent Activity

Yesterday

Hui added inline comments to D67689: [llvm-objcopy] Add support for --gap-fill and --pad-to options.
Sun, Sep 22, 8:50 PM · Restricted Project

Jul 23 2019

Hui added a comment to D65123: Restore tests for lldb-server and lldb-vscode removed at rL366590.

Any chance to restore the tracking history as well?

Jul 23 2019, 11:46 AM · Restricted Project, Restricted Project

Jul 18 2019

Hui added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Jul 18 2019, 10:56 PM · Restricted Project, Restricted Project
Hui added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Jul 18 2019, 10:48 PM · Restricted Project, Restricted Project
Hui added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Jul 18 2019, 9:10 PM · Restricted Project, Restricted Project

Jul 3 2019

Hui added inline comments to D63166: Move common functionality from processwindows into processdebugger.
Jul 3 2019, 6:39 PM · Restricted Project

Jun 21 2019

Hui added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Jun 21 2019, 9:39 AM · Restricted Project, Restricted Project
Hui added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Jun 21 2019, 8:35 AM · Restricted Project, Restricted Project

Jun 14 2019

Hui added inline comments to D63165: Initial support for native debugging of x86/x64 Windows processes.
Jun 14 2019, 2:50 PM · Restricted Project, Restricted Project
Hui added inline comments to D63166: Move common functionality from processwindows into processdebugger.
Jun 14 2019, 2:10 PM · Restricted Project
Hui added inline comments to D63166: Move common functionality from processwindows into processdebugger.
Jun 14 2019, 1:05 PM · Restricted Project

Jun 12 2019

Hui added a comment to D63166: Move common functionality from processwindows into processdebugger.

Given that you're just moving code around, this should be fine, but I am including @amccarth, as I believe he is more familiar with this code.

The thing I would consider in your place is whether putting this into a separate class is enough, or if you want to put it into a separate module/subfolder as well. The reason for that is that if this is in the same module as the liblldb stuff, then you will still end up pulling the all the transitive deps into lldb-server. If you put that into a separate module then you can control the dependencies more precisely. I don't think this is that important, since hopefully the local codepath (along with the deps) will disappear once lldb-server starts to work, but you know.. temporary solutions have a tendency for becoming permanent... Anyway, I'll leave the decision up to you..

Jun 12 2019, 3:16 PM · Restricted Project
Hui added a comment to D63165: Initial support for native debugging of x86/x64 Windows processes.

Sorry for the stupid question, but ...

What exactly is meant here by "Native"? How is a NativeProcessWindows different from ProcessWindows?

The Native*** classes are meant to be used from lldb-server. They look somewhat similar to their non-native counterpart because they still do debugging, but they're a lot dumber, because they only deal with basic process control, and none of the fancy symbolic stuff that you'd need debug info for.

Jun 12 2019, 3:02 PM · Restricted Project, Restricted Project

May 15 2019

Hui added inline comments to D61687: Update Python tests for lldb-server on Windows.
May 15 2019, 8:25 PM · Restricted Project, Restricted Project
Hui added inline comments to D61687: Update Python tests for lldb-server on Windows.
May 15 2019, 6:30 PM · Restricted Project, Restricted Project

May 13 2019

Hui added inline comments to D61687: Update Python tests for lldb-server on Windows.
May 13 2019, 9:02 PM · Restricted Project, Restricted Project

May 8 2019

Hui added inline comments to D61686: Enable lldb-server on Windows.
May 8 2019, 12:29 PM · Restricted Project

Apr 17 2019

Hui added inline comments to D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID().
Apr 17 2019, 7:50 AM · Restricted Project

Apr 16 2019

Hui added inline comments to D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID().
Apr 16 2019, 7:54 AM · Restricted Project

Apr 3 2019

Hui added inline comments to D59347: [DebugInfo] Combine Trivial and NonTrivial flags.
Apr 3 2019, 2:22 PM · Restricted Project, Restricted Project

Feb 12 2019

Hui added a comment to D56237: Implement GetLoadAddress for the Windows process plugin.

I like the direction this is going. I have one small comment about the code organization inline, but the bigger question I have in mind is: After these changes, is there any use for Host::GetProcessBaseAddress left? If so, what is it? I'd like to understand it and remove it.

Feb 12 2019, 10:47 PM · Restricted Project

Feb 4 2019

Hui added inline comments to D56237: Implement GetLoadAddress for the Windows process plugin.
Feb 4 2019, 8:24 AM · Restricted Project
Hui added a comment to D56237: Implement GetLoadAddress for the Windows process plugin.

Going to the Host straight from the DynamicLoader is the worst possible option, since then this code will not be correct for remote processes (it will just randomly pick up some data from the local process with the same pid, if you happen to have one). I'd like to hear what you think about the latest comments by Greg & myself.

I think there are some architectural issues here in how ProcessWindows works. Normally (i.e., on all non-windows process classes), it is the job of the dynamic loader plugin to create modules and set their load addresses. However, ProcessWindows does things differently, and creates the modules by itself (ProcessWindows::OnLoadDLL).
I think the behavior of ProcessWindows is fine (it flows naturally from how modules are handled on windows), but if it's going to handle module loading by itself, then I think it should do the job completely, and not only half-way. So, if we want ProcessWindows to do the module loading, then it should return a null value for GetDynamicLoader() (or a reasonably stubbed out implementation) instead of relying on a generic dynamic loader plugin to finish the job.

Or, if we want to have the DynamicLoader plugin, then ProcessWindows should stop meddling with the module list in the OnLoadDLL function. One way to achieve that would be:

  • ProcessWindows::OnLoadDLL calls DynamicLoaderWindowsDYLD::OnLoadDll
  • dynamic loader loads the module and sets load address
  • dynamic loader does not have to call ProcessWindows::GetFileLoadAddress as it already got the load address from in the OnLoadDLL function This would make the flow very similar to how other platforms handle it (the only difference would be that the "OnLoadDLL" equivalent is called from a breakpoint callback function, instead of from a special debug event callback, but that's just due to a difference in how modules are reported).

I would like the see the dynamic loader do all of the work. We can easily add any new virtual functions to the DynamicLoader class to fit how windows does things.

That should be fairly easy to achieve. The process class is the one that creates the dynamic loader (in GetDynamicLoader()), so it can easily call any special method on it that it needs from OnLoadDLL (i.e., we don't even need to extend the public DynamicLoader API).

However, note that this would not be the first process class that handles module loading on its own. ProcessMinidump already does that too. It could be changed to use a special dynamic loader too, but it's not clear to me whether that is worth the trouble, as the loading process for minidumps is very simple.

Feb 4 2019, 8:08 AM · Restricted Project
Hui added inline comments to D56237: Implement GetLoadAddress for the Windows process plugin.
Feb 4 2019, 7:19 AM · Restricted Project

Jan 24 2019

Hui added a comment to D56237: Implement GetLoadAddress for the Windows process plugin.

I think there is something wrong here. Normally, it should be the job of the dynamic loader plugin to figure out where a module got loaded (and populate the target section load list). Process::GetFileLoadAddress is there to help it with this job. So, the fact that ProcessWindows::GetFileLoadAddress goes back to the target section load list to get the data creates a weird loop. The only way I can see this working is if this implementation happens to return the module base address, as specified in the object file, and that base address happens to be correct (no ASLR). The kind of code I would expect to see here is a call to some windows API to determine the load address straight from the OS (if such a thing is possible). For example, on Linux, we parse the /proc/<pid>/maps file to determine this information.

Jan 24 2019, 10:50 AM · Restricted Project
Hui added inline comments to D56196: ProcessLaunchInfo: Remove Target reference.
Jan 24 2019, 10:37 AM

Jan 21 2019

Hui added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
In D56230#1361746, @Hui wrote:

It could be the llvm::sys::flattenWindowsCommandLine issue to flatten the command “dir c:\" for Windows Command Terminal.
However it is not an issue for PS or MingGW.

It is observed the flattened one is

"\"C:\\WINDOWS\\system32\\cmd.exe\" /C \" dir c:\\\\\" "

which will be interpreted as the following that is not accepted by CMD.exe.(dir c:\ or dir c:\.\ is fine. There is no '\' directory or file on my Drive c).
However it is accepted by PS and MingGW.

"C:\\WINDOWS\\system32\\cmd.exe" /C " dir c:\\"
Jan 21 2019, 10:11 PM
Hui added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Jan 21 2019, 8:35 PM
Hui added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Jan 21 2019, 8:02 PM
Hui added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Jan 21 2019, 7:18 PM
Hui added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Jan 21 2019, 11:33 AM

Jan 17 2019

Hui added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

It could be the llvm::sys::flattenWindowsCommandLine issue to flatten the command “dir c:\" for Windows Command Terminal.
However it is not an issue for PS or MingGW.

Jan 17 2019, 9:05 AM
Hui added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

I've always disliked this argument and hoped that someday someone would remove it entirely. My recollection (which may be wrong) is that the only actual use of it is so that if someone types a command, and we later need to print the command back, we will print it with the same quote char. It almost seems like we could just delete the argument and use a standardized quote char when flattening a command string.

+100

BTW, today I've tried to switch ProcessLauncherWindows to flattenWindowsCommandLine and this change alone was enough to fix TestQuoting, which has some tests XFAILed for windows due to quoting problems. I haven't sent out a patch yet because that has also broken platform shell dir c:\ for some reason, and I haven't gotten around to investigating that. If you (for any value of you) have some time, I'd encourage you to look into that. Otherwise, I'll get to that eventually.

Jan 17 2019, 8:07 AM

Jan 15 2019

Hui added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
  • 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 yourself
Jan 15 2019, 11:18 AM
Hui added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

For example, for a Args vector like lldb-server, gdb-remote, --log-channels=foo\\\ \\\""" ''', whatever, QuoteForCreateProcess would return
lldb-server gdb-remote "--log-channels=foo\\\ \\\\\\\"\"\" '''" whatever (there are other ways to quote this too). Passing this string to CreateProcess will result in the original vector being available to the main function of lldb-server, which is what this code (and all other code that works with the Args class) expects.

Btw, there is already code for doing this in llvm (llvm::sys::flattenWindowsCommandLine), so we can just steal the implementation from there.

Jan 15 2019, 10:16 AM

Jan 11 2019

Hui added a comment to D56232: [lldb-server] Add remote platform capabilities for Windows.

Is that even necessary? If a platform is not remote aware, IsHost() will always just return true by definition. So we could put all of this in the existing Platform base class.

I remember looking at this a while a go and concluding against it, but i'm not sure if it was impossible of just I didn't like the result.

The issue here is that PlatformWindows and PlatformPosix already have a m_remote_platform member (which normally is an instance of PlatformRemoteGDBServer). To move the common class into the base one, we'd need to move this member too. That would mean that any platform has a "remote" member, even those that already are "remote". That sounds a bit weird.

Jan 11 2019, 4:53 PM · Restricted Project
Hui added a comment to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.

I think the key problem here is to make sure the argument will be treated as a single argument to the process launcher.

Jan 11 2019, 2:01 PM

Jan 10 2019

Hui added a comment to D56234: [lldb-server] Add unnamed pipe support to PipeWindows.

Looks very nice. Thanks.

Jan 10 2019, 12:16 PM

Jan 9 2019

Hui added inline comments to D56234: [lldb-server] Add unnamed pipe support to PipeWindows.
Jan 9 2019, 2:43 PM

Jan 8 2019

Hui added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Jan 8 2019, 2:58 PM

Jan 4 2019

Hui added inline comments to D56230: [gdb-remote] Use lldb's portable Host::GetEnvironment() instead of getenv.
Jan 4 2019, 2:02 PM
Hui added a comment to D56229: [PECOFF] Implementation of ObjectFilePECOFF:: GetUUID().

Not quite sure but correct me if i am wrong.

Jan 4 2019, 1:36 PM · Restricted Project
Hui added a comment to D56231: [lldb-server] Improve support on Windows.

With all the aaron's pending reviews on lldb-server, I could try the patch with the following platform apis.

Jan 4 2019, 1:05 PM

Nov 15 2018

Hui added inline comments to D54544: Implement basic DidAttach and DidLaunch for DynamicLoaderWindowsDYLD.
Nov 15 2018, 1:38 PM

Oct 26 2018

Hui added inline comments to D53759: [PDB] Support PDB-backed expressions evaluation.
Oct 26 2018, 8:25 AM · Restricted Project
Hui added inline comments to D52618: [Windows] A basic implementation of memory allocations in a debuggee process.
Oct 26 2018, 7:31 AM · Restricted Project

Oct 22 2018

Hui added a comment to D53375: [PDB] Improve performance of the PDB DIA plugin.

Thanks for adding the cache. We noticed the slowdown also and were discussing the same thing. This LGTM if the other reviewers done have any comments.

Oct 22 2018, 1:36 PM · Restricted Project

Oct 10 2018

Hui added inline comments to D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules().
Oct 10 2018, 6:38 PM
Hui added inline comments to D53002: Create a new symbol file plugin for cross-platform PDB symbolization.
Oct 10 2018, 5:19 PM

Sep 30 2018

Hui added a comment to D52461: [PDB] Introduce `MSVCUndecoratedNameParser`.

Try to use and extend CPlusPlusLanguage::MethodName as needed. I believe it was recently backed by a new clang parser that knows how to chop up C++ demangled names

It seems that CPlusPlusLanguage::MethodName is backed by LLDB CPlusPlusNameParser, which can't parse demangled names... Can you tell me, please, how is called a new Clang parser you have mentioned? May be I'll use it directly instead of PDBNameParser, or will back PDBNameParser by it (if the interface will be not very convenient)?

Sep 30 2018, 8:58 AM · Restricted Project

Jul 24 2018

Hui added inline comments to D45123: [CodeView] Emit function options for subprogram and member functions.
Jul 24 2018, 4:26 PM
Hui added inline comments to D45123: [CodeView] Emit function options for subprogram and member functions.
Jul 24 2018, 3:19 PM

Jul 20 2018

Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 20 2018, 1:04 AM
Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 20 2018, 1:01 AM

Jul 18 2018

Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 18 2018, 10:49 PM
Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 18 2018, 10:33 PM
Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 18 2018, 10:07 PM
Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 18 2018, 10:02 PM
Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 18 2018, 9:57 PM
Hui added inline comments to D49410: [PDB] Parse UDT symbols and pointers to members.
Jul 18 2018, 9:53 PM

Feb 26 2018

Hui added inline comments to D43060: [CodeView] Lower __restrict and other pointer qualifiers correctly.
Feb 26 2018, 4:33 PM
Hui added inline comments to D43060: [CodeView] Lower __restrict and other pointer qualifiers correctly.
Feb 26 2018, 3:21 PM