rust-gcc / cargo-gccrs Goto Github PK

This should replace the use of fake_output here.

Maybe the parsing and the translation should be split? So the parsing would create a struct like https://github.com/rust-lang/rust/blob/456a03227e3c81a51631f87ec80cac301e5fa6d7/compiler/rustc_session/src/options.rs#L66-L71 (maybe even just copy this file verbatim? that way the parsing is guaranteed to be correct.) and then translation would create the appropriate arguments for gccrs.

Originally posted by @bjorn3 in #23 (comment)

Otherwise this causes the CI to consistently fail.

Since Rust-GCC/gccrs#788 was merged, we can now set the crate name in gccrs via the cargo wrapper

IIUC most of the Options are common to all gccrs invocations. So

rustc --crate-type=dyn --crate-type=bin src/<file>.rs --other-flag-1 --other-flag-2

becomes

gccrs src/<file>.rs --converted-other-flag-1 --converted-other-flag-2  # bin
gccrs -shared src/<file>.rs -o libfile.so --converted-other-flag-1 --converted-other-flag-2  # shared lib

where --converted-other-flag-1 --converted-other-flag-2 are common to all invocations. So why not only store them once?

pub struct OptionsCollection {
    common_args: Vec<Arg>,
    crate_types: Vec<CrateType>,
    ... // Other fields that require multiple gccrs invocations, if any
}

I think that would make things way cleaner and would avoid saving the common options and input+output file name N times.

Maybe even:

pub struct Args {
    input_files: Vec<String/PathBuf>,
    output_file: String/PathBuf,
    common_args: Vec<Arg>,
    crate_types: Vec<CrateType>,
    ... // Other fields that require multiple gccrs invocations, if any
}

And only once you generate the gccrs commands, you will build the whole Vec<Vec> argument matrix.

Originally posted by @flip1995 in #47 (comment)

The binary should not panic on errors but simply write information to stderr and return an exit code different than 0.

We will probably need to look into test runners, as well as see how gccrs handles attributes such as #[test] and #[cfg(test)]

This is something I think you should consider doing when you are finishing up with google summer of code.

I think being able to rely on bors will be nice for the long term of the project.

When building a simple binary executable (tests/binary_project), cargo build && readelf -h target/debug/binary_project produces the following output:

ELF Header:
  Magic:   7f 45 4c 46 02 01 01 00 00 00 00 00 00 00 00 00
  Class:                             ELF64
  Data:                              2's complement, little endian
  Version:                           1 (current)
  OS/ABI:                            UNIX - System V
  ABI Version:                       0
  Type:                              DYN (Shared object file)
  Machine:                           Advanced Micro Devices X86-64
  Version:                           0x1
  Entry point address:               0x6760
  Start of program headers:          64 (bytes into file)
  Start of section headers:          3351096 (bytes into file)
  Flags:                             0x0
  Size of this header:               64 (bytes)
  Size of program headers:           56 (bytes)
  Number of program headers:         14
  Size of section headers:           64 (bytes)
  Number of section headers:         42
  Section header string table index: 41

Meaning that the produced binary is an executable shared object file. This is fine, but isn't what gccrs currently does.

I wonder if this behavior is something that should be reproduced in gccrs? In that case I'll open up a corresponding issue. I am assuming that the answer is yes, since there are probably valid reasons for rustc to produce a shared object instead of a simple binary.

Therefore, I'm also wondering about the reasons behind that choice? Maybe @bjorn3 you know about this?

For reference, the binary currently produced by gccrs is of type EXEC

This is a review of the binary library split branch: cleanup-error-handling

First posted in #21 (review), but I wanted to also track this outside of the PR.

I figured that this is now the right time to do a whole project review, and not only a review of #21. My suggestions don't have to be implemented all in one PR, but rather get implemented step by step. I tried to sort them from easy to hard to do.

• cfg!(release)

What is this line supposed to do?

• is_installed

You can use the which crate for cross-platform support for this

• serde for config dump/parsing?

What format is the config dump in and can you use serde for this?

• Config dump/parsing impl

A fallible display impl seems weird to me. You should rather implement a fallible constructor and then the Display trait. That way you can use the format machinery and get functions like to_string for free.

I'm also not 100% sure what the option dumping is for, so please tell me if there is a reason why it has to be done that way.

• multiple binaries?

I think you want to have 2 binaries here. One for the driver, that does all the argument conversion and other hard work, and one that just invokes the driver from the cargo command. But I may be misunderstanding something, see my comment #21 (comment)

• GccrsArgs not holding all arguments

I still don't quite get why it is necessary to return a Vec<GccrsArgs>. Just from the struct name and the documentation, I would assume that when I have a GccrsArgs instance, that I have all the arguments that I need. This is also the main thing that prevents the GccrsArgs from implementing things like the TryFrom trait and so on.

• GccrsArgs is doing too much

I still think that with this, GccrsArgs is doing too much. One good principle of OOP is that one class should only do one thing. With that principle, GccrsArgs should only handle arguments, which includes parsing, converting, and then holding them. Now it handles arguments and does random other things like creating archives and in the future possibly even more.

This is the area of the current implementation that really needs a refactor IMHO. Apply the KISS principle as often as you can. I have trouble following what is done where, so the current impl isn't really simple to follow.

rustc has the following behavior:

• rustc --crate-name test src/main.rs produces a binary named test.
• rustc --crate-name test --crate-type dylib src/main.rs produces a dynamic library named libtest.so
• Likewise, rustct ... --crate-type staticlibproduces a static library named libtest.a

However, gcc -shared -o test main.c produces a dynamic library named test. We should therefore modify the output parameter based on the type of crate required, so that we can produce a shared library named libtest.so even though the given crate-name will only be test

One of the first invocations of rustc is the following:

rustc - --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib ... --print=sysroot --print=cfg

The output expected is something like the following:

___
lib___.rlib
lib___.so
.../.rustup/toolchains/nightly-x86_64-unknown-linux-gnu
debug_assertions
panic="unwind"
target_arch="x86_64"
target_endian="little"
target_env="gnu"
target_family="unix"
target_feature="fxsr"
target_feature="sse"
target_feature="sse2"
target_has_atomic="16"
target_has_atomic="32"
target_has_atomic="64"
target_has_atomic="8"
target_has_atomic="ptr"
target_has_atomic_equal_alignment="16"
target_has_atomic_equal_alignment="32"
target_has_atomic_equal_alignment="64"
target_has_atomic_equal_alignment="8"
target_has_atomic_equal_alignment="ptr"
target_has_atomic_load_store="16"
target_has_atomic_load_store="32"
target_has_atomic_load_store="64"
target_has_atomic_load_store="8"
target_has_atomic_load_store="ptr"
target_os="linux"
target_pointer_width="64"
target_thread_local
target_vendor="unknown"
unix

We can adapt this output from gccrs with the option to dump config to create a corresponding valid output for cargo

$ cat gccrs.target-options.dump
unix
target_endian: "little"
target_pointer_width: "64"
target_feature: "slow-3ops-lea"
target_feature: "vzeroupper"
target_feature: "slow-shld"
target_feature: "fxsr"
target_feature: "macrofusion"
target_feature: "mmx"
target_feature: "fast-scalar-shift-masks"
target_feature: "64bit-mode"
target_feature: "slow-incdec"
target_feature: "cx8"
target_feature: "sse"
target_feature: "nopl"
target_feature: "cmov"
target_feature: "sse2"
target_feature: "64bit"
target_feature: "slow-unaligned-mem-16"
target_arch: "x86_64"
target_os: "linux"
target_family: "unix"
target_vendor: "unknown"
target_env: "gnu"

Thanks to @philberty for pointing me to the option

It's currently really annoying to add new arguments to give to our gccrs invocation. We should think about having an Argument trait which could be converted from a rustc arg, validated, and passed as a string to a gccrs invocation

As pointed by @philberty, the system needs to be highly configurable. For example, we need to allow the user to pass different options to ar, or maybe even to gccrs in order to suit their needs.

