Skip to content
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

Respect "getBypassResilience" on "isResilient" #78537

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

augusto2112
Copy link
Contributor

LLDB sets BypassResilience to have direct access to private properties of modules built with resilience. This setting is ignored in many paths that check resiliency though, which causes LLDB to fail to evaluate certain expressions.

This commit simply makes sure that isResilient checks whether resiliency should be bypassed or not.

rdar://137876089

LLDB sets BypassResilience to have direct access to private properties
of modules built with resilience. This setting is ignored in many paths
that check resiliency though, which causes LLDB to fail to evaluate
certain expressions.

This commit simply makes sure that `isResilient` checks whether
resiliency should be bypassed or not.

rdar://137876089
@augusto2112
Copy link
Contributor Author

@swift-ci test

1 similar comment
@augusto2112
Copy link
Contributor Author

@swift-ci test

@adrian-prantl
Copy link
Contributor

Is this tested in LLDB?

@adrian-prantl
Copy link
Contributor

The change seems very broad and makes me wonder how this setting was previously applied and if this could have any unintended sideeffects. How/where was the setting used so far?

@augusto2112
Copy link
Contributor Author

The change seems very broad and makes me wonder how this setting was previously applied and if this could have any unintended sideeffects. How/where was the setting used so far?

This is a swift-frontend flag:

-bypass-resilience-checks
                          Ignore all checks for module resilience.

Which is turned on by LLDB, and seems to be introduced for that purpose:

  /// Return whether the module was imported with resilience disabled. The
  /// debugger does this to access private fields.
  bool getBypassResilience() const { return BypassResilience; }

It's only checked in one place today:

static bool isDirectToStorageAccess(const DeclContext *UseDC,
                                    const VarDecl *var, bool isAccessOnSelf) {
  // Some code here

  // If the storage is resilient, we cannot access it directly at all.
  if (var->isResilient(UseDC->getParentModule(),
                       UseDC->getResilienceExpansion()))
    return var->getModuleContext()->getBypassResilience();

I understand that this change is pretty broad, but I've seen multiple bug reports which are caused by the compiler assuming that certain functions exist because resiliency is enabled in different code paths (one of them fixed in #78486, which would not be needed anymore if this patch is approved). Instead of local fixes for each of these instances I thought a more broad fix made sense.

Also, the flag is supposed to "Ignore all checks for module resilience", and it does not work as described today.

@augusto2112
Copy link
Contributor Author

augusto2112 commented Jan 10, 2025

Is this tested in LLDB?

This is tested in swiftlang/llvm-project#9809, I will add another test to that PR later if we go forward with this solution.

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.

2 participants