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

transpile: Rewrite fn compare_src_locs implementation to have a total order #1128

Merged
merged 2 commits into from
Oct 24, 2024

Conversation

Kriskras99
Copy link
Contributor

@Kriskras99 Kriskras99 commented Sep 18, 2024

This implementation is simplified compared to the previous one. It is also almost twice as slow in the exhaustive test (15 vs 25 seconds).
However, in real sort usage the impact should be significantly less.

@kkysen kkysen self-requested a review September 18, 2024 22:20
@kkysen kkysen changed the title Rewrite compare_src_locs implementation to have a total order transpile: Rewrite fn compare_src_locs implementation to have a total order Sep 18, 2024
Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks a lot better, and the better comments help a lot as well.

Is there a test we can add to make sure this is working as intended (correctly sorted, no panics)?

c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
@Kriskras99
Copy link
Contributor Author

I've added a test for the total order, based on a reduced test case from the issue (apparently creduce can reduce any text file).
I've also included your code suggestions.

c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
c2rust-transpile/src/c_ast/mod.rs Outdated Show resolved Hide resolved
@Kriskras99
Copy link
Contributor Author

Not sure what my IDE did to the imports but I'll fix it, I'll also fix your other suggestions. But it will have to wait until after the weekend

@kkysen
Copy link
Contributor

kkysen commented Sep 20, 2024

Not sure what my IDE did to the imports but I'll fix it, I'll also fix your other suggestions. But it will have to wait until after the weekend

No problem, no rush.

@Kriskras99 Kriskras99 force-pushed the sorting_fix branch 2 times, most recently from b562165 to 12f7571 Compare September 23, 2024 15:12
@Kriskras99
Copy link
Contributor Author

@kkysen I've included your suggestions. Is there anything else that needs to change?

@kkysen kkysen self-requested a review September 30, 2024 19:45
@Kriskras99
Copy link
Contributor Author

Added your last suggestion

@kkysen
Copy link
Contributor

kkysen commented Oct 3, 2024

CI seems to be failing, only on Darwin, for some reason:

CI

/Users/runner/work/1/s/tests/statics/src/thread_locals.c:4:17: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
static __thread gsti = 37;
~~~~~~~~~~~~~~~ ^
int
/Users/runner/work/1/s/tests/statics/src/thread_locals.c:5:17: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
extern __thread geti;
~~~~~~~~~~~~~~~ ^
int
/Users/runner/work/1/s/tests/statics/src/thread_locals.c:12:21: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
    static __thread fsti = 59;
    ~~~~~~~~~~~~~~~ ^
    int
/Users/runner/work/1/s/tests/statics/src/thread_locals.c:13:21: error: type specifier missing, defaults to 'int'; ISO C99 and later do not support implicit int [-Wimplicit-int]
    extern __thread feti;
    ~~~~~~~~~~~~~~~ ^
    int

This doesn't seem to be related to the changes, here, though, so I'm not sure what's happening.

@Kriskras99
Copy link
Contributor Author

Maybe the darwin image got updated or something? I really don't see how this change can cause this

@Kriskras99
Copy link
Contributor Author

Could you try triggering the CI on the last commit in master? See if it has the same failure

Kriskras99 and others added 2 commits October 23, 2024 23:28
This implementation is simplified compared to the previous one.
It is also almost twice as slow in the exhaustive test (15 vs 25 seconds) in
immunant#1126 (comment)
However, in real sort usage the impact should be significantly less.

Fixes immunant#1126
Also included some code suggestions by Khyber.

Co-authored-by: Khyber Sen <[email protected]>
@Kriskras99
Copy link
Contributor Author

@kkysen with your CI fix from yesterday, this PR is now green again.

@kkysen
Copy link
Contributor

kkysen commented Oct 24, 2024

@kkysen with your CI fix from yesterday, this PR is now green again.

Awesome! I'll merge this now. Sorry for the delay with the CI, and thanks for fixing this!

@kkysen kkysen merged commit 0d2c6c2 into immunant:master Oct 24, 2024
8 checks passed
@kkysen
Copy link
Contributor

kkysen commented Oct 24, 2024

Oops, I'm now getting a different CI error in the CI workflow that won't run on outside contributors' PRs correctly: https://github.com/immunant/c2rust/actions/runs/11493361902/job/31988919249. Do you have an idea of what this might be? Sorry this CI workflow still isn't set up to correctly run on external PRs.

@Kriskras99
Copy link
Contributor Author

Maybe a path confusion? If you look at the first error:

error[E0422]: cannot find struct, variant or union type `C2RustUnnamed_4` in this scope
    --> src/lib/conncache.rs:3264:38
     |
3264 |                 __sigaction_handler: C2RustUnnamed_4 {
     |                                      ^^^^^^^^^^^^^^^ not found in this scope
     |
help: consider importing one of these items
     |
3293 |     use crate::src::lib::conncache::C2RustUnnamed_4;
     |
       and 28 other candidates

It seems that C2RustUnnamed_4 is put in src/src/lib/conncache.rs instead of src/lib/conncache.rs and therefore requiring an import. But I haven't looked into c2rust-testsuite enough to understand what it's doing.

kkysen added a commit that referenced this pull request Nov 18, 2024
…rcLoc` order

#1128 fixed the non-total (non-transitive) `SrcLoc` ordering,
but accidentally broke the `curl` transpilation test in `c2rust-testsuites`,
which is tested in CI, but was broken for external contributors PRs (fixed now in #1159).

In the `curl` transpilation, a couple of `use` imports (`__sigset_t` and `C2RustUnnamed_4`)
were missing, cause the build to fail afterward.

I'm still not exactly sure why this fixes the issue while maintaining a transitive, total order,
but it passes the total order test and transpiles `curl` correctly now.
Hopefully this is a complete fix, and I didn't just fix a one-off error in `curl`.
kkysen added a commit that referenced this pull request Nov 18, 2024
…rcLoc` order (#1163)

#1128 fixed the non-total (non-transitive) `SrcLoc` ordering, but
accidentally broke the `curl` transpilation test in `c2rust-testsuites`,
which is tested in CI, but was broken for external contributors PRs
(fixed now in #1159).

In the `curl` transpilation, a couple of `use` imports (`__sigset_t` and
`C2RustUnnamed_4`) were missing, cause the build to fail afterward.

I'm still not exactly sure why this fixes the issue while maintaining a
transitive, total order, but it passes the total order test and
transpiles `curl` correctly now. Hopefully this is a complete fix, and I
didn't just fix a one-off error in `curl`.

* 9679a3b is the real fix.
* a4a052b are generally useful for testing and debugging.
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.

SrcLoc sorting is non-transitive and a false total order and equality (panics in 1.81)
2 participants