[lldb-dap] Fix: disableASLR launch argument not working. #129753

da-viper · 2025-03-04T17:48:34Z

Fixes #94338

disable_aslr.mp4

i used pidof a.out | xargs pmap to show the memory map of the binary

llvmbot · 2025-03-04T17:49:08Z

@llvm/pr-subscribers-lldb

Author: None (Da-Viper)

Changes

Fixes #94338

disable_aslr.mp4

i used pidof a.out | xargs pmap to show the memory map of the binary

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

1 Files Affected:

(modified) lldb/tools/lldb-dap/Handler/RequestHandler.cpp (+3)

diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
index 606ada90ce2e5..f56821ec03e27 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.cpp
@@ -175,6 +175,9 @@ RequestHandler::LaunchProcess(const llvm::json::Object &request) const {
 
   if (GetBoolean(arguments, "disableASLR", true))
     flags |= lldb::eLaunchFlagDisableASLR;
+  else
+    flags &= ~lldb::eLaunchFlagDisableASLR;
+
   if (GetBoolean(arguments, "disableSTDIO", false))
     flags |= lldb::eLaunchFlagDisableSTDIO;
   if (GetBoolean(arguments, "shellExpandArguments", false))

JDevlieghere

I'd like to suggest two improvements:

Add an overload to GetBoolean that returns an std::optional<bool> so we know if the value was not set, or explicitly set to true or false.
Have a helper that does the same thing for all the flags: set or clear it depending on whether the value was set, or do nothing if it wasn't set.

WDYT?

da-viper · 2025-03-04T21:07:23Z

I'd like to suggest two improvements:

Add an overload to GetBoolean that returns an std::optional<bool> so we know if the value was not set, or explicitly set to true or false.

Have a helper that does the same thing for all the flags: set or clear it depending on whether the value was set, or do nothing if it wasn't set.

WDYT?

I am not sure if it is useful since the boolean will have to be either true or false.
Will the helper functions be scoped to the RequestHandler.cpp class and be something like

   SetMask(uint32_t &flags, LaunchInfo mask);
   ClearMask(uint32_t &flags, LaunchInfo mask);

JDevlieghere · 2025-03-04T21:49:33Z

I am not sure if it is useful since the boolean will have to be either true or false.

Okay, just to confirm, you're saying that's it's not possible for these values not to be set?

Will the helper functions be scoped to the RequestHandler.cpp class and be something like

I was thinking something like:

SetLaunchFlag(llvm::StringRef key, uint32_t flag, bool default) {
    if (GetBoolean(arguments, key, default))
     flags |= flag;
   else
     flags &= ~flag;
}
// Used like this:
SetLaunchFlag("disableASLR", lldb::eLaunchFlagDisableASLR, true);
SetLaunchFlag("disableSTDIO", lldb::eLaunchFlagDisableSTDIO, false);

Edit: this could probably be a lambda.

da-viper · 2025-03-04T22:22:25Z

Okay, just to confirm, you're saying that's it's not possible for these values not to be set?

Yes because lldb has default values for all its settings. using settings show ....
Also internally when fetching a value for the first time it uses a predefined default if it is not set.

see

llvm-project/lldb/source/Target/TargetProperties.td

Lines 143 to 145 in a12744f

    
           def DisableASLR: Property<"disable-aslr", "Boolean">, 
        
             DefaultTrue, 
        
             Desc<"Disable Address Space Layout Randomization (ASLR)">;

walter-erquinigo · 2025-03-04T22:42:09Z

I kind of agree with Jonas' initial remark. Wouldn't it be better to conceptually have something like this:

if (HasBoolean(arguments, key)) {
  if (GetBoolean(arguments, key))
     flags |= flag;
   else
     flags &= ~flag;
}

that way you can let LLDB set the flag value on its own. An interesting case when this might be useful is if one of the initCommands sets the default value for this setting, then you don't want to change this unless the disableASLR field in the debug config is set explicitly

da-viper · 2025-03-04T22:50:37Z

Alright.

One more thing if it is better to return a flag from function or pass a reference.

static setLaunchFlag(uint32_t &flags, const llvm::json::Object *obj,
                              llvm::StringRef key, lldb::LaunchFlags mask,
                              bool default_value) {
  if (HasBoolean(obj, key))
    if (GetBoolean(obj, key, default_value))
      flags |= mask;
    else
      flags &= ~mask;
}

setLaunchFlag(flags, arguments, "disableASLR",
                        lldb::eLaunchFlagDisableASLR, false);

vs

static uint32_t setLaunchFlag(uint32_t flags, const llvm::json::Object *obj,
                              llvm::StringRef key, lldb::LaunchFlags mask,
                              bool default_value) {
  if (HasBoolean(obj, key))
    if (GetBoolean(obj, key, default_value))
      flags |= mask;
    else
      flags &= ~mask;

  return flags;
}

flags = setLaunchFlag(flags, arguments, "disableASLR",
                        lldb::eLaunchFlagDisableASLR, false);

JDevlieghere · 2025-03-04T22:56:00Z

I don't think we need to have both the HasBoolean and GetBoolean. I think we should rewrite GetBoolean from this:

bool GetBoolean(const llvm::json::Object &obj, llvm::StringRef key,
                bool fail_value) {
  if (auto value = obj.getBoolean(key))
    return *value;
  if (auto value = obj.getInteger(key))
    return *value != 0;
  return fail_value;
}

to this:

std::optional<bool> GetBoolean(const llvm::json::Object &obj, llvm::StringRef key) {
    if (auto value = obj.getBoolean(key))
    return *value;
  if (auto value = obj.getInteger(key))
    return *value != 0;
  return std::nullopt;
}

All existing uses of GetBoolean(obj, key, fail_value) can be rewritten as GetBoolean(obj, key).value_or(fail_value).

FWIW this applies to all the GetFoo helpers in JSONUtils.cpp, but obviously that should be a separate PR and I'm not trying to sign up for that work. I think we can add the overload for GetBoolean as part of this PR and cleanup the rest in another one.

walter-erquinigo · 2025-03-04T23:12:25Z

Oh yes, I completely agree with std::optional<bool> being the best choice. My example was just something conceptual and easy to read.
And indeed, as you say, all the fields should return an optional because the client just sends a blob of JSON and you can't make any guarantees on whether it makes sense or not.

walter-erquinigo

requesting changes given the existing discussion

JDevlieghere · 2025-03-05T04:16:17Z

#129818

Return a std::optional<bool> from GetBoolean so you can distinguish between the value not being present and it being explicitly set to true or false. All existing uses are replaced by calling `value_or(fail_value`). Motivated by #129753

