Page MenuHomePhabricator

[lldb] Fix TestSettings.test_pass_host_env_vars on windows
ClosedPublic

Authored by labath on Mar 26 2020, 3:42 AM.

Details

Summary

A defensive check in ProcessLauncherWindows meant that we would never
attempt to launch a process with a completely empty environment -- the
host environment would be used instead. Insted, I make function add an
extra null wchar_t at the end of an empty environment. The documentation
on this is a bit fuzzy, but it seems to be what is needed to make
windows accept these kinds of environments.

Diff Detail

Event Timeline

labath created this revision.Mar 26 2020, 3:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 3:42 AM
amccarth accepted this revision.Mar 26 2020, 9:09 AM

The code is correct, so I'm approving, but I suggest a couple fixes to the comments before committing..

lldb/source/Host/windows/ProcessLauncherWindows.cpp
27

You have an extra "list of" between null-terminated and "strings". And I think we should point out that it's UTF-16 encoded, so maybe:

// The buffer is a list of null-terminated UTF-16 strings, followed by an
// extra L'\0' (two bytes of 0).  An empty environment must have one
// empty string, followed by an extra L'\0'.
40

"... an extra two bytes ..."

42

There would be no harm in always adding the extra terminator.

This revision is now accepted and ready to land.Mar 26 2020, 9:09 AM
friss added a comment.Mar 26 2020, 9:09 AM

Thanks for fixing this Pavel!

labath marked 4 inline comments as done.Mar 30 2020, 7:08 AM
labath added inline comments.
lldb/source/Host/windows/ProcessLauncherWindows.cpp
42

Yeah, I wondered myself what would be less confusing... inserting a seemingly needless terminator, or cluttering the code with an extra if...

This revision was automatically updated to reflect the committed changes.
labath marked an inline comment as done.