labath added a comment.
Yes, this is definitely better.
================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:187
if (error.Fail() || !exe_module_sp) {
- if (FileSystem::Instance().Readable(
- resolved_module_spec.GetFileSpec())) {
+ if (FileSystem::Instance().Open(resolved_module_spec.GetFileSpec(),
+ lldb_private::File::eOpenOptionRead)) {
----------------
JDevlieghere wrote:
> Not your fault, but it seems weird that we're doing this check after using
> the `resolved_module_spec` and only if it failed.. Shouldn't we try to read
> it first and stop early if that fails?
In an ideal world the actual operation which tries to create the module would
return an error and we would use that. That way we wouldn't be accessing the
file twice, and creating opportunities for inconsistencies (or races). In that
sense, this code is still trying to "reverse engineer" why the earlier
operation failed, but I think it is a step forward, as it reduces the amount if
reversing.
================
Comment at: lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp:195
error.SetErrorStringWithFormat(
- "'%s' is not readable",
+ "'%s' is not readable by the current user",
resolved_module_spec.GetFileSpec().GetPath().c_str());
----------------
I guess this should be also updated to use the error message from the os(?)
================
Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:329
self.expect("target create -c doesntexist", error=True,
- substrs=["core file 'doesntexist' doesn't exist"])
+ substrs=["Cannot open 'doesntexist': No such file or
directory"])
----------------
Now that the error message comes from the OS, we might need to adjust the
expectation based on that. Just an FYI, in case you get some failure message
from some bot (most likely the windows one).
================
Comment at: lldb/test/API/commands/target/basic/TestTargetCommand.py:342
+ @no_debug_info_test
+ def test_target_create_unowned_core_file(self):
+ tf = tempfile.NamedTemporaryFile()
----------------
This is not testing an unowned file anymore, and is probably redudant given the
test above it. Given that the new implementation does not do anything special
wrt. ownership, I don't think an explicit test for those.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D78712/new/
https://reviews.llvm.org/D78712
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
