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

Fixes #982 #983

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Fixes #982 #983

merged 1 commit into from
Sep 23, 2024

Conversation

d2weber
Copy link
Contributor

@d2weber d2weber commented Sep 10, 2024

No description provided.

@d2weber d2weber force-pushed the pr branch 2 times, most recently from 9a64ca2 to f4498f5 Compare September 10, 2024 16:16
src/socket/tcp.rs Outdated Show resolved Hide resolved
@Dirbaio
Copy link
Member

Dirbaio commented Sep 10, 2024

Fix looks good!

Could you add to CI a 16bit target that would've triggered the errors, so that CI catches similar busg in the future?

@d2weber
Copy link
Contributor Author

d2weber commented Sep 10, 2024

I could try to reproduce with the msp430-none-elf target (so we wouldn't need a separate target.json). But there is no std. Currently in ci.sh test all feature combinations are tested. One option would be maintaining a separate set of "no-std" feature combinations. Or I check if it'd be enough to run clippy with the msp430 target.

@d2weber
Copy link
Contributor Author

d2weber commented Sep 10, 2024

Just checked, it's reproducible with the msp430-none-elf target. But the standard library cannot be compiled with 16 bit pointer-width, so only no-std feature sets can be tested. I'd need help figuring out, which feature sets make sense here.

Then I could add something like this in ci.sh

clippy() {
    rustup +nightly component add rust-src
    cargo +nightly clippy -Z build-std=core,alloc --target msp430-none-elf --no-default-features --features="socket-tcp proto-ipv4 medium-ethernet"
}

By the way, I'm getting lots of warnings with cargo +beta clippy.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 10, 2024

the errors are caught by rustc, not clippy, right? If so, just doing a cargo build for msp430 should be enough?

For the features i'd just do one build, enabling all features that do work on nostd and ignoring the others.

@d2weber
Copy link
Contributor Author

d2weber commented Sep 17, 2024

Yes you are right, its build.

@d2weber
Copy link
Contributor Author

d2weber commented Sep 17, 2024

I can't find the location where this error originates. Line numbers seem to be off. Maybe there is some macro magic going on? I also can't reproduce this error locally, no idea what's going on:

error: this arithmetic operation will overflow
   --> src/socket/tcp.rs:699:67
    |
699 |         Some(cmp::min(last_win_adjusted >> self.remote_win_shift, (1 << 16) - 1) as u16)
    |                                                                   ^^^^^^^^^ attempt to shift left by `16_i32`, which would overflow
    |
    = note: `#[deny(arithmetic_overflow)]` on by default
    ```

@thvdveld
Copy link
Contributor

@d2weber I think actions/checkout was drunk and used an old commit. I opened a PR to update to the latest version of actions/checkout. You might want to rebase on main once that PR is merged.

.github/workflows/test.yml Outdated Show resolved Hide resolved
ci.sh Outdated Show resolved Hide resolved
@d2weber d2weber force-pushed the pr branch 2 times, most recently from 47958b3 to 16345da Compare September 18, 2024 16:54
@thvdveld
Copy link
Contributor

thvdveld commented Sep 18, 2024

I don't understand why CI is using the wrong source code. All jobs are checking out 1946465, which looks fine, but then when the tests are running, the source doesn't look like what's in 1946465.

Never mind, you actually have (1 << 16) - 1 on line 699 in src/socket/tcp.rs.

@d2weber
Copy link
Contributor Author

d2weber commented Sep 23, 2024

I don't understand why CI is using the wrong source code. All jobs are checking out 1946465, which looks fine, but then when the tests are running, the source doesn't look like what's in 1946465.

Never mind, you actually have (1 << 16) - 1 on line 699 in src/socket/tcp.rs.

No idea how I missed that. I have to check later, what went wrong, maybe my editor didn't reload the file after the rebase...

@thvdveld
Copy link
Contributor

I didn't catch it at first sight either! Thanks for fixing it.

@thvdveld thvdveld added this pull request to the merge queue Sep 23, 2024
Merged via the queue into smoltcp-rs:main with commit a2b92ed Sep 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants