Skip to content

Conversation

@kastiglione
Copy link

@kastiglione kastiglione commented Dec 3, 2025

Adopt stringForPrintObject(_:mangledTypeName:), an overload added in swiftlang/swift#84742.

This new overload allows lldb to call stringForPrintObject using only a pointer referencing the value, and the mangled typename corresponding to the value. The resulting expression does not reference the concrete type.

By not referencing a value's type, po can be performed on variables (and their nested stored properties) without loading the modules corresponding to the value's type. This will improve performance and reliability of po.

rdar://158968103

Adopt `stringForPrintObject(_:mangledTypeName:)`, an overload added in
swiftlang/swift#84742.

This new overload allows lldb to call `stringForPrintObject` using only a pointer
referencing the value, and the mangled typename corresponding to the value. The
resulting expression does not reference the concrete type.

By not referencing a value's type, `po` can be performed on variables (and their nested
stored properties) without loading the modules corresponding to the value's type. This
will improve performance and reliability of `po`.
@kastiglione kastiglione requested a review from a team as a code owner December 3, 2025 00:00
@kastiglione
Copy link
Author

@swift-ci test

@kastiglione kastiglione marked this pull request as draft December 3, 2025 00:00
@kastiglione
Copy link
Author

Draft mode: follow up will include experimental setting, and tests.

@kastiglione
Copy link
Author

@swift-ci test

@kastiglione
Copy link
Author

@swift-ci test macOS

@kastiglione
Copy link
Author

Existing tests all pass with new po.

@kastiglione kastiglione marked this pull request as ready for review December 3, 2025 23:38
@kastiglione
Copy link
Author

@swift-ci test

)
self.expect("po value", substrs=["2025"])
self._filecheck("INT")
# CHECK-INT: stringForPrintObject(UnsafeRawPointer(bitPattern: {{[0-9]+}}), mangledTypeName: "SiD")

Choose a reason for hiding this comment

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

Neat test, reusing filecheck this way!

llvm::VersionTuple version = process_sp->GetHostOSVersion();
os_vers << version.getAsString();
auto platform = target->GetPlatform();
bool is_simulator = platform->GetPluginName().ends_with("-simulator");

Choose a reason for hiding this comment

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

Do we really not have a nicer way to tell if a Platform is a simulator?

Copy link
Author

Choose a reason for hiding this comment

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

There's Triple::isSimulatorEnvironment but it's unclear if that applies here, based on the comment:

// The simulators look like the host OS to Process, but Platform
// can get the version out of an environment variable.


bool GetPreparePlaygroundStubFunctions() const { return m_prepare_playground_stub_functions; }

void SetDisableAvailability() { m_disable_availability = true; }

Choose a reason for hiding this comment

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

This is an odd set API. It only allows you to set it to true, which the name doesn't really imply?

We usually do:

SetWhatever(bool new_value)

Copy link
Author

Choose a reason for hiding this comment

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

which the name doesn't really imply?

It doesn't? I'm failing to see how it would be interpreted otherwise…

There's no reason for code to call SetDisableAvailability(False), as that is the default. Since there's no reason for it to be called like that, I made the function take no arguments, so that it can be called only one way. This may be different than other bool setters, but I feel it's better prevent it from being mis-called than make it match other setters.

Choose a reason for hiding this comment

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

Set means "give some value to this feature" - true or false to "Disabling Ability". If all this API does is "DisableAvailability" then it should be called DisableAvailability(). The "Set" is just confusing and adds nothing.

If there are never any situations where you Disable Availability, then you want to enable it again on the same object, then just having a DisableAvailability API is fine. If you would ever want to turn it back on, then you will need a Set API with a bool input.

Copy link
Author

Choose a reason for hiding this comment

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

Fair, I've added a parameter.

addr_t addr = LLDB_INVALID_ADDRESS;
if (flags.Test(eTypeInstanceIsPointer)) {
// Objects are pointers.
addr = object.GetValueAsUnsigned(LLDB_INVALID_ADDRESS);

Choose a reason for hiding this comment

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

This should use GetValueAsAddress.

Copy link
Author

Choose a reason for hiding this comment

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

GetValueAsAddress is on SBValue, I don't see an equivalent on ValueObject.

Choose a reason for hiding this comment

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

I'm surprised we don't have a little wrapper for this in ValueObject, but you can do what GetValueAsAddress does, and call FixDataAddress of the addr you get back.

@jasonmolenda will know for sure whether that's the right thing to do.

Choose a reason for hiding this comment

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

Yeah if it's guaranteed to be an address it's safe to run it through Process::FixAnyAddress() (the method to use when you don't know if it's an address of code or data). With MTE managed heap, if the value might get put into a jitted expression (so used in code running in the inferior, with MTE) you need to retain the MTE nibble in the top byte. It's an edge case but something to mention, @felipepiovezan dealt with a tricky case where we needed to maintain metadata bits through to the point where we looked it up/use it, just recently, for something that ended up in a jitted expression.

Copy link
Author

Choose a reason for hiding this comment

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

@jasonmolenda thanks, this address is data, and will be used in a jitted expression.

Copy link
Author

Choose a reason for hiding this comment

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

I added a call to FixDataAddress.

@kastiglione
Copy link
Author

@swift-ci test

Copy link

@adrian-prantl adrian-prantl left a comment

Choose a reason for hiding this comment

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

LGTM with outstanding feedback addressed.

@kastiglione
Copy link
Author

@swift-ci test

@kastiglione
Copy link
Author

@jimingham I've replied to your feedback

@kastiglione
Copy link
Author

@swift-ci test

Copy link

@jimingham jimingham left a comment

Choose a reason for hiding this comment

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

There was one grammatical error in a comment (can the -> can get the).

Other than that, LGTM.

@kastiglione
Copy link
Author

@swift-ci test

Comment on lines +1320 to +1322
if (!m_options.GetUseContextFreeSwiftPrintObject())
if (!m_swift_ast_ctx.GetImplicitImports(
m_sc, process_sp, additional_imports, implicit_import_error)) {
Copy link
Author

Choose a reason for hiding this comment

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

@adrian-prantl I've introduced this change to the PR. This prevents module loading when the expression is running the new po.

Choose a reason for hiding this comment

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

This LGTM, assuming m_options.GetUseContextFreeSwiftPrintObject() is only true in a po!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants