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

StmtExpr dropped during translation (was lseek not translated) #181

Closed
emmericp opened this issue Oct 23, 2019 · 18 comments
Closed

StmtExpr dropped during translation (was lseek not translated) #181

emmericp opened this issue Oct 23, 2019 · 18 comments
Assignees
Labels
bug Something isn't working

Comments

@emmericp
Copy link

emmericp commented Oct 23, 2019

example:

#include <unistd.h>
#include <fcntl.h>
#include <sys/stat.h>

uintptr_t virt_to_phys(void* virt) {
	long pagesize = sysconf(_SC_PAGESIZE);
	int fd = open("/proc/self/pagemap", O_RDONLY);
	lseek(fd, (uintptr_t) virt / pagesize * sizeof(uintptr_t), SEEK_SET);
	uintptr_t phy = 0;
	read(fd, &phy, sizeof(phy));
	return (phy & 0x7fffffffffffffULL) * pagesize + ((uintptr_t) virt) % pagesize;
}

is translated to

#![allow(dead_code,
         mutable_transmutes,
         non_camel_case_types,
         non_snake_case,
         non_upper_case_globals,
         unused_assignments,
         unused_mut)]
extern crate libc;
extern "C" {
    #[no_mangle]
    fn read(__fd: libc::c_int, __buf: *mut libc::c_void, __nbytes: size_t)
     -> ssize_t;
    #[no_mangle]
    fn sysconf(__name: libc::c_int) -> libc::c_long;
    #[no_mangle]
    fn open(__file: *const libc::c_char, __oflag: libc::c_int, _: ...)
     -> libc::c_int;
}
(...)
#[no_mangle]
pub unsafe extern "C" fn virt_to_phys(mut virt: *mut libc::c_void)
 -> intptr_t {
    let mut pagesize: libc::c_long = sysconf(_SC_PAGESIZE as libc::c_int);
    let mut fd: libc::c_int =
        open(b"/proc/self/pagemap\x00" as *const u8 as *const libc::c_char,
             0i32);
    let mut phy: intptr_t = 0i32 as intptr_t;
    read(fd, &mut phy as *mut intptr_t as *mut libc::c_void,
         ::std::mem::size_of::<intptr_t>() as libc::c_ulong);
    panic!("Reached end of non-void function without returning");
}
@emmericp
Copy link
Author

similar (related?) problem:

#include <sys/stat.h>
#include <sys/mman.h>
#include <fcntl.h>
#include <sys/file.h>

void* pci_map_resource() {
         char* path = "/sys/bus/pci/devices/0000:01:00.0/resource0";
         int fd = open(path, O_RDWR);
         struct stat stat;
         fstat(fd, &stat);
         return mmap(NULL, stat.st_size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
}

skips the mmap() call for some reason

the function we are actually trying to translate is https://github.com/emmericp/ixy/blob/57679a086343b9ae9d97e059e13a3226482910a6/src/pci.c#L42 which curiously fails for another reason: it skips the fstat call but does the mmap call, I was just trying to minimize it

the examples included in the posts were checked with https://c2rust.com/ the actual code linked was converted with current c2rust git master (current crates.io release didn't compile)

@emmericp
Copy link
Author

There's another place in the code that uses lseek that seemed to be translated properly: https://github.com/emmericp/ixy/blob/57679a086343b9ae9d97e059e13a3226482910a6/src/pci.c#L27

@ackxolotl
Copy link

We did get a few warnings when transpiling the C code:

Transpiling ixy-fwd.c
warning: Missing top-level node with id: 94248191120760
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248191107872
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248191107408
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
Transpiling pci.c
Transpiling memory.c
warning: Missing top-level node with id: 94248195610792
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248195610112
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248195609648
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
Transpiling stats.c
warning: Missing top-level node with id: 94248196296208
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248196291424
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248196290960
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
Transpiling device.c
warning: Missing top-level node with id: 94248193395480
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248193394832
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248193394368
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
Transpiling ixgbe.c
warning: Missing top-level node with id: 94248196280504
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248196279856
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]
warning: Missing top-level node with id: 94248196279392
Exported Clang AST was invalid. Check warnings above for unimplemented features.
 [-Wclang-ast]

Unfortunately, c2rust doesn't state which functions where omitted?

@emmericp
Copy link
Author

Well, looks like we need the stdint.h include for the minimal example on the website, so no minimal example, sorry :(

This is the actual code we are trying to translate which lacks the lseek call in the output: https://github.com/emmericp/ixy/blob/57679a086343b9ae9d97e059e13a3226482910a6/src/memory.c#L16

@TheDan64
Copy link
Contributor

TheDan64 commented Oct 23, 2019

I think check_err might be the culprit here as this seems to reproduce the issue on master:

#define check_err(expr, op) ({\
    int64_t result = (int64_t) (expr);\
    if ((int64_t) result == -1LL) {\
            int err = errno;\
            char buf[512];\
            strerror_r(err, buf, sizeof(buf));\
            fprintf(stderr, "[ERROR] %s:%d %s(): Failed to %s: %s\n", __FILE__, __LINE__, __func__, op, buf);\
            exit(err);\
    }\
    result;\
})

int bar(void) {
    return 1;
}

void foo(void) {
    check_err(bar(), "afoo");
}

int main(void) {
    foo();
}

c2rs seems to be ignoring the expanded macro contents for some reason.

@rinon
Copy link
Contributor

rinon commented Oct 23, 2019

