Skip to content

Conversation

Rua
Copy link
Contributor

@Rua Rua commented Oct 11, 2025

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.

I think the CI failings are because you didn't update the os- and arch-specific snapshots that aren't run locally.

)]
#[no_mangle]
pub static mut TRUE: core::ffi::c_int = 1 as core::ffi::c_int;
pub static mut TRUE: ::core::ffi::c_int = 1 as ::core::ffi::c_int;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ahomescu, is #1399 able to handle stuff like this and still shorten the paths with imports? Also, is it or another refactor able to remove the :: where not needed? Would that be difficult at all?

The change here is correct and handles some edge cases, but it is longer, so it'd be nice to be able to refactor out the unnecessary ones afterward.

@Rua Rua force-pushed the abs-crate branch 2 times, most recently from 71c6b98 to 8d87265 Compare October 11, 2025 20:30
@Rua
Copy link
Contributor Author

Rua commented Oct 11, 2025

I think the CI failings are because you didn't update the os- and arch-specific snapshots that aren't run locally.

The three snapshot CI runs are failing because the leading double colon :: is expected by the snapshot (because I added it) but, for some reason, not actually generated by the transpiler. And this has me rather puzzled at the moment. For the Clang 18 test, snapshots__transpile-x86_64@vm_x86.c.snap gives this diff:

    6     6 │     unused_assignments,
    7     7 │     unused_mut
    8     8 │ )]
    9     9 │ #![feature(asm)]
   10       │-use ::core::arch::asm;
         10 │+use core::arch::asm;
   11    11 │ #[derive(Copy, Clone)]
   12    12 │ #[repr(C)]
   13    13 │ pub struct vm_t {
   14    14 │     pub programStack: ::core::ffi::c_int,

I can find only one place in the whole codebase that adds an import with the string "asm":

item_store.add_use(vec!["core".into(), "arch".into()], "asm");

All items added to the item store are eventually fed through PathedMultiImports::into_items, where I have added lines to make absolute paths:

if leaves.len() == 1 {
path.push(leaves.pop().unwrap());
let path = mk().abs_path(path);
attrs.use_simple_item(path, None as Option<Ident>)
} else {
let path = mk().abs_path(path);
attrs.use_multiple_item(path, leaves.into_iter())
}
}

And yet, the generated code does not have an absolute path. It's as if abs_path there is having no effect.

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.

This is weird. fs::read_to_string(&platform_rs_path).unwrap() is showing the use ::core::arch::asm;, but then the fs::read_to_string(&rs_path).unwrap() a few lines later is showing use core::arch::asm; again.

@kkysen
Copy link
Contributor

kkysen commented Oct 11, 2025

Ah, I figured out what it is. It's rustfmt. It's deleting the ::.

@kkysen kkysen changed the base branch from master to kkysen/snapshots-rustfmt-edition October 13, 2025 04:35
@kkysen kkysen force-pushed the kkysen/snapshots-rustfmt-edition branch from 400a962 to c9a70be Compare October 13, 2025 04:36
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.

@Rua, I rebased this on #1409 to test if it fixes the issue.

@Rua
Copy link
Contributor Author

Rua commented Oct 13, 2025

It seems to be fixed, but it is now getting stuck on the empty Ident issue fixed by #1411.

kkysen added a commit that referenced this pull request Oct 13, 2025
This is what was breaking
#1408 (comment).
@Rua, I think this should fix your issue. Let me know and you can review
this PR.
@kkysen kkysen deleted the branch immunant:master October 13, 2025 22:47
@kkysen kkysen closed this Oct 13, 2025
@kkysen
Copy link
Contributor

kkysen commented Oct 13, 2025

Ugh, why did this get closed?

@fw-immunant
Copy link
Contributor

Ugh, why did this get closed?

GH seems to suggest it was closed because the target branch was deleted.

@kkysen kkysen reopened this Oct 13, 2025
@kkysen kkysen changed the base branch from kkysen/snapshots-rustfmt-edition to master October 13, 2025 22:52
@kkysen
Copy link
Contributor

kkysen commented Oct 13, 2025

Ugh, why did this get closed?

GH seems to suggest it was closed because the target branch was deleted.

Ugh, GitHub being dumb again. Normally when you delete the target branch, it changes it to master (or whatever the target branch of the PR that was closed is), but apparently when it's an external PR, it just closes the PR.

@Rua
Copy link
Contributor Author

Rua commented Oct 14, 2025

Hmm strange, the CI is still failing on the same error. Is it perhaps using an old version?

EDIT: But the MacOS test passed... huh?

@kkysen
Copy link
Contributor

kkysen commented Oct 14, 2025

Hmm strange, the CI is still failing on the same error. Is it perhaps using an old version?

EDIT: But the MacOS test passed... huh?

I'm not sure, but the error message is slightly different. Might be a different error?

@Rua
Copy link
Contributor Author

Rua commented Oct 14, 2025

Transpile is able to generate a new Cargo.toml when transpiling, and it will fill it with dependencies used by the transpiled code, including c2rust-bitfields. Is it possible that the file it generates there refers to the stable/crates.io version of c2rust-bitfields, and not the one in the current directory tree? If so, that could explain what's going on maybe.

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.

Transpile is able to generate a new Cargo.toml when transpiling, and it will fill it with dependencies used by the transpiled code, including c2rust-bitfields. Is it possible that the file it generates there refers to the stable/crates.io version of c2rust-bitfields, and not the one in the current directory tree? If so, that could explain what's going on maybe.

I think you're right. And it seems to be using c2rust-bitfields= "0.3" from what I can tell, which is very old.

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.

Qualified paths like libc::c_int should be globally qualified like ::libc::c_int to avoid name clashes

3 participants