Skip to content

Commit

Permalink
Fix debug location parsing (tensorflow#271)
Browse files Browse the repository at this point in the history
* Fix debug location parsing

The old code would incorrectly consume commas that might belong
to other fields and it also expected the format presented in the SIL
documentation, which doesn't match what the compiler outputs today.

* Review comments
  • Loading branch information
Adam Paszke authored and Eugene Burmako committed Sep 10, 2019
1 parent e26b2c2 commit ce02570
Show file tree
Hide file tree
Showing 5 changed files with 69 additions and 35 deletions.
19 changes: 19 additions & 0 deletions Sources/SIL/Parser.swift
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,17 @@ class Parser {

// MARK: Tree-level APIs

// Applies the function, but restores the cursor from before the call if it returns nil.
func maybeParse<T>(_ f: () throws -> T?) rethrows -> T? {
let savedCursor = cursor
if let result = try f() {
return result
} else {
cursor = savedCursor
return nil
}
}

/// Same as `parseMany` but returning `nil` if the cursor isn't pointing at `pre`.
/// This is necessary to e.g. accommodate a situation when not having a parameter list is
/// as valid as having an empty parameter list.
Expand Down Expand Up @@ -141,6 +152,14 @@ class Parser {
return try parseMany(pre, parseOne)
}

func parseUntilNil<T>(_ parseOne: () throws -> T?) rethrows -> [T] {
var result = [T]()
while let element = try parseOne() {
result.append(element)
}
return result
}

/// Run a given parser multiple times as follows:
/// 1) Check that cursor if pointing at `pre` without consuming `pre`.
/// 2) Run parser, repeat.
Expand Down
4 changes: 2 additions & 2 deletions Sources/SIL/SIL.swift
Original file line number Diff line number Diff line change
Expand Up @@ -410,10 +410,10 @@ public class Result {

// https://github.com/apple/swift/blob/master/docs/SIL.rst#basic-blocks
public class SourceInfo {
public let scopeRef: String?
public let scopeRef: Int?
public let loc: Loc?

public init(_ scopeRef: String?, _ loc: Loc?) {
public init(_ scopeRef: Int?, _ loc: Loc?) {
self.scopeRef = scopeRef
self.loc = loc
}
Expand Down
69 changes: 39 additions & 30 deletions Sources/SIL/SILParser.swift
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class SILParser: Parser {
switch instructionName {
case "alloc_stack":
let type = try parseType()
let attributes = try parseNilOrMany(", ") { try parseDebugAttribute() } ?? []
let attributes = try parseUntilNil { try parseDebugAttribute() }
return .allocStack(type, attributes)
case "apply":
let nothrow = skip("[nothrow]")
Expand Down Expand Up @@ -148,11 +148,11 @@ class SILParser: Parser {
return .deallocStack(operand)
case "debug_value":
let operand = try parseOperand()
let attributes = try parseNilOrMany(", ") { try parseDebugAttribute() } ?? []
let attributes = try parseUntilNil { try parseDebugAttribute() }
return .debugValue(operand, attributes)
case "debug_value_addr":
let operand = try parseOperand()
let attributes = try parseNilOrMany(", ") { try parseDebugAttribute() } ?? []
let attributes = try parseUntilNil { try parseDebugAttribute() }
return .debugValueAddr(operand, attributes)
case "destroy_value":
let operand = try parseOperand()
Expand Down Expand Up @@ -260,7 +260,7 @@ class SILParser: Parser {
return .structExtract(operand, declRef)
case "switch_enum":
let operand = try parseOperand()
let cases = try parseNilOrMany(", ") { try parseCase() } ?? []
let cases = try parseUntilNil { try parseCase() }
return .switchEnum(operand, cases)
case "tuple":
let elements = try parseTupleElements()
Expand Down Expand Up @@ -308,18 +308,20 @@ class SILParser: Parser {
}

// https://github.com/apple/swift/blob/master/docs/SIL.rst#switch-enum
func parseCase() throws -> Case {
try take(",")
if skip("case") {
let declRef = try parseDeclRef()
try take(":")
let identifier = try parseIdentifier()
return .case(declRef, identifier)
} else if skip("default") {
let identifier = try parseIdentifier()
return .default(identifier)
} else {
throw parseError("unknown case")
func parseCase() throws -> Case? {
return try maybeParse {
guard skip(",") else { return nil }
if skip("case") {
let declRef = try parseDeclRef()
try take(":")
let identifier = try parseIdentifier()
return .case(declRef, identifier)
} else if skip("default") {
let identifier = try parseIdentifier()
return .default(identifier)
} else {
return nil
}
}
}

Expand All @@ -344,13 +346,15 @@ class SILParser: Parser {
}

// https://github.com/apple/swift/blob/master/docs/SIL.rst#debug-value
func parseDebugAttribute() throws -> DebugAttribute {
try take(",")
guard !skip("argno") else { return .argno(try parseInt()) }
guard !skip("name") else { return .name(try parseString()) }
guard !skip("let") else { return .let }
guard !skip("var") else { return .var }
throw parseError("unknown debug attribute")
func parseDebugAttribute() throws -> DebugAttribute? {
return try maybeParse {
guard skip(",") else { return nil }
guard !skip("argno") else { return .argno(try parseInt()) }
guard !skip("name") else { return .name(try parseString()) }
guard !skip("let") else { return .let }
guard !skip("var") else { return .var }
return nil
}
}

func parseDeclKind() throws -> DeclKind? {
Expand Down Expand Up @@ -483,7 +487,6 @@ class SILParser: Parser {

// https://github.com/apple/swift/blob/master/docs/SIL.rst#debug-information
func parseLoc() throws -> Loc? {
guard skip(",") else { return nil }
guard skip("loc") else { return nil }
let path = try parseString()
try take(":")
Expand Down Expand Up @@ -580,18 +583,24 @@ class SILParser: Parser {
}

// https://github.com/apple/swift/blob/master/docs/SIL.rst#debug-information
func parseScopeRef() throws -> String? {
guard skip(",") else { return nil }
func parseScopeRef() throws -> Int? {
guard skip("scope") else { return nil }
let ref = try parseInt()
return "scope " + String(ref)
return try parseInt()
}

// https://github.com/apple/swift/blob/master/docs/SIL.rst#basic-blocks
func parseSourceInfo() throws -> SourceInfo? {
let scopeRef = try parseScopeRef()
// NB: The SIL docs say that scope refs precede locations, but this is
// not true once you look at the compiler outputs or its source code.
guard skip(",") else { return nil }
let loc = try parseLoc()
guard scopeRef != nil || loc != nil else { return nil }
// NB: No skipping if we failed to parse the location.
let scopeRef = loc == nil || skip(",") ? try parseScopeRef() : nil
// We've skipped the comma, so failing to parse any of those two
// components is an error.
guard scopeRef != nil || loc != nil else {
throw parseError("Failed to parse source info")
}
return SourceInfo(scopeRef, loc)
}

Expand Down
8 changes: 5 additions & 3 deletions Sources/SIL/SILPrinter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -432,9 +432,9 @@ class SILPrinter: Printer {
func print(_ loc: Loc) {
print("loc ")
literal(loc.path)
print(" : ")
print(":")
literal(loc.line)
print(" : ")
print(":")
literal(loc.column)
}

Expand All @@ -453,8 +453,10 @@ class SILPrinter: Printer {
}

func print(_ sourceInfo: SourceInfo) {
print(", ", sourceInfo.scopeRef) { print($0) }
// NB: The SIL docs say that scope refs precede locations, but this is
// not true once you look at the compiler outputs or its source code.
print(", ", sourceInfo.loc) { print($0) }
print(", scope ", sourceInfo.scopeRef) { print($0) }
}

func print(_ elements: TupleElements) {
Expand Down
4 changes: 4 additions & 0 deletions Tests/SILTests/InstructionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ let instructionDefs = [
"%64 = tuple_extract %63 : $(Builtin.Int64, Builtin.Int1), 0",
"alloc_stack $Float",
"alloc_stack $IndexingIterator<Range<Int>>, var, name \"$inputIndex$generator\"",
"%79 = alloc_stack $Optional<(Int, Int)>, loc \"Examples/cnn.swift\":16:3, scope 6",
"apply %10(%1) : $@convention(method) (@guaranteed Array<Float>) -> Int",
"apply %17<Self>(%1, %2, %16) : $@convention(witness_method: Comparable) <τ_0_0 where τ_0_0 : Comparable> (@in_guaranteed τ_0_0, @in_guaranteed τ_0_0, @thick τ_0_0.Type) -> Bool",
"apply %8<Int, Int>(%2, %6) : $@convention(thin) <τ_0_0, τ_0_1 where τ_0_0 : Strideable, τ_0_1 : Strideable> (@in_guaranteed τ_0_0, @in_guaranteed τ_0_1) -> ()",
"%14 = begin_borrow %10 : $Tensor<Float>",
"%6 = copy_value %0 : $TensorShape, loc \"Examples/cnn.swift\":10:11, scope 3",
"%54 = copy_value %53 : $Tensor<Float>",
"destroy_value %10 : $Tensor<Float>",
"end_borrow %27 : $Tensor<Float>",
Expand All @@ -33,6 +35,7 @@ let instructionDefs = [
"debug_value %1 : $Array<Float>, let, name \"input\", argno 2",
"debug_value %11 : $Int, let, name \"n\"",
"debug_value_addr %0 : $*Array<Float>, var, name \"out\", argno 1",
"debug_value %0 : $TensorShape, let, name \"ar\", argno 1, loc \"Examples/cnn.swift\":9:18, scope 2",
"end_access %265 : $*Array<Float>",
"end_access [abort] %42 : $T",
"end_apply %268",
Expand Down Expand Up @@ -63,6 +66,7 @@ let instructionDefs = [
// "string_literal utf8 \"\\n\"",
"struct_element_addr %235 : $*Float, #Float._value",
"switch_enum %122 : $Optional<Int>, case #Optional.some!enumelt.1: bb11, case #Optional.none!enumelt: bb18",
"switch_enum %84 : $Optional<(Int, Int)>, case #Optional.some!enumelt.1: bb5, case #Optional.none!enumelt: bb6, loc \"Examples/cnn.swift\":16:3, scope 6",
"tuple ()",
"tuple (%a : $A, %b : $B)",
// TODO(#23): Parse tuple types with argument labels
Expand Down

0 comments on commit ce02570

Please sign in to comment.