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

Interface nil check #1138

Merged
merged 2 commits into from
May 28, 2020
Merged

Interface nil check #1138

merged 2 commits into from
May 28, 2020

Conversation

aykevl
Copy link
Member

@aykevl aykevl commented May 27, 2020

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 is finally allowed on function
parameters, but it'll probably take a while before that is implemented.


The first commit refactors some code that makes the second commit easier to implement. It also has a (very small) code size impact.

…d call

This is a common case, but it also complicates the code. Removing this
special case does have a negative effect on code size in rare cases, but
I don't think it's worth keeping around (and possibly causing bugs) for
such uncommon cases.

This should not result in functional changes, although the output (as
stated above) sometimes changes a little bit.
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
@deadprogram
Copy link
Member

Very good check to have, certainly worth the very few bytes it adds. Now merging, thanks @aykevl

@deadprogram deadprogram merged commit 734613c into dev May 28, 2020
@deadprogram deadprogram deleted the interface-nil-check branch May 28, 2020 11:42
@niaow niaow added this to the v0.14 milestone Jun 27, 2020
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.

3 participants