Make env and source map dictionaries #95137 #106919
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-lldb Author: None (Da-Viper) ChangesFixes #95137 Full diff: https://github.com/llvm/llvm-project/pull/106919.diff 5 Files Affected:
diff --git a/lldb/tools/lldb-dap/JSONUtils.cpp b/lldb/tools/lldb-dap/JSONUtils.cpp
index 7338e7cf41eb03..333c0b21e57d01 100644
--- a/lldb/tools/lldb-dap/JSONUtils.cpp
+++ b/lldb/tools/lldb-dap/JSONUtils.cpp
@@ -136,6 +136,31 @@ std::vector<std::string> GetStrings(const llvm::json::Object *obj,
return strs;
}
+std::unordered_map<std::string, std::string>
+GetStringObject(const llvm::json::Object &obj, llvm::StringRef key) {
+ std::unordered_map<std::string, std::string> strs;
+ const auto *const json_object = obj.getObject(key);
+ if (!json_object)
+ return strs;
+
+ for (const auto &[key, value] : *json_object) {
+ switch (value.kind()) {
+ case llvm::json::Value::String:
+ strs.emplace(key.str(), value.getAsString()->str());
+ break;
+ case llvm::json::Value::Number:
+ case llvm::json::Value::Boolean:
+ strs.emplace(key.str(), llvm::to_string(value));
+ break;
+ case llvm::json::Value::Null:
+ case llvm::json::Value::Object:
+ case llvm::json::Value::Array:
+ break;
+ }
+ }
+ return strs;
+}
+
static bool IsClassStructOrUnionType(lldb::SBType t) {
return (t.GetTypeClass() & (lldb::eTypeClassUnion | lldb::eTypeClassStruct |
lldb::eTypeClassArray)) != 0;
@@ -1370,13 +1395,15 @@ CreateRunInTerminalReverseRequest(const llvm::json::Object &launch_request,
if (!cwd.empty())
run_in_terminal_args.try_emplace("cwd", cwd);
- // We need to convert the input list of environments variables into a
- // dictionary
- std::vector<std::string> envs = GetStrings(launch_request_arguments, "env");
+ std::unordered_map<std::string, std::string> envMap =
+ GetStringObject(*launch_request_arguments, "env");
llvm::json::Object environment;
- for (const std::string &env : envs) {
- size_t index = env.find('=');
- environment.try_emplace(env.substr(0, index), env.substr(index + 1));
+ for (const auto &[key, value] : envMap) {
+ if (key.empty())
+ g_dap.SendOutput(OutputType::Stderr,
+ "empty environment variable for value: \"" + value + '\"');
+ else
+ environment.try_emplace(key, value);
}
run_in_terminal_args.try_emplace("env",
llvm::json::Value(std::move(environment)));
diff --git a/lldb/tools/lldb-dap/JSONUtils.h b/lldb/tools/lldb-dap/JSONUtils.h
index b6356630b72682..de6228a0429944 100644
--- a/lldb/tools/lldb-dap/JSONUtils.h
+++ b/lldb/tools/lldb-dap/JSONUtils.h
@@ -16,6 +16,7 @@
#include "llvm/Support/JSON.h"
#include <cstdint>
#include <optional>
+#include <unordered_map>
namespace lldb_dap {
@@ -151,6 +152,26 @@ bool ObjectContainsKey(const llvm::json::Object &obj, llvm::StringRef key);
/// strings, numbers or booleans.
std::vector<std::string> GetStrings(const llvm::json::Object *obj,
llvm::StringRef key);
+/// Extract an object of key value strings for the specified key from an object.
+///
+/// String values in the array will be extracted without any quotes
+/// around them. Numbers and Booleans will be converted into
+/// strings. Any NULL, array or objects values in the array will be
+/// ignored.
+///
+/// \param[in] obj
+/// A JSON object that we will attempt to extract the array from
+///
+/// \param[in] key
+/// The key to use when extracting the value
+///
+/// \return
+/// An object of key value strings for the specified \a key, or
+/// \a fail_value if there is no key that matches or if the
+/// value is not an object or key and values in the object are not
+/// strings, numbers or booleans.
+std::unordered_map<std::string, std::string>
+GetStringObject(const llvm::json::Object &obj, llvm::StringRef key);
/// Fill a response object given the request object.
///
diff --git a/lldb/tools/lldb-dap/README.md b/lldb/tools/lldb-dap/README.md
index 11a14d29ab51e2..c4f48001dad19a 100644
--- a/lldb/tools/lldb-dap/README.md
+++ b/lldb/tools/lldb-dap/README.md
@@ -34,7 +34,7 @@ file that defines how your program will be run. The JSON configuration file can
|**launchCommands** |[string]| | LLDB commands executed to launch the program. Commands and command output will be sent to the debugger console when they are executed.
|**exitCommands** |[string]| | LLDB commands executed when the program exits. Commands and command output will be sent to the debugger console when they are executed.
|**terminateCommands** |[string]| | LLDB commands executed when the debugging session ends. Commands and command output will be sent to the debugger console when they are executed.
-|**sourceMap** |[string[2]]| | Specify an array of path re-mappings. Each element in the array must be a two element array containing a source and destination pathname.
+|**sourceMap** |dictionary| | Specify an array of path re-mappings. Each element in the array must be a two element array containing a source and destination pathname.
|**debuggerRoot** | string| |Specify a working directory to use when launching lldb-dap. If the debug information in your executable contains relative paths, this option can be used so that `lldb-dap` can find source files and object files that have relative paths.
### Attaching Settings
@@ -77,7 +77,10 @@ adds `FOO=1` and `bar` to the environment:
"name": "Debug",
"program": "/tmp/a.out",
"args": [ "one", "two", "three" ],
- "env": [ "FOO=1", "BAR" ],
+ "env": {
+ "FOO": "1"
+ "BAR": ""
+ }
}
```
diff --git a/lldb/tools/lldb-dap/lldb-dap.cpp b/lldb/tools/lldb-dap/lldb-dap.cpp
index c5c4b09f15622b..59957344ff7f50 100644
--- a/lldb/tools/lldb-dap/lldb-dap.cpp
+++ b/lldb/tools/lldb-dap/lldb-dap.cpp
@@ -599,27 +599,28 @@ lldb::SBValue FindVariable(uint64_t variablesReference, llvm::StringRef name) {
// argument (or neither), from which we need to set the target.source-map.
void SetSourceMapFromArguments(const llvm::json::Object &arguments) {
const char *sourceMapHelp =
- "source must be be an array of two-element arrays, "
- "each containing a source and replacement path string.\n";
+ "source must be be an object of key-value strings. "
+ "e.g sourceMap: { \"/path/to/source\": \"/path/to/destination\" }.\n";
std::string sourceMapCommand;
llvm::raw_string_ostream strm(sourceMapCommand);
strm << "settings set target.source-map ";
- auto sourcePath = GetString(arguments, "sourcePath");
+ const auto sourcePath = GetString(arguments, "sourcePath");
// sourceMap is the new, more general form of sourcePath and overrides it.
- auto sourceMap = arguments.getArray("sourceMap");
- if (sourceMap) {
- for (const auto &value : *sourceMap) {
- auto mapping = value.getAsArray();
- if (mapping == nullptr || mapping->size() != 2 ||
- (*mapping)[0].kind() != llvm::json::Value::String ||
- (*mapping)[1].kind() != llvm::json::Value::String) {
+ const auto *sourceMap = arguments.getObject("sourceMap");
+ if (sourceMap && !sourceMap->empty()) {
+ for (const auto &[key, value] : *sourceMap) {
+ const auto mapFrom = llvm::StringRef(key);
+ if (mapFrom.empty()) {
+ return;
+ }
+ if (value.kind() != llvm::json::Value::String) {
g_dap.SendOutput(OutputType::Console, llvm::StringRef(sourceMapHelp));
return;
}
- auto mapFrom = GetAsString((*mapping)[0]);
- auto mapTo = GetAsString((*mapping)[1]);
+
+ const llvm::StringRef mapTo = GetAsString(value);
strm << "\"" << mapFrom << "\" \"" << mapTo << "\" ";
}
} else {
@@ -1831,7 +1832,14 @@ lldb::SBError LaunchProcess(const llvm::json::Object &request) {
launch_info.SetArguments(MakeArgv(args).data(), true);
// Pass any environment variables along that the user specified.
- auto envs = GetStrings(arguments, "env");
+ auto envMap = GetStringObject(*arguments, "env");
+ std::vector<std::string> envs;
+ envs.reserve(envMap.size());
+ for (const auto &[key, value] : envMap) {
+ if (!key.empty())
+ envs.emplace_back(key + '=' + value);
+ }
+
if (!envs.empty())
launch_info.SetEnvironmentEntries(MakeArgv(envs).data(), true);
diff --git a/lldb/tools/lldb-dap/package.json b/lldb/tools/lldb-dap/package.json
index 4f4261d1718c01..1404a1f29c6bb0 100644
--- a/lldb/tools/lldb-dap/package.json
+++ b/lldb/tools/lldb-dap/package.json
@@ -160,9 +160,14 @@
"default": "${workspaceRoot}"
},
"env": {
- "type": "array",
- "description": "Additional environment variables to set when launching the program. This is an array of strings that contains the variable name followed by an optional '=' character and the environment variable's value.",
- "default": []
+ "type": "object",
+ "description": "Additional environment variables to set when launching the program. eg { \"FOO\": \"1\" }",
+ "patternProperties": {
+ ".*": {
+ "type": "string"
+ }
+ },
+ "default": {}
},
"stopOnEntry": {
"type": "boolean",
@@ -194,9 +199,14 @@
"description": "Specify a source path to remap \"./\" to allow full paths to be used when setting breakpoints in binaries that have relative source paths."
},
"sourceMap": {
- "type": "array",
- "description": "Specify an array of path remappings; each element must itself be a two element array containing a source and destination path name. Overrides sourcePath.",
- "default": []
+ "type": "object",
+ "description": "Specify an object of path remappings; each entry has a key containing the source path and a value containing the destination path. E.g { \"/the/source/path\": \"/the/destination/path\" } Overrides sourcePath.",
+ "patternProperties": {
+ ".*": {
+ "type": "string"
+ }
+ },
+ "default": {}
},
"debuggerRoot": {
"type": "string",
@@ -299,9 +309,14 @@
"description": "Specify a source path to remap \"./\" to allow full paths to be used when setting breakpoints in binaries that have relative source paths."
},
"sourceMap": {
- "type": "array",
- "description": "Specify an array of path remappings; each element must itself be a two element array containing a source and destination path name. Overrides sourcePath.",
- "default": []
+ "type": "object",
+ "description": "Specify an object of path remappings; each entry has a key containing the source path and a value containing the destination path. E.g { \"/the/source/path\": \"/the/destination/path\" } Overrides sourcePath.",
+ "patternProperties": {
+ ".*": {
+ "type": "string"
+ }
+ },
+ "default": {}
},
"debuggerRoot": {
"type": "string",
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
vogelsgesang
reviewed
Sep 2, 2024
|
@vogelsgesang I am not sure if it possible to use a dict for sourceMaps since a source can map to multiple destination see test Using a source map as a dictionary will invalidate the previous set destination. Makes sense to revert just the sourcemap for now |
da-viper
force-pushed
the
Make-env-and-sourceMap-dictionaries-#95137
branch
from
f0a3d5d to
d2bddca
Compare
|
✅ With the latest revision this PR passed the Python code formatter. |
vogelsgesang
reviewed
Sep 2, 2024
Interestingly, CodeLLDB also uses an object, see https://github.com/vadimcn/codelldb/blob/v1.10.0/MANUAL.md#source-path-remapping. Probably they just don't support mapping a path to multiple source paths. I guess we want to stay with the "array-of-array" configuration, then. What do you think, @JDevlieghere |
|
Can we modify this patch to support both formats? It should be easy to see if "env" is a dictionary, and if it is, use your new code. If it is an array, use the old parsing code. I would like to make sure this change doesn't break anyone that has existing launch configurations. |
Should the older one be marked as deprecated or we just support both and test both ? |
JDevlieghere
approved these changes
Sep 5, 2024
walter-erquinigo
requested changes
Sep 10, 2024
walter-erquinigo
approved these changes
Sep 13, 2024
Will do this evening |
|
@walter-erquinigo Is there anything missing for this pull request ? |
|
@da-viper do you have the necessary permissions to merge PRs by yourself? Or do you need somebody to merge this on your behalf? |
|
|
@da-viper , you should request commit access! |
|
see https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access for instructions |
|
Hi! Looks like this patch broke lldb-arm-ubuntu bot :
|
|
I suspect a missing unsupported annotation: I've gone ahead and added the same skip 98c9c1a. Though I am curious why these tests are so x86 specific. Windows too, but we don't see a failure on Windows on Arm (unless somehow x86 emulation is making it work). |
adrian-prantl
pushed a commit
to adrian-prantl/llvm-project
that referenced
this pull request
Dec 20, 2024
Fixes llvm#95137 (cherry picked from commit d4c1789) Conflicts: lldb/test/API/tools/lldb-dap/runInTerminal/TestDAP_runInTerminal.py
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #95137