Skip to content

[lldb] Support alternatives for scope format entries #137751

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.

Sign up for GitHub

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 2 commits into from
May 5, 2025
Merged

[lldb] Support alternatives for scope format entries #137751

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6618a2
[lldb] Support alternatives for scope format entries
JDevlieghere Apr 29, 2025
717cbea
Use static helper function
JDevlieghere Apr 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions lldb/include/lldb/Core/FormatEntity.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,10 @@

#include "lldb/lldb-enumerations.h"
#include "lldb/lldb-types.h"
#include "llvm/ADT/SmallVector.h"
#include <algorithm>
#include <cstddef>
#include <cstdint>

#include <string>
#include <vector>

Expand Down Expand Up @@ -158,9 +158,7 @@ struct Entry {
}

Entry(Type t = Type::Invalid, const char *s = nullptr,
const char *f = nullptr)
: string(s ? s : ""), printf_format(f ? f : ""), type(t) {}

const char *f = nullptr);
Entry(llvm::StringRef s);
Entry(char ch);

Expand All @@ -170,15 +168,19 @@ struct Entry {

void AppendText(const char *cstr);

void AppendEntry(const Entry &&entry) { children.push_back(entry); }
void AppendEntry(const Entry &&entry);

void StartAlternative();

void Clear() {
string.clear();
printf_format.clear();
children.clear();
children_stack.clear();
children_stack.emplace_back();
type = Type::Invalid;
fmt = lldb::eFormatDefault;
number = 0;
level = 0;
deref = false;
}

Expand All @@ -191,7 +193,7 @@ struct Entry {
return false;
if (printf_format != rhs.printf_format)
return false;
if (children != rhs.children)
if (children_stack != rhs.children_stack)
return false;
if (type != rhs.type)
return false;
Expand All @@ -202,9 +204,18 @@ struct Entry {
return true;
}

std::vector<Entry> &GetChildren();

std::string string;
std::string printf_format;
std::vector<Entry> children;

/// A stack of children entries, used by Scope entries to provide alterantive
/// children. All other entries have a stack of size 1.
/// @{
llvm::SmallVector<std::vector<Entry>, 1> children_stack;
size_t level = 0;
/// @}

Type type;
lldb::Format fmt = lldb::eFormatDefault;
lldb::addr_t number = 0;
Expand Down
87 changes: 63 additions & 24 deletions lldb/source/Core/FormatEntity.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
#include "llvm/Support/Regex.h"
#include "llvm/TargetParser/Triple.h"

#include <cassert>
#include <cctype>
#include <cinttypes>
#include <cstdio>
Expand Down Expand Up @@ -281,31 +282,53 @@ constexpr Definition g_top_level_entries[] = {
constexpr Definition g_root = Entry::DefinitionWithChildren(
"<root>", EntryType::Root, g_top_level_entries);

FormatEntity::Entry::Entry(Type t, const char *s, const char *f)
: string(s ? s : ""), printf_format(f ? f : ""), children_stack({{}}),
type(t) {}

FormatEntity::Entry::Entry(llvm::StringRef s)
: string(s.data(), s.size()), printf_format(), children(),
type(Type::String) {}
: string(s.data(), s.size()), children_stack({{}}), type(Type::String) {}

FormatEntity::Entry::Entry(char ch)
: string(1, ch), printf_format(), children(), type(Type::String) {}
: string(1, ch), printf_format(), children_stack({{}}), type(Type::String) {
}

std::vector<Entry> &FormatEntity::Entry::GetChildren() {
assert(level < children_stack.size());
return children_stack[level];
}

void FormatEntity::Entry::AppendChar(char ch) {
if (children.empty() || children.back().type != Entry::Type::String)
children.push_back(Entry(ch));
auto &entries = GetChildren();
if (entries.empty() || entries.back().type != Entry::Type::String)
entries.push_back(Entry(ch));
else
children.back().string.append(1, ch);
entries.back().string.append(1, ch);
}

void FormatEntity::Entry::AppendText(const llvm::StringRef &s) {
if (children.empty() || children.back().type != Entry::Type::String)
children.push_back(Entry(s));
auto &entries = GetChildren();
if (entries.empty() || entries.back().type != Entry::Type::String)
entries.push_back(Entry(s));
else
children.back().string.append(s.data(), s.size());
entries.back().string.append(s.data(), s.size());
}

void FormatEntity::Entry::AppendText(const char *cstr) {
return AppendText(llvm::StringRef(cstr));
}

void FormatEntity::Entry::AppendEntry(const Entry &&entry) {
auto &entries = GetChildren();
entries.push_back(entry);
}

void FormatEntity::Entry::StartAlternative() {
assert(type == Entry::Type::Scope);
children_stack.emplace_back();
level++;
}

#define ENUM_TO_CSTR(eee) \
case FormatEntity::Entry::Type::eee: \
return #eee
Expand Down Expand Up @@ -405,8 +428,9 @@ void FormatEntity::Entry::Dump(Stream &s, int depth) const {
if (deref)
s.Printf("deref = true, ");
s.EOL();
for (const auto &child : children) {
child.Dump(s, depth + 1);
for (const auto &children : children_stack) {
for (const auto &child : children)
child.Dump(s, depth + 1);
}
}

Expand Down Expand Up @@ -1308,7 +1332,7 @@ bool FormatEntity::Format(const Entry &entry, Stream &s,
return true;

case Entry::Type::Root:
for (const auto &child : entry.children) {
for (const auto &child : entry.children_stack[0]) {
if (!Format(child, s, sc, exe_ctx, addr, valobj, function_changed,
initial_function)) {
return false; // If any item of root fails, then the formatting fails
Expand All @@ -1322,19 +1346,26 @@ bool FormatEntity::Format(const Entry &entry, Stream &s,

case Entry::Type::Scope: {
StreamString scope_stream;
bool success = false;
for (const auto &child : entry.children) {
success = Format(child, scope_stream, sc, exe_ctx, addr, valobj,
function_changed, initial_function);
if (!success)
break;
auto format_children = [&](const std::vector<Entry> &children) {
scope_stream.Clear();
for (const auto &child : children) {
if (!Format(child, scope_stream, sc, exe_ctx, addr, valobj,
function_changed, initial_function))
return false;
}
return true;
};

for (auto &children : entry.children_stack) {
if (format_children(children)) {
s.Write(scope_stream.GetString().data(),
scope_stream.GetString().size());
return true;
}
}
// Only if all items in a scope succeed, then do we print the output into
// the main stream
if (success)
s.Write(scope_stream.GetString().data(), scope_stream.GetString().size());
}

return true; // Scopes always successfully print themselves
}

case Entry::Type::Variable:
case Entry::Type::VariableSynthetic:
Expand Down Expand Up @@ -2124,7 +2155,7 @@ static Status ParseInternal(llvm::StringRef &format, Entry &parent_entry,
uint32_t depth) {
Status error;
while (!format.empty() && error.Success()) {
const size_t non_special_chars = format.find_first_of("${}\\");
const size_t non_special_chars = format.find_first_of("${}\\|");

if (non_special_chars == llvm::StringRef::npos) {
// No special characters, just string bytes so add them and we are done
Expand Down Expand Up @@ -2161,6 +2192,14 @@ static Status ParseInternal(llvm::StringRef &format, Entry &parent_entry,
.drop_front(); // Skip the '}' as we are at the end of the scope
return error;

case '|':
format = format.drop_front(); // Skip the '|'
if (parent_entry.type == Entry::Type::Scope)
parent_entry.StartAlternative();
else
parent_entry.AppendChar('|');
break;

case '\\': {
format = format.drop_front(); // Skip the '\' character
if (format.empty()) {
Expand Down
59 changes: 53 additions & 6 deletions lldb/unittests/Core/FormatEntityTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,36 @@

#include "lldb/Core/FormatEntity.h"
#include "lldb/Utility/Status.h"

#include "lldb/Utility/StreamString.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Support/Error.h"
#include "llvm/Testing/Support/Error.h"
#include "gtest/gtest.h"

using namespace lldb_private;
using namespace llvm;

using Definition = FormatEntity::Entry::Definition;
using Entry = FormatEntity::Entry;

TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
namespace {
class FormatEntityTest : public ::testing::Test {
public:
Expected<std::string> Format(StringRef format_str) {
StreamString stream;
FormatEntity::Entry format;
Status status = FormatEntity::Parse(format_str, format);
if (status.Fail())
return status.ToError();

FormatEntity::Format(format, stream, nullptr, nullptr, nullptr, nullptr,
false, false);
return stream.GetString().str();
}
};
} // namespace

TEST_F(FormatEntityTest, DefinitionConstructionNameAndType) {
Definition d("foo", FormatEntity::Entry::Type::Invalid);

EXPECT_STREQ(d.name, "foo");
Expand All @@ -29,7 +49,7 @@ TEST(FormatEntityTest, DefinitionConstructionNameAndType) {
EXPECT_FALSE(d.keep_separator);
}

TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
TEST_F(FormatEntityTest, DefinitionConstructionNameAndString) {
Definition d("foo", "string");

EXPECT_STREQ(d.name, "foo");
Expand All @@ -41,7 +61,7 @@ TEST(FormatEntityTest, DefinitionConstructionNameAndString) {
EXPECT_FALSE(d.keep_separator);
}

TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
TEST_F(FormatEntityTest, DefinitionConstructionNameTypeData) {
Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);

EXPECT_STREQ(d.name, "foo");
Expand All @@ -53,7 +73,7 @@ TEST(FormatEntityTest, DefinitionConstructionNameTypeData) {
EXPECT_FALSE(d.keep_separator);
}

TEST(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
TEST_F(FormatEntityTest, DefinitionConstructionNameTypeChildren) {
Definition d("foo", FormatEntity::Entry::Type::Invalid, 33);
Definition parent("parent", FormatEntity::Entry::Type::Invalid, 1, &d);
EXPECT_STREQ(parent.name, "parent");
Expand Down Expand Up @@ -153,10 +173,37 @@ constexpr llvm::StringRef lookupStrings[] = {
"${target.file.fullpath}",
"${var.dummy-var-to-test-wildcard}"};

TEST(FormatEntity, LookupAllEntriesInTree) {
TEST_F(FormatEntityTest, LookupAllEntriesInTree) {
for (const llvm::StringRef testString : lookupStrings) {
Entry e;
EXPECT_TRUE(FormatEntity::Parse(testString, e).Success())
<< "Formatting " << testString << " did not succeed";
}
}

TEST_F(FormatEntityTest, Scope) {
// Scope with one alternative.
EXPECT_THAT_EXPECTED(Format("{${frame.pc}|foo}"), HasValue("foo"));

// Scope with multiple alternatives.
EXPECT_THAT_EXPECTED(Format("{${frame.pc}|${function.name}|foo}"),
HasValue("foo"));

// Escaped pipe inside a scope.
EXPECT_THAT_EXPECTED(Format("{foo\\|bar}"), HasValue("foo|bar"));

// Unescaped pipe outside a scope.
EXPECT_THAT_EXPECTED(Format("foo|bar"), HasValue("foo|bar"));

// Nested scopes. Note that scopes always resolve.
EXPECT_THAT_EXPECTED(Format("{{${frame.pc}|foo}|{bar}}"), HasValue("foo"));
EXPECT_THAT_EXPECTED(Format("{{${frame.pc}}|{bar}}"), HasValue(""));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this resolve to bar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because a scope always resolves successfully, no matter what's in it, this behaves as expected.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured this was a weird case which is why I included it :-)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh so within {${frame.pc}}, ${frame.pc} would fail, but the entire scope is seen as "successfully resolved"? Hence it doesn't fall back to the other scope?

Does that mean the fallback can't ever be used between scopes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be difficult to make a scope fail if all its children failed? That would be more intuitive. But I suspect it might be a breaking change we're not willing to make?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would break literally every existing use of scopes because the only reason to use them was to make expansion failures non-fatal. I'm not saying we can't do that, but I think that's what it is. We could try to split hairs and say this only applies to scopes with more than one alternative, but I don't know if it's worth it. I suppose it might be used to write something like {common prefix {${foo}|${bar}}| alternative prefix}, but I don't know if there's a realistic use case for that.

Copy link
Member

@Michael137 Michael137 Apr 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we maybe disallow alternatives to be used between scopes (or rather where a scope is on the left-hand side)? I guess technically it does "work", but one would have to know the intricacies of this language to understand why. Would there ever be a case where writing { { foo } | { bar } } would not be a bug?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on the fence. It's probably not too hard to implement (although you need to be careful to only trigger this for an alternative scope, a regular nested scope is okay). On the other hand, if you know how a scope works, I think this behaves pretty much as expected, it's just a rather unhelpful thing to do. I don't think we have precedence for diagnosing something like this. I can see how "clean" or "ugly" it is to implement and that might sway me one way or another.

Copy link
Member

@Michael137 Michael137 May 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. If it's a pain to implement I won't insist. There is precedent for diagnosing invalid syntax (like mismatching curly braces). But maybe that's not the level at which this diagnostic would need to be implemented


// Pipe between scopes.
EXPECT_THAT_EXPECTED(Format("{foo}|{bar}"), HasValue("foo|bar"));
EXPECT_THAT_EXPECTED(Format("{foo}||{bar}"), HasValue("foo||bar"));

// Empty space between pipes.
EXPECT_THAT_EXPECTED(Format("{{foo}||{bar}}"), HasValue("foo"));
EXPECT_THAT_EXPECTED(Format("{${frame.pc}||{bar}}"), HasValue(""));
}
Loading