Add definition splitting to transpiler

[fw-immunant]

Currently this is just the C-side implementation; I'll add commits moving the impl of Rust splitting and merging from the crisp repository tomorrow.

[ahomescu]

Not against this approach per se, just wanted to bring up the alternative approach of using the existing c2rust::src_loc annotations for this. Do we not want to use those?

[fw-immunant]

Not against this approach per se, just wanted to bring up the alternative approach of using the existing c2rust::src_loc annotations for this. Do we not want to use those?

I discussed this with @thedataking in a call.

One problem is that we only have the c2rust::src_loc annotations if the transpiler was invoked with --reorganize-definitions, which may not always be the case. We could just say that we require this flag (or another that would also cause these attributes to be emitted) for comment reinsertion, but then it may be the case that these attributes themselves are surprising to the LLM and damage performance. Emitting this information out of band keeps this functionality independent and simplifies using it in other tools; our other tooling on the CRISP side currently handles a similar format of a flat JSON string->string map.

The other problem is that even given said annotations, we only know the start of the C definition, not its entire bounds (nor bounds of any relevant preceding comments). Most of the logic here is in getting precise bounds from the information in the Clang AST, which is otherwise lost. So we would need to add that logic somewhere even if we were going to use the c2rust::src_loc attribute for this.

[fw-immunant]

We can't practically integrate the Rust splitting/merging tools into our Cargo.toml because they use Rust 2024 and have dependencies that also do. But I don't think it's a problem to just have them live in their own subdirectories for now; they just have to be built before the c2rust-postprocess tool can invoke them.

[kkysen]

We can't practically integrate the Rust splitting/merging tools into our Cargo.toml because they use Rust 2024 and have dependencies that also do. But I don't think it's a problem to just have them live in their own subdirectories for now; they just have to be built before the c2rust-postprocess tool can invoke them.

That seems fine for now. When we get around to #1227, we can add it back.

[fw-immunant]

We now seem to extract correct source ranges for all the definitions in libxml2 and lua in the testsuite.

[thedataking]

For me, that picks up the c2rust nightly toolchain and fails because it is too old. I had to run cargo +stable build --release.

[fw-immunant]

Oh, that's because I was running actual cargo, not rustup aliased to cargo. In the latter case (which is probably more common) your command is the right one; I'll clarify. Or alternately, rustup might pick up a rust-toolchain.toml if we placed one somewhere.

[kkysen]

How do I test this? Also, do we need to merge the --emit-c-decl-map changes to c2rust-transpile/c2rust-ast-exporter at the same time as the new tools/? The former looks mostly good to me, but I haven't reviewed the latter. Also, is it worth it to review split_ffi_entry_points, as FWIU, it's already in crisp?

[kkysen]

Suggested change

	decls_sorted.sort_by(|v1, v2| {
	self.c_decls[v1]
	.begin_loc()
	.cmp(&self.c_decls[v2].begin_loc())
	});
	decls_sorted.sort_by_key(|v| self.c_decls[v].begin_loc());

[kkysen]

Could you also move the sorting to before name_loc_map and prev_src_loc, which are only used in the loop?

[kkysen]

It's probably best to just write to a Vec<u8>/String and then write the file all at once. You could also write to a buffered writer, but that's easy to forget, like here.

[kkysen]

Suggested change

	"Unable to write C declaration map to file {}: {}",
	output_path.display(),
	e
	"Unable to write C declaration map to file {}: {e}",
	output_path.display(),

[kkysen]

Could we use a .expect() here or fs_err? A file missing is a fairly common error, so better context is important.

[fw-immunant]

Probably a good idea, will do.

[kkysen]

What's VT?

[kkysen]

Vertical tab?

[fw-immunant]

Yep, ASCII VT, which has gone out of fashion so Rust lacks an escape for it.

[kkysen]

Could we put this in its own function? I think it'd be easier to follow that way.

[fw-immunant]

Yeah, an emit_c_decl_map function would probably be good to avoid bloating fn translate.

[kkysen]

decl_source_ranges.map(|decl_sources_ranges| {})?

[kkysen]

What's the memchr for?

[fw-immunant]

It would be a more efficient equivalent, but I don't think we have the dependency and this hasn't been a bottleneck.

[kkysen]

Could you use u8::is_ascii_whitespace + ;?

[fw-immunant]

It doesn't include \v, but this could be expressed using that function if we wanted. I don't know if it's clearer, given the specific set of characters we care about.

[kkysen]

Hmm. is_ascii_whitespace also handles FF but not VT. Is VT worth handling at all? I guess it doesn't matter that much. I don't imagine anything will actually produce VTs or FFs.

[fw-immunant]

How do I test this? Also, do we need to merge the --emit-c-decl-map changes to c2rust-transpile/c2rust-ast-exporter at the same time as the new tools/? The former looks mostly good to me, but I haven't reviewed the latter. Also, is it worth it to review split_ffi_entry_points, as FWIU, it's already in crisp?

The flag is independent from tools but intended to be used alongside them in the CRISP loop. This PR doesn't include any automated testing yet, but I've been manually testing just by invoking rustc with the flag and examining the produced .c_decls.json files.

In addition to the testsuite, I've been using the following test case which I'll probably add as a snapshot test:

#include <stddef.h>
#include <stdint.h>
int a;int bb;/*comment for cc*/int ccc;
/*
123456789012345678901234567890
*/
typedef uintptr_t ngx_uint_t;
void f(void){}void g(void){}/*comment for h*/void h(void){}int d;int e;;;;;;;;int another;
//comment for ngx_hash_init
void
ngx_hash_init(void)
{
    size_t len;
    ngx_uint_t align = len & ~sizeof(void *);
}
int
anotherfunc(void)
{
    int var = 0;
    return 4 + var;
}
int
funcholdingdefine(void)
{
#define FOO 4000+50
    return FOO;
}
void forwarddeclbeforeotherdef(int x);
void funcafterdecl(int n) {}
/*real defn*/
void forwarddeclbeforeotherdef(int x) { }