Skip to content

[lldb-dap] Always pass disableASLR to the DAP executable #113891

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Oct 28, 2024

More context can be found in #110303

For DAP tests running in constrained environments (e.g., Docker
containers), disabling ASLR isn't allowed. So we set disableASLR=False
(since #113593).

However, the dap_server.py will currently only forward the value
of disableASLR to the DAP executable if it's set to True. If the
DAP executable wasn't provided a disableASLR field it defaults to
true too:

if (GetBoolean(arguments, "disableASLR", true))
flags |= lldb::eLaunchFlagDisableASLR;

This means that passing disableASLR=False from the tests is currently
not possible.

This is also true for many of the other boolean arguments of
request_launch. But this patch only addresses disableASLR for now
since it's blocking a libc++ patch.

Copy link

github-actions bot commented Oct 28, 2024

✅ With the latest revision this PR passed the Python code formatter.

@Michael137 Michael137 force-pushed the libcxx-ci-lldb-dap-fix branch from 7071fe8 to e835592 Compare October 28, 2024 17:11
@Michael137 Michael137 marked this pull request as ready for review October 28, 2024 17:12
@llvmbot llvmbot added libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. lldb github:workflow labels Oct 28, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 28, 2024

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-libcxx

Author: Michael Buch (Michael137)

Changes

Full diff: https://github.com/llvm/llvm-project/pull/113891.diff

2 Files Affected:

  • (modified) .github/workflows/libcxx-build-and-test.yaml (+11-8)
  • (modified) lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py (+1-2)
diff --git a/.github/workflows/libcxx-build-and-test.yaml b/.github/workflows/libcxx-build-and-test.yaml
index 184fed2268e818..3521b5d5a3def4 100644
--- a/.github/workflows/libcxx-build-and-test.yaml
+++ b/.github/workflows/libcxx-build-and-test.yaml
@@ -49,7 +49,8 @@ env:
 jobs:
   stage1:
     if: github.repository_owner == 'llvm'
-    runs-on: libcxx-runners-8-set
+    runs-on: libcxx-runners-set
+    container: ghcr.io/libcxx/actions-builder:testing-2024-09-21
     continue-on-error: false
     strategy:
       fail-fast: false
@@ -84,7 +85,8 @@ jobs:
             **/crash_diagnostics/*
   stage2:
     if: github.repository_owner == 'llvm'
-    runs-on: libcxx-runners-8-set
+    runs-on: libcxx-runners-set
+    container: ghcr.io/libcxx/actions-builder:testing-2024-09-21
     needs: [ stage1 ]
     continue-on-error: false
     strategy:
@@ -160,20 +162,21 @@ jobs:
           'benchmarks',
           'bootstrapping-build'
         ]
-        machine: [ 'libcxx-runners-8-set' ]
+        machine: [ 'libcxx-runners-set' ]
         include:
         - config: 'generic-cxx26'
-          machine: libcxx-runners-8-set
+          machine: libcxx-runners-set
         - config: 'generic-asan'
-          machine: libcxx-runners-8-set
+          machine: libcxx-runners-set
         - config: 'generic-tsan'
-          machine: libcxx-runners-8-set
+          machine: libcxx-runners-set
         - config: 'generic-ubsan'
-          machine: libcxx-runners-8-set
+          machine: libcxx-runners-set
         # Use a larger machine for MSAN to avoid timeout and memory allocation issues.
         - config: 'generic-msan'
-          machine: libcxx-runners-8-set
+          machine: libcxx-runners-set
     runs-on: ${{ matrix.machine }}
+    container: ghcr.io/libcxx/actions-builder:testing-2024-09-21
     steps:
       - uses: actions/checkout@v4
       - name: ${{ matrix.config }}
diff --git a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
index 63748a71f1122d..c29992ce9c7848 100644
--- a/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
+++ b/lldb/packages/Python/lldbsuite/test/tools/lldb-dap/dap_server.py
@@ -793,8 +793,6 @@ def request_launch(
             args_dict["env"] = env
         if stopOnEntry:
             args_dict["stopOnEntry"] = stopOnEntry
-        if disableASLR:
-            args_dict["disableASLR"] = disableASLR
         if disableSTDIO:
             args_dict["disableSTDIO"] = disableSTDIO
         if shellExpandArguments:
@@ -829,6 +827,7 @@ def request_launch(
         if customThreadFormat:
             args_dict["customThreadFormat"] = customThreadFormat
 
+        args_dict["disableASLR"] = disableASLR
         args_dict["enableAutoVariableSummaries"] = enableAutoVariableSummaries
         args_dict["enableSyntheticChildDebugging"] = enableSyntheticChildDebugging
         args_dict["displayExtendedBacktrace"] = displayExtendedBacktrace

@Michael137 Michael137 changed the title [DO NOT MERGE] Test libc++ CI LLDB DAP failures [lldb-dap] Always pass disableASLR to the DAP executable Oct 28, 2024
@Michael137
Copy link
Member Author

Confirmed the libc++ tests pass. Removing them from this PR

More context can be found in
llvm#110303

For DAP tests running in constrained environments (e.g., Docker
containers), disabling ASLR isn't allowed. So we set `disableASLR=False`
(since llvm#113593).

However, the `dap_server.py` will currently only forward the value
of `disableASLR` to the DAP executable if it's set to `True`. If the
DAP executable wasn't provided a `disableASLR` field it defaults to
`true` too
(https://github.com/llvm/llvm-project/blob/f14743794587db102c6d1b20f9c87a1ac20decfd/lldb/tools/lldb-dap/lldb-dap.cpp#L2103-L2104).

This means that passing `disableASLR=False` from the tests is currently
not possible.

This is also true for many of the other boolean arguments of
`request_launch`. But this patch only addresses `disableASLR` for now
since it's blocking a libc++ patch.
@Michael137 Michael137 force-pushed the libcxx-ci-lldb-dap-fix branch from e835592 to 5da7c34 Compare October 28, 2024 18:53
@Michael137
Copy link
Member Author

If we don't always want forward disableASLR we could instead flip the condition to:

if not disableASLR:
    args_dict["disableASLR"] = disableASLR

Any thoughts?

@augusto2112
Copy link
Contributor

If we don't always want forward disableASLR we could instead flip the condition to:

if not disableASLR:
    args_dict["disableASLR"] = disableASLR

Any thoughts?

I think "if not" would make it more confusing. The way it is in this patch seems fine to me

@Michael137 Michael137 merged commit b4e1af0 into llvm:main Oct 29, 2024
7 checks passed
@Michael137 Michael137 deleted the libcxx-ci-lldb-dap-fix branch October 29, 2024 18:40
NoumanAmir657 pushed a commit to NoumanAmir657/llvm-project that referenced this pull request Nov 4, 2024
More context can be found in
llvm#110303

For DAP tests running in constrained environments (e.g., Docker
containers), disabling ASLR isn't allowed. So we set `disableASLR=False`
(since llvm#113593).

However, the `dap_server.py` will currently only forward the value
of `disableASLR` to the DAP executable if it's set to `True`. If the
DAP executable wasn't provided a `disableASLR` field it defaults to
`true` too:
https://github.com/llvm/llvm-project/blob/f14743794587db102c6d1b20f9c87a1ac20decfd/lldb/tools/lldb-dap/lldb-dap.cpp#L2103-L2104

This means that passing `disableASLR=False` from the tests is currently
not possible.

This is also true for many of the other boolean arguments of
`request_launch`. But this patch only addresses `disableASLR` for now
since it's blocking a libc++ patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github:workflow libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. lldb lldb-dap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants