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.

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

Jump to bottom

[🍒][6.1] Pass correct SubExpr as source location when emitting Load of LValue #79584

Open
wants to merge 2 commits into
base: release/6.1
Choose a base branch
from
Open

[🍒][6.1] Pass correct SubExpr as source location when emitting Load of LValue #79584

wants to merge 2 commits into from

Conversation

rastogishubham
Copy link
Contributor

@rastogishubham rastogishubham commented Feb 24, 2025 •
edited
Loading

  • Explanation:

This PR cherry-picks from #79524 to the 6.1 release branch. It fixes an issue where a LoadExpr, which is implicit in SIL, is passed in as the SILLocation when emitting the LValue of the LoadExpr, as opposed to the SubExpr contained within the LoadExpr.

This has the effect of making all SIL instructions having implicit SILLocations which are generated when emitting the LValue of the LoadExpr, even if the SubExprs contained within the LoadExpr are not marked as implicit in the AST dump.

To make this possible, we also added a new flag, -sil-print-debuginfo-verbose, which prints the Implicit, and autoGenerated fields for a SILLocation, and prints what the result of calling SILLocation::isHiddenFromDebugInfo() would be. For testing and diagnostic purposes.

For example, if we take the ast-dump below, we can see that the load_expr is marked as implicit, but AST nodes it contains are not, however, if we look at the SILGen output of these AST nodes, they are marked as Implicit

(load_expr implicit type="UInt8" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:35 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:15 - line:14:35]
                      (member_ref_expr type="@lvalue UInt8" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:35 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:15 - line:14:35] decl="Swift.(file).UnsafeMutablePointer extension.pointee [with (substitution_map generic_signature=<Pointee where Pointee : Escapable> Pointee -> UInt8)]"
                        (call_expr type="UnsafeMutablePointer<UInt8>" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:17 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:15 - line:14:33] nothrow isolation_crossing="none"
                          (dot_syntax_call_expr type="(Int) -> UnsafeMutablePointer<UInt8>" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:17 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:15 - line:14:17] nothrow isolation_crossing="none"
                            (declref_expr type="(UnsafeMutablePointer<UInt8>) -> (Int) -> UnsafeMutablePointer<UInt8>" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:17 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:17 - line:14:17] decl="Swift.(file)._Pointer extension.advanced(by:) [with (substitution_map generic_signature=<Self where Self : _Pointer> Self -> UnsafeMutablePointer<UInt8>)]" function_ref=single)
                            (argument_list implicit
                              (argument
                                (declref_expr type="UnsafeMutablePointer<UInt8>" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:15 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:15 - line:14:15] decl="OSX_Swift.(file).ContentView.asan_bug().x@/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:12:13" function_ref=unapplied))))
                          (argument_list labels="by:"
                            (argument label="by"
                              (integer_literal_expr type="Int" location=/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:30 range=[/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift:14:30 - line:14:30] value="128" builtin_initializer="Swift.(file).Int.init(_builtinIntegerLiteral:)" initializer="**NULL**"))))))

%31 = mark_dependence [nonescaping] %28 on %30, loc "/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift":14:35 isImplicit: true, isAutoGenerated: false, isHiddenFromDebugInfo: true, scope 3 // user: %32
  %32 = begin_access [read] [unsafe] %31, loc "/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift":14:35 isImplicit: true, isAutoGenerated: false, isHiddenFromDebugInfo: true, scope 3 // users: %33, %34
  %33 = load %32, loc "/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift":14:35 isImplicit: true, isAutoGenerated: false, isHiddenFromDebugInfo: true, scope 3 // user: %36
  end_access %32, loc * "/Users/shubhamrastogi/Development/test143924559/sanitizer-qa/OSX_Swift/ContentView.swift":14:35 isImplicit: true, isAutoGenerated: true, isHiddenFromDebugInfo: true, scope 3 // id: %34

  • Scope:

    This changes the SILLocation passed to any LoadExpr LValue emission to be the SubExpr instead of the LoadExpr itself, should be a fairly minimal impact.

  • Issues:

    None

  • Original PRs:

#79524 and #79519

  • Risk:

    The risk should be very low, the change will only affect the SILLocation that is passed and should not cause any major issues.

  • Testing:

New test cases, and passing test suite.

@rastogishubham rastogishubham requested a review from a team as a code owner February 24, 2025 20:12
@rastogishubham rastogishubham changed the title Fix asan bug6.1 Pass correct SubExpr as source location when emitting Load of LValue Feb 24, 2025
@rastogishubham rastogishubham changed the title Pass correct SubExpr as source location when emitting Load of LValue [🍒][6.1] Pass correct SubExpr as source location when emitting Load of LValue Feb 24, 2025
@rastogishubham
Copy link
Contributor Author

@swift-ci please test

adrian-prantl
adrian-prantl approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@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.

I support taking this change.

@adrian-prantl
Copy link
Contributor

Also tracked as rdar://143924559.

@rastogishubham
Add option -sil-print-debuginfo-verbose
502ec8a 
The option -sil-print-debuginfo-verbose will print the values of the
fields, implicit, and autoGenerated for a SILLocation, as well as
whether the SILLocation is considered hidden from debug information
by calling SILLocation::isHiddenFromDebugInfo()

(cherry picked from commit 1567386)
@rastogishubham
Pass correct SubExpr as source location when emitting Load of LValue.
e225a80 
When emitting an LValue of a LoadExpr during SILGen, the Expression
passed as a SILLocation for the LValue was the parent LoadExpr instead
of the SubExpr that contains the LValue for the load. This leads to
the SILLocation to be marked as implicit, because a LoadExpr is always
an implicit expression.

An implicit SILLocation is not emitted when doing IRGen as it is
considered to be hidden from debug info and thus we lose the debug
location for the LValue of a LoadExpr.

This patch fixes this issue.

(cherry picked from commit 52a5ed8)
@rastogishubham
Copy link
Contributor Author

@swift-ci please test

@rastogishubham
Copy link
Contributor Author

@swift-ci please test macOS

1 similar comment
@rastogishubham
Copy link
Contributor Author

@swift-ci please test macOS

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

@adrian-prantl adrian-prantl adrian-prantl approved these changes

@DougGregor DougGregor Awaiting requested review from DougGregor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rastogishubham @adrian-prantl