[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
https://github.com/slydiman closed https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
https://github.com/labath approved this pull request. I like this new version, thanks for sticking with me. https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
slydiman wrote: @labath, Sorry for not being clear with my comment. Let me re-phrase. I think unconditionally setting the executable flag for everything installed by Platform::Install by default for all platforms is overkill. BTW, there is no API to change this behavior, so `by default` means always. Implementation details aside. Test files have the correct permissions set if the host allows it. Platform::Install copies the host file permissions to the target. This seems a correct behavior except for the case when the host has a smaller set of permissions than the target, or permission sets on the host and the target do not match significantly. I'm aware of the only such case: Windows host and POSIX target. Do you know more? https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
labath wrote: I'm not sure I'm following your reasoning. I'm not saying we should replace/remove Target::Install. I'm saying we should make everything installed by Platform::Install executable by default (which, by extension, includes everything installed by Target::Install, because it delegates to this function. And you're saying that's not a good idea, because what? That not everything installed through Platform::Install is an actual executable? https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
slydiman wrote: Target::Install() and Platform::Install() are used indirectly in many cases. For example look at the test `lldb/test/API/python_api/hello_world/TestHelloWorld.py`. target.LaunchSimple() uses Target::Install() and there is no problem with the exec permission. spawnSubprocess() uses the class _RemoteProcess and finally Platform::Install(). spawnSubprocess() is used in 27 test files and they are failed is case of Windows host and Linux target. > Target::Install does (i.e., set the execute flag unconditionally) Target::Install() checks is_main_executable enumerating all modules. But Target::Install()'s logic is not applicable in most cases where Platform::Install() is used. I think `Target::Install` is not a workaround and we cannot remove this code. I'd say this patch is a workaround for the case host=Windows and target!=Windows. We can even add a comment FIXME:... if someone will have an idea how to fix it better way. https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
labath wrote: The `ifdef` in the generic code is not exactly ideal, and I'm wondering if we should just do the same thing as Target::Install does (i.e., set the execute flag unconditionally). Looking at the history, it appears that the Target::Install code was [introduced](https://reviews.llvm.org/D9492) to fix the same problem you are having, and it's quite possible this code path was not noticed because we were still in the very early stages of cross-debugging bringup (or the function did even not exist back then). In fact, if I'm reading this correctly, you should be able to remove the Target::Install workaround if you put the code here. WDYT? https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
llvmbot wrote: @llvm/pr-subscribers-lldb Author: Dmitry Vasilyev (slydiman) Changes Target::Install() set 0700 permissions for the main executable file. Platform::Install() just copies permissions from the source. But the permission eFilePermissionsUserExecute is missing on the Windows host. A lot of tests failed in case of Windows host and Linux target because of this issue. There is no API to provide the exec flag. This patch set the permission eFilePermissionsUserExecute for all files installed via Platform::Install() from the Windows host. It fixes a lot of tests in case of Windows host and Linux target. --- Full diff: https://github.com/llvm/llvm-project/pull/91887.diff 1 Files Affected: - (modified) lldb/source/Target/Platform.cpp (+4) ``diff diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index 4af4aa68ccd01..0e7739b3712d7 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -1227,6 +1227,10 @@ Status Platform::PutFile(const FileSpec , const FileSpec , if (permissions == 0) permissions = lldb::eFilePermissionsFileDefault; +#if defined(_WIN32) + permissions |= lldb::eFilePermissionsUserExecute; +#endif + lldb::user_id_t dest_file = OpenFile( destination, File::eOpenOptionCanCreate | File::eOpenOptionWriteOnly | File::eOpenOptionTruncate | File::eOpenOptionCloseOnExec, `` https://github.com/llvm/llvm-project/pull/91887 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host (PR #91887)
https://github.com/slydiman created https://github.com/llvm/llvm-project/pull/91887 Target::Install() set 0700 permissions for the main executable file. Platform::Install() just copies permissions from the source. But the permission eFilePermissionsUserExecute is missing on the Windows host. A lot of tests failed in case of Windows host and Linux target because of this issue. There is no API to provide the exec flag. This patch set the permission eFilePermissionsUserExecute for all files installed via Platform::Install() from the Windows host. It fixes a lot of tests in case of Windows host and Linux target. >From 94f75bd21aac412b9971ccc8dc8382e4a75dc1cd Mon Sep 17 00:00:00 2001 From: Dmitry Vasilyev Date: Sun, 12 May 2024 18:29:09 +0400 Subject: [PATCH] [lldb][Windows] Enforce exec permission using Platform::Install() from Windows host Target::Install() set 0700 permissions for the main executable file. Platform::Install() just copies permissions from the source. But the permission eFilePermissionsUserExecute is missing on the Windows host. A lot of tests failed in case of Windows host and Linux target because of this issue. There is no API to provide the exec flag. This patch set the permission eFilePermissionsUserExecute for all files installed via Platform::Install() from the Windows host. It fixes a lot of tests in case of Windows host and Linux target. --- lldb/source/Target/Platform.cpp | 4 1 file changed, 4 insertions(+) diff --git a/lldb/source/Target/Platform.cpp b/lldb/source/Target/Platform.cpp index 4af4aa68ccd01..0e7739b3712d7 100644 --- a/lldb/source/Target/Platform.cpp +++ b/lldb/source/Target/Platform.cpp @@ -1227,6 +1227,10 @@ Status Platform::PutFile(const FileSpec , const FileSpec , if (permissions == 0) permissions = lldb::eFilePermissionsFileDefault; +#if defined(_WIN32) + permissions |= lldb::eFilePermissionsUserExecute; +#endif + lldb::user_id_t dest_file = OpenFile( destination, File::eOpenOptionCanCreate | File::eOpenOptionWriteOnly | File::eOpenOptionTruncate | File::eOpenOptionCloseOnExec, ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