This could be done by using environment variables. For example, in the following code snippet:

        Command::new("ar")
            .args(&[
                "rcs", // Create the archive and add the files to it
                output_file,
                object_file_name(&output_file).as_str(),
            ])
            .status()?;

could use an environment variable instead of simply using "rcs" as the primary argument for ar (CARGO_GCCRS_AR_ARGUMENTS maybe?)

We could also have extra arguments for shared or static libraries, etc

This allows cargo to cache them and allows programs like rust-analyzer to show errors inline. The rustc json error format consists of two parts: The rendered diagnostic as rustc would show it when not using the json error format. This is used by cargo. And the diagnostic in a structured format describing each part of the error message, like which parts of the source are underlined.

$ echo "fn foo() {} # fn bar() {}" | rustc - --error-format json |& jq
{
  "message": "expected one of `!` or `[`, found keyword `fn`",
  "code": null,
  "level": "error",
  "spans": [
    {
      "file_name": "<anon>",
      "byte_start": 14,
      "byte_end": 16,
      "line_start": 1,
      "line_end": 1,
      "column_start": 15,
      "column_end": 17,
      "is_primary": true,
      "text": [
        {
          "text": "fn foo() {} # fn bar() {}",
          "highlight_start": 15,
          "highlight_end": 17
        }
      ],
      "label": "expected one of `!` or `[`",
      "suggested_replacement": null,
      "suggestion_applicability": null,
      "expansion": null
    }
  ],
  "children": [],
  "rendered": "error: expected one of `!` or `[`, found keyword `fn`\n --> <anon>:1:15\n  |\n1 | fn foo() {} # fn bar() {}\n  |               ^^ expected one of `!` or `[`\n\n"
}
{
  "message": "aborting due to previous error",
  "code": null,
  "level": "error",
  "spans": [],
  "children": [],
  "rendered": "error: aborting due to previous error\n\n"
}

You can also pass --json diagnostic-rendered-ansi to use color ansi escape codes for the rendered diagnostic.

As pointed out by @philberty here, we need to clean up the handling of extensions based on the system used. Therefore, .so and .a need to be acquired through a function call or a static variable whose behavior/value would change based on the environment.

As pointed out by @philberty here, we need to refactor the GccrsArg structure defined here and clean it up

Currently, cargo gccrs build does not show colored errors from the compiler

We now support the --edition argument and should use it in the cargo wrapper

Since --cfg support is now merged in gccrs, we should make use of it in the cargo wrapper

From reading through the code so far I am unsure if there is a way to turn on debug on which requires the -g option for gccrs. Also not sure how to turn on optimizations -O0, -O1, -O2, -O3 etc.

If there is a debug / release thing I would suggest debug should be -g -O0 and for release -g -O2 for now if you need default options.

A question, is there options in cargo to pass extra flags to rustc outside of cargo using an environment variable? Just wondering if we need a mechanism to be able to override these defaults with an environment variable.

This is the same crate as rustc uses.

In order to avoid always relying on cargo install --path . to test your changes, let's add some instructions regarding PATH manipulation and whatnot to enable faster feedback.

I think this should get split up in multiple PRs in order to better the review process. This issue shall become the tracking issue for the subtasks of the final PR.

After discussing it with @flip1995, it seems that running functional tests within the rust code would be best. This is what clippy does, for example here

This should be split into multiple sub-issues:

• Add valid tests application that gets run when executing cargo test #26
• Add abstraction around checking the validity of a test (exit code 0) and reporting this result to the developer #26
• The test harness shall only be compiled when running tests #26
• Checking that the objects created by both rustc and gccrs have the same filename #33
• Check for deps in target/[release|debug]/deps #33
• #41
• Check that static libraries are not empty #37
• #45
• Check that the objects created are of the correct file type: Executable, Shared object, Static object etc using readelf for example. #37

Once this test harness framework is done, add tests for the project

• Add functional testing to the CI #26
• Test the basic projects included in the repo as is (tests/binary_project, test/static_lib, etc...) #26
• #43
• #44