da-viper · 2025-03-05T10:38:10Z

Different timezone, updated the commit with the new changes.

JDevlieghere · 2025-03-05T16:38:43Z

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

+static uint32_t SetLaunchFlag(uint32_t flags, const llvm::json::Object *obj,
+                              llvm::StringRef key, lldb::LaunchFlags mask,
+                              bool default_value) {
+  if (GetBoolean(obj, key).value_or(default_value))
+    flags |= mask;
+  else
+    flags &= ~mask;
+
+  return flags;
+}


This still doesn't distinguish between the value not being set and the value being set to true or false. Here's what I was suggesting:

Suggested change

static uint32_t SetLaunchFlag(uint32_t flags, const llvm::json::Object *obj,

llvm::StringRef key, lldb::LaunchFlags mask,

bool default_value) {

if (GetBoolean(obj, key).value_or(default_value))

flags |= mask;

else

flags &= ~mask;

return flags;

}

static uint32_t SetLaunchFlag(uint32_t flags, const llvm::json::Object *obj,

llvm::StringRef key, lldb::LaunchFlags mask) {

if (auto b = GetBoolean(obj, key)) {

if (*b)

flags |= mask;

else

flags &= ~mask;

}

return flags;

}

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

JDevlieghere

LGTM. Thanks for bearing with me.

lldb/tools/lldb-dap/Handler/RequestHandler.cpp

da-viper requested a review from JDevlieghere as a code owner March 4, 2025 17:48

llvmbot added lldb lldb-dap labels Mar 4, 2025

JDevlieghere reviewed Mar 4, 2025

View reviewed changes

walter-erquinigo requested changes Mar 4, 2025

View reviewed changes

JDevlieghere mentioned this pull request Mar 5, 2025

[lldb-dap] Return a std::optional<bool> from GetBoolean (NFC) #129818

Merged

[lldb-dap] Fix: disableASLR launch argument not working.

2f773de

da-viper force-pushed the fix-disableAslr-launchArgument#94338 branch from 22aede0 to 2f773de Compare March 5, 2025 10:35

JDevlieghere reviewed Mar 5, 2025

View reviewed changes

[lldb] [lldb-dap] do not set a flag if it is not in json object.

2486994

JDevlieghere approved these changes Mar 5, 2025

View reviewed changes

lldb/tools/lldb-dap/Handler/RequestHandler.cpp Outdated Show resolved Hide resolved

[lldb] [lldb-dap] fix dangling-else warning

e5c1312

JDevlieghere merged commit 5d8b4ea into llvm:main Mar 5, 2025
8 of 9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lldb-dap] Fix: disableASLR launch argument not working. #129753

[lldb-dap] Fix: disableASLR launch argument not working. #129753

da-viper commented Mar 4, 2025

llvmbot commented Mar 4, 2025

JDevlieghere left a comment

da-viper commented Mar 4, 2025 •

edited

Loading

JDevlieghere commented Mar 4, 2025 •

edited

Loading

da-viper commented Mar 4, 2025

walter-erquinigo commented Mar 4, 2025

da-viper commented Mar 4, 2025 •

edited

Loading

JDevlieghere commented Mar 4, 2025

walter-erquinigo commented Mar 4, 2025

walter-erquinigo left a comment

JDevlieghere commented Mar 5, 2025

da-viper commented Mar 5, 2025

JDevlieghere Mar 5, 2025

JDevlieghere left a comment

[lldb-dap] Fix: disableASLR launch argument not working. #129753

[lldb-dap] Fix: disableASLR launch argument not working. #129753

Conversation

da-viper commented Mar 4, 2025

llvmbot commented Mar 4, 2025

JDevlieghere left a comment

Choose a reason for hiding this comment

da-viper commented Mar 4, 2025 • edited Loading

JDevlieghere commented Mar 4, 2025 • edited Loading

da-viper commented Mar 4, 2025

walter-erquinigo commented Mar 4, 2025

da-viper commented Mar 4, 2025 • edited Loading

JDevlieghere commented Mar 4, 2025

walter-erquinigo commented Mar 4, 2025

walter-erquinigo left a comment

Choose a reason for hiding this comment

JDevlieghere commented Mar 5, 2025

da-viper commented Mar 5, 2025

JDevlieghere Mar 5, 2025

Choose a reason for hiding this comment

JDevlieghere left a comment

Choose a reason for hiding this comment

da-viper commented Mar 4, 2025 •

edited

Loading

JDevlieghere commented Mar 4, 2025 •

edited

Loading

da-viper commented Mar 4, 2025 •

edited

Loading