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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