Since all of these PRs wouldn't make sense by themselves, I think it would be best to create them against a specific branch such as feature/functional-testing and then merge this branch into master once the test harness is complete.

#5 (comment)

This is more of a general question for cargo is there a way to emit a log or debug message for the commands being invoked? It might be a useful feature for debugging things.

Allow the generation of .a archive files. This will require spawning a separate ar command, and archiving some object files together.

Also a more general note: I saw that you have a main.rs and then a gccrs module.
Usually you just make a lib and add one small main.rs to keep this split. You could do this, by moving everything from src/gccrs/* to src/ and rename mod.rs to lib.rs

Originally posted by @flip1995 in #21 (comment)

Or how to implement --crate-type and --emit. My proposal would be the following:

cargo-gccrs invokes gccrs once with -fPIC to get the crate metadata, an object file, an assembly file, ... as necessary/requested. For all emit types except link, the respective file generated by gccrs is directly passed to the user. For link depending on the crate type(s) one of more of the following will be done:

• bin: Gcc will be invoked with the right arguments to link the final executable.
• lib/rlib: The crate metadata, object file and if necessary LTO metadata are packaged in a single archive.
• dylib: Gcc will be invoked with the right arguments to link a dylib with all symbols exported and the crate metadata embedded in the dylib.
• cdylib: Gcc will be invoked with the right arguments to link a dylib with only #[no_mangle] symbols exported and crate metadata not embedded.
• staticlib: The object file combined with the object files of all dependencies (rust and native archives) will be bundled in a single archive without crate metadata.
• proc-macro: Gcc will be invoked with the right arguments to link a proc-macro. (will have to wait until gccrs supports proc macros)

Gccrs will then have to learn how to read crate metadata from rlibs and dylibs.

This proposal should solve the problem of having to invoke gcc multiple times when using multiple crate types or when using --emit obj,link or something like that. It is also somewhat close to what rustc internally does except that some logic is moved from rustc_codegen_ssa::back::link to cargo-gccrs. It may also be possible to implement this logic in the gcc driver so you have -frust-crate-type=bin,lib,dylib.

WDYT @philberty?

Figure out how to get getopts() to print the argument and handle it gracefully, instead of expect()-ing on the function call.

Here is an example of the output produced when compiling a project where cargo gives us some arguments we can't parse yet:

> $ cargo gccrs build
   Compiling proc-macro2 v1.0.27
   Compiling unicode-xid v0.2.2
   Compiling syn v1.0.73
   Compiling serde_derive v1.0.126
   Compiling serde v1.0.126
   Compiling autocfg v1.0.1
   Compiling futures-core v0.3.15
   Compiling memchr v2.4.0
   Compiling libc v0.2.97
   Compiling proc-macro-hack v0.5.19
   Compiling futures-task v0.3.15
   Compiling futures-channel v0.3.15
   Compiling proc-macro-nested v0.1.7
   Compiling futures-io v0.3.15
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
error: could not compile `serde_derive`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'main' panicked at 'cannot translate rustc arguments into gccrs ones: InvalidArg', src/main.rs:29:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: build failed

which is not helpful

error: output of --print=split-debuginfo missing when learning about target-specific information from rustc
command was: `gccrs-driver rustc - --crate-name ___ --print=file-names --crate-type bin --crate-type rlib --crate-type dylib --crate-type cdylib --crate-type staticlib --crate-type proc-macro --print=sysroot --print=split-debuginfo --print=crate-name --print=cfg`

--- stdout
___
lib___.rlib
lib___.so
lib___.so
lib___.a
lib___.so
target_arch="x86_64"target_endian="little"target_env="gnu"target_family="unix"target_feature="fxsr"target_feature="mmx"target_feature="sse"target_feature="sse2"target_os="linux"target_pointer_width="64"target_vendor="unknown"

--- stderr
/usr/bin/ld: /lib/../lib64/crt1.o: in function `_start':
(.text+0x1b): undefined reference to `main'
collect2: error: ld returned 1 exit status

Error: `cargo-gccrs` did not complete succesfully

Reported by @Skallwar