I just translated the minimal example using github master, and it seems fine. What OS are you translating on? check_err may be #ifdefd out in the configuration you're building in?

I just tried translating ixy on the latest Arch linux, and it fails to translate a static_assert, but I'm not sure if that's what's impacting you. I'll look into that for now.

@rinon
Copy link
Contributor

rinon commented Oct 23, 2019

the examples included in the posts were checked with https://c2rust.com/ the actual code linked was converted with current c2rust git master (current crates.io release didn't compile)

Could you elaborate on build issues? The crates.io release should compile.

@emmericp
Copy link
Author

check_err worked fine in most places and did in fact catch the bugs, e.g., the missing fstat call was caught by this check_err on mmap here https://github.com/emmericp/ixy/blob/57679a086343b9ae9d97e059e13a3226482910a6/src/pci.c#L51 because it tried to mmap a size of 0

We've manually fixed the two obvious mistranslations (the fstat and lseek calls linked above), it then starts up fine but doesn't work, so I guess there are more failures that we didn't catch. Basic initialization seems fine, it successfully initializes the device, the link comes up and it reads statistics registers showing correct packet counts. But it doesn't actually retrieve any packets from the NIC which is unfortunately very hard to debug

build issues: @ackxolotl

@TheDan64
Copy link
Contributor

TheDan64 commented Oct 24, 2019

That's weird, I get an empty foo body for my example in Ubuntu 16

@ackxolotl
Copy link

Could you elaborate on build issues? The crates.io release should compile.

Well, it turned out that after uninstalling the c2rust git version, cargo +nightly-2019-10-04 install c2rust just works fine. But it didn't beforehand. @emmericp and I had this same issue on Mac OS and Arch Linux. Unfortunately we are now unable to reproduce the issue. We tried to clean the cargo cache by deleting ~/.cargo/registry and ~/.cargo/git but with no effect. Any ideas on that?

@emmericp
Copy link
Author

I can also reproduce the build issues on my Mac:

$ brew install llvm python3 cmake openssl
$ rustup install nightly-2019-10-04
$ rustup component add --toolchain nightly-2019-10-04 rustfmt
$ LLVM_CONFIG_PATH=/usr/local/opt/llvm/bin/llvm-config cargo +nightly-2019-10-04 install c2rust
(...)
error[E0433]: failed to resolve: use of undeclared type or module `Guard`
   --> /Users/paul/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/c2rust-ast-builder-0.11.0/src/builder.rs:966:35
    |
966 |         let guard = guard.map(|g| Guard::If(g.make(&self)));
    |                                   ^^^^^ use of undeclared type or module `Guard`
error[E0433]: failed to resolve: use of undeclared type or module `Arg`
    --> /Users/paul/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/c2rust-ast-builder-0.11.0/src/builder.rs:2097:9
     |
2097 |         Arg::from_self(attrs, eself, ident)
     |         ^^^ use of undeclared type or module `Arg`
(... and more like that)

compiling git master works

@rinon
Copy link
Contributor

rinon commented Oct 24, 2019

Could you elaborate on build issues? The crates.io release should compile.

Well, it turned out that after uninstalling the c2rust git version, cargo +nightly-2019-10-04 install c2rust just works fine. But it didn't beforehand. @emmericp and I had this same issue on Mac OS and Arch Linux. Unfortunately we are now unable to reproduce the issue. We tried to clean the cargo cache by deleting ~/.cargo/registry and ~/.cargo/git but with no effect. Any ideas on that?

I hadn't pushed the latest version to crates.io (did that last night), so the build instructions on github specified a newer rustc nightly version than the crates.io package required. I'm guessing something along those lines was happening.

I'll check and see if I can reproduce the install issue on Mac.

@emmericp
Copy link
Author

can confirm that it builds fine now on my mac; the log from above is a build i started yesterday evening. we'll retry to translate our C code tomorrow

@rinon
Copy link
Contributor

rinon commented Oct 24, 2019 via email

@rinon rinon changed the title lseek not translated StmtExpr dropped during translation (was lseek not translated) Oct 24, 2019
@rinon rinon self-assigned this Oct 24, 2019
@TheDan64 TheDan64 added the bug Something isn't working label Oct 29, 2019
@TheDan64
Copy link
Contributor

The checked_err issue should be fixed on master now. Please feel free to reopen or comment if not

@emmericp
Copy link
Author

it's working now, thanks :)

And the transpiled code is actually 17% faster than the original C code when compiled with the same LLVM version :O

@emmericp
Copy link
Author

emmericp commented Oct 30, 2019

Not sure if this is related, but I got a few warnings like this during compilation:

warning: unnecessary parentheses around assigned value
    --> src/ixgbe.rs:1234:9
     |
1234 | /         ({
1235 | |              let mut __mptr: *const ixy_device = ixy;
1236 | |              (__mptr as
1237 | |                   *mut libc::c_char).offset(-(0 as libc::c_ulong as isize)) as
1238 | |                  *mut ixgbe_device
1239 | |          });
     | |___________^
help: remove these parentheses

probably caused by the container_of macro: https://github.com/emmericp/ixy/blob/minimal-ixgbe-fwd/src/device.h#L28

@TheDan64
Copy link
Contributor

TheDan64 commented Oct 30, 2019

We generate extra parens in some cases due to this rust issue: rust-lang/rust#54482

It might be possible to determine it's not necessary in this particular case, but it'd probably require some extra analysis. And cargo fix would probably remove them for you anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants