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

Amalgamate and c2rust transpile #1

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Amalgamate and c2rust transpile #1

wants to merge 10 commits into from

Conversation

kkysen
Copy link
Collaborator

@kkysen kkysen commented Apr 2, 2024

This was based on lighttpd-rust's amalgamation and transpilation.

}
- Alignment = &mut (*(0 as *mut C2RustUnnamed_25)).Align as *mut CFE_ES_PoolAlign_t as cpuaddr;
+ Alignment = core::mem::align_of::<CFE_ES_PoolAlign_t>() as _;
if Alignment < 4 as libc::c_int as libc::c_ulong {

Choose a reason for hiding this comment

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

Suggested change
if Alignment < 4 as libc::c_int as libc::c_ulong {
if Alignment < 4 as libc::c_int as size_t {

from #2 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, sure, but I can just merge in this change from #2 next, right?


const commands = JSON.parse((await fsp.readFile("compile_commands.json")).toString())
.map(({ directory, file, output, command, arguments: args }) => {
const relativePath = pathLib.relative(".", file);

Choose a reason for hiding this comment

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

If we enable the tests, we get duplicate files. We should check for that (e.g. see #2, or use whatever approach you think works).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember exactly, but I think part of the problem is that even though there are duplicate files, they do different things (like the unit tests). And since we don't care as much about the unit tests (at least I made that decision; if it's incorrect, we can switch things), I excluded them all. If we just only choose the first of each file, then we may exclude the versions of a file that we actually want.

const flagsWithoutDefines = flags.filter(flag => !flag.startsWith(definePrefix));

const includeLines = () => [
...defines().map(({ name, value }) => `#define ${name} ${value}`),

Choose a reason for hiding this comment

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

The macro might have already been defined elsewhere. I think we should do something like (see #2)

#pragma push_macro("${name}")
#undef ${name}
#define ${name} ${value}
...
#undef ${name}
#pragma pop_macro("${name}")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How does {push,pop}_macro work? If it works the way I assume it does, then yes, this seems more robust and handles macros, too.

};
})
// Exclude unit tests as that compiles things twice, leading to redefinition errors.
.filter(e => !e.defines().find(define => define.name === "_UNIT_TEST_"))

Choose a reason for hiding this comment

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

See comment above: couldn't we just check for duplicates?

"#define _GNU_SOURCE",
"#include <pthread.h>",
"#include <sched.h>",
"#undef _GNU_SOURCE",

Choose a reason for hiding this comment

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

Do we need to do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IIRC it didn't build without this, since otherwise the first use of pthread.h and sched.h didn't have _GNU_SOURCE defined yet, so it didn't define the necessary functions, but we can't define _GNU_SOURCE for every translation unit, since some have it undefined (I think).



function toShellCommand(cmd) {
const command = cmd.arguments && cmd.arguments.join(" ") || cmd.command;

Choose a reason for hiding this comment

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

Do we need to escape the arguments here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably?

mv ${rust_dir} ${rust_dir}.old
cargo new ${rust_dir}
(cd "${rust_dir}"
cargo add libc

Choose a reason for hiding this comment

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

We should pin these and all their dependencies. I'm already running into problems with cc updating to version 1.1.0 and asking for a newer rustc.

Choose a reason for hiding this comment

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

Alternatively, could we just freeze Cargo.toml and Cargo.lock?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alternatively, could we just freeze Cargo.toml and Cargo.lock?

Yeah, maybe.

cargo add memoffset
cargo add f128

# mv ../${rust_dir}.old/build.rs . # Don't need it.

Choose a reason for hiding this comment

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

Why not delete this line?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was from lighttpd-rust/amalgamate.mjs. And since I did things very quick and dirty, I wanted to leave in stuff I may want to re-enable in the future if I encountered problems or something.


rust_dir=${name}_rust_amalgamated

# export CC=clang

Choose a reason for hiding this comment

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

Delete all the commented out lines?

@@ -0,0 +1,264 @@
#!/usr/bin/env node

Choose a reason for hiding this comment

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

In the future, could we use either Python or shell scripts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. Shell probably isn't a good idea; it's too complex. But we could do Python. It started out as some very basic scripting, and since compile_commands.json is JSON, I used the node REPL, and then it grew, but I only worked on it for a couple of days quickly and dirtily, so it wasn't worth rewriting it much. And for cfs-rust, I just copied the one from lighttpd-rust and modified it.

const { stderr } = await runCommand(amalgamated.o, "-fdiagnostics-format=json");
const errors = JSON.parse(stderr);
return errors
.filter(error => error.message.match(/^rede(fini|clara)tion of /))

Choose a reason for hiding this comment

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

One problem we have here are macro redefinitions, but I'm not sure how to solve that because we can't rename them using #define.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah, that's a problem. Did you run into any of them?

await fsp.writeFile(amalgamatedPath("compile_commands.json"), JSON.stringify(commands, null, 4));
}

async function main() {

Choose a reason for hiding this comment

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

Are we getting any value out of async here? It seems like each step blocks on the previous one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You can't really block in JS, so stuff like file writing is always async. Perf doesn't matter, though, so it doesn't need any concurrency.

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.

2 participants