Skip to content

Commit

Permalink
transform: introduce check for method calls on nil interfaces
Browse files Browse the repository at this point in the history
I ran into an issue where I did a method call on a nil interface and it
resulted in a HardFault. Luckily I quickly realized what was going on so
I could fix it, but I think undefined behavior is definitely the wrong
behavior in this case. This commit therefore changes such calls to cause
a nil panic instead of introducing undefined behavior.

This does have a code size impact. It's relatively minor, much lower
than I expected. When comparing the before and after of the drivers
smoke tests (probably the most representative sample available), I found
that most did not change at all and those that did change, normally not
more than 100 bytes (16 or 32 byte changes are typical).

Right now the pattern is the following:

    switch typecode {
    case 1:
        call method 1
    case 2:
        call method 2
    default:
        nil panic
    }

I also tried the following (in the hope that it would be easier to
optimize), but it didn't really result in a code size reduction:

    switch typecode {
    case 1:
        call method 1
    case 2:
        call method 2
    case 0:
        nil panic
    default:
        unreachable
    }

Some code got smaller, while other code (the majority) got bigger. Maybe
this can be improved once range[1] is finally allowed[2] on function
parameters, but it'll probably take a while before that is implemented.

[1]: https://llvm.org/docs/LangRef.html#range-metadata
[2]: rust-lang/rust#50156
  • Loading branch information
aykevl committed May 27, 2020
1 parent e223a9b commit d2325cc
Show file tree
Hide file tree
Showing 3 changed files with 15 additions and 2 deletions.
13 changes: 11 additions & 2 deletions transform/interface-lowering.go
Original file line number Diff line number Diff line change
Expand Up @@ -616,10 +616,19 @@ func (p *lowerInterfacesPass) createInterfaceMethodFunc(itf *interfaceInfo, sign
// Create entry block.
entry := p.ctx.AddBasicBlock(fn, "entry")

// Create default block and make it unreachable (which it is, because all
// possible types are checked).
// Create default block and call runtime.nilPanic.
// The only other possible value remaining is nil for nil interfaces. We
// could panic with a different message here such as "nil interface" but
// that would increase code size and "nil panic" is close enough. Most
// importantly, it avoids undefined behavior when accidentally calling a
// method on a nil interface.
defaultBlock := p.ctx.AddBasicBlock(fn, "default")
p.builder.SetInsertPointAtEnd(defaultBlock)
nilPanic := p.mod.NamedFunction("runtime.nilPanic")
p.builder.CreateCall(nilPanic, []llvm.Value{
llvm.Undef(llvm.PointerType(p.ctx.Int8Type(), 0)),
llvm.Undef(llvm.PointerType(p.ctx.Int8Type(), 0)),
}, "")
p.builder.CreateUnreachable()

// Create type switch in entry block.
Expand Down
1 change: 1 addition & 0 deletions transform/testdata/interface.ll
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ declare void @runtime.printuint8(i8)
declare void @runtime.printint32(i32)
declare void @runtime.printptr(i32)
declare void @runtime.printnl()
declare void @runtime.nilPanic(i8*, i8*)

define void @printInterfaces() {
call void @printInterface(i32 ptrtoint (%runtime.typeInInterface* @"typeInInterface:reflect/types.type:basic:int" to i32), i8* inttoptr (i32 5 to i8*))
Expand Down
3 changes: 3 additions & 0 deletions transform/testdata/interface.out.ll
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ declare void @runtime.printptr(i32)

declare void @runtime.printnl()

declare void @runtime.nilPanic(i8*, i8*)

define void @printInterfaces() {
call void @printInterface(i32 4, i8* inttoptr (i32 5 to i8*))
call void @printInterface(i32 16, i8* inttoptr (i8 120 to i8*))
Expand Down Expand Up @@ -83,6 +85,7 @@ entry:
]

default: ; preds = %entry
call void @runtime.nilPanic(i8* undef, i8* undef)
unreachable

"reflect/types.type:named:Number": ; preds = %entry
Expand Down

0 comments on commit d2325cc

Please sign in to comment.