[Lldb-commits] [PATCH] D77662: [WIP][lldb/test] Make TestLoadUnload compatible with windows

[Pavel Labath via Phabricator via lldb-commits]

labath marked 3 inline comments as done.
labath added inline comments.


================
Comment at: lldb/packages/Python/lldbsuite/test/make/dylib.h:50
+  FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM,
+      NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (char *)&msg, 0, NULL);
+  return msg;
----------------
amccarth wrote:
> This leaks the buffer allocated for the message.  I guess we don't expect this to happen often, so maybe that's not a big deal, and it's a terrible API to have to deal with.
yeah, that's just the test code and it's only purpose is to give some indication for when things go wrong. I've contemplated returning std::string here, but then I figured we may want to use this from some C code too and it may be better to just stick to C.


================
Comment at: lldb/test/API/functionalities/load_unload/TestLoadUnload.py:209
+        env_cmd_string = "settings set target.env-vars " + self.dylibPath + "=" + self.getBuildArtifact(".")
+        self.runCmd(env_cmd_string)
+
----------------
amccarth wrote:
> The env_cmd_string is going to eliminate ALL of the environment variables for the target except the one(s) that it explicitly sets.  Is that what you intended?
> 
> The lldb help says:
> > Warning:  The 'set' command re-sets the entire array or dictionary.  If you just want to add, remove or update individual values (or add something to the end), use one of the other settings sub-commands: append, replace, insert-before or insert-after.
> 
That's not as bad as it sounds because target.env-vars (as of last week, anyway) will only contain the variables set by the user, and the host environment will be applied separately.

Nonetheless, using append does seem like a better choice here.


================
Comment at: lldb/test/API/functionalities/load_unload/TestLoadUnload.py:419
         self.copy_shlibs_to_remote()
+        env_cmd_string = "settings set target.env-vars " + self.dylibPath + "=" + self.getBuildArtifact(".")
+        self.runCmd(env_cmd_string)
----------------
jingham wrote:
> You could also do this by making an SBLaunchInfo, adding the environment variable to it, and then you could use lldbutil.run_to_name_breakpoint.  That would remove some of the boilerplate below.
run_to_name_breakpoint does not seem like a completely good fit here as the test has assertions about numbers of breakpoint locations being found before running the process.

However, I think I've found an even better solution -- the test actually already contains logic for setting these paths, but it was hidden behind an `if(!darwin)` as it was not needed due to the use of `@executable_path` in dlopen calls. Now I've just removed the `if` and made that code slightly more generic.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77662/new/

https://reviews.llvm.org/D77662