Define immutable statics with const qualified types by yvt · Pull Request #165 ·...

What is the size of an optimized hello world before and after this PR?

@bjorn3

$ cargo new --bin hello
$ cd hello
$ echo '[profile.release]' >> Cargo.toml
$ echo 'panic = "abort"' >> Cargo.toml

---- cg_gcc master ----
$ rustc_codegen_gcc/cargo.sh build --release
$ size target/release/hello
   text    data     bss     dec     hex filename
 358162   34944     112  393218   60002 target/release/hello

---- cg_gcc #165 ----
$ rustc_codegen_gcc/cargo.sh build --release
$ size target/release/hello
   text    data     bss     dec     hex filename
 389882    1880       8  391770   5fa5a target/release/hello

---- cg_llvm ----
$ cargo +nightly-2022-03-26 build --release
$ size target/release/hello
   text    data     bss     dec     hex filename
 291793   11792     536  304121   4a3f9 target/release/hello

Perhaps comparing the sizes with only one Rust codegen unit would give a better comparison.

With -Ccodegen-units=1 for app:

$ echo 'codegen-units = 1' >> Cargo.toml

---- cg_gcc master ----
   text    data     bss     dec     hex filename
 358018   34944     112  393074   5ff72 target/release/hello

---- cg_gcc #165 ----
   text    data     bss     dec     hex filename
 389738    1880       8  391626   5f9ca target/release/hello

---- cg_llvm ----
   text    data     bss     dec     hex filename
 291793   11792     536  304121   4a3f9 target/release/hello

With -Ccodegen-units=1 for app and sysroot:

$ echo 'codegen-units = 1' >> Cargo.toml
$ echo 'codegen-units = 1' >> rustc_codegen_gcc/build_sysroot/Cargo.toml

---- cg_gcc master ----
   text    data     bss     dec     hex filename
 311747   28808      88  340643   532a3 target/release/hello

---- cg_gcc #165 ----
   text    data     bss     dec     hex filename
 337443    1848       8  339299   52d63 target/release/hello

The call to is_const() will require to be feature-gated for libgccjit12.
I'll add it to libgccjit_without_int128.so (edit: Done).

I realized that a PlaceRef having a const qualifier in its pointee type may be created in violation of the following invariant. I'd like to poke around a bit more.

The call to is_const() will require to be feature-gated for libgccjit12.

Do you want me to disable this PR's behavior entirely for libgccjit12, or ...?

The call to is_const() will require to be feature-gated for libgccjit12.

Do you want me to disable this PR's behavior entirely for libgccjit12, or ...?

Just adding #[cfg(feature="master")] before the line using is_const() should do it.
To avoid the warning of unused mut, it might be better to create a new variable with the same name instead.

The const qualifier is now stripped as soon as a static's address is taken. This prevents const-qualified pointers, which shouldn't appear otherwise, from circulating and may fix bugs that we are not aware of.

Also feature-gates the use of is_const by #[cfg(feature="master")].

Can you explain to me why you canonicalize the rvalue? I don't understand why it's done.

Basically, when taking a pointer of a static, we don't want to get different pointer types depending on whether the static is mutable (T *) or immutable (T const *). Otherwise we'll get libgccjit errors like #165 (comment) and #165 (comment). Also that's a violation of an internal invariant (#165 (comment)).

Otherwise we'll get libgccjit errors like

Do you mean that was added as a workaround to libgccjit, to be able to load the address of a global variable?

If so, maybe there's a better solution. Thinking out loud, it would be great if we could know in Builder::load whether this is a global constant: in that case, we would not need to assign the value to a local variable; we could just return the dereference of the pointer to the global constant (this is safe as it is constant).
The problem with this option is that we need to know whether this is a pointer to a global: perhaps we know it just by looking at the keys of CodegenCx.global_lvalues.

Since that might not be easy to do (and the solution posted above is a bit hackish), another option could be to set an initializer to the local variable. Not sure if that would work as well (because the needs to be allowed in initializers), so perhaps you would have to only do that when the pointer is const (but then, we can surely have a const pointer to a non-global).

What do you think of this?

Now that I think of it, those two ideas do not seem better than yours. However, I worry about the effect of only having non-const pointer (could that change the code generation?). Perhaps it would be better to only call canonicalize in Builder.load?

Do you mean that was added as a workaround to libgccjit, to be able to load the address of a global variable?

To be more precise, it's to make the local variable to store the loaded data assignable. (A const-qualified local variable cannot be assigned to.) I'd rather see this as a symptom of inconsistent typing of rvalues, which this "canonicalization" fixes.

Meanwhile, #170 has changed the situation; the local variable created in load is now explicitly typed by the caller, and the source pointer is automatically bit-cast, which removes the possibility of this libgccjit error. In my quick far-from-comprehensive testing, there seems to be no remaining ways by which const-qualified pointers can cause visible issues. This gives us another option: proceed without canonicalization. I recommend against doing so, but I guess it's your call.

I worry about the effect of only having non-const pointer (could that change the code generation?).

Assuming that the semantics of libgccjit instructions is similar to that of C, the presence of const qualifier only matters for the mutability of variables (T var vs T const var). Const qualifiers in other positions can be cast in or away anytime and (afaik) mean nothing.

This gives us another option: proceed without canonicalization. I recommend against doing so, but I guess it's your call.

Why would you recommend to still do canonicalization?
I try to keep the same structure as the LLVM codegen in order to keep up with the changes there and also in the hope that I can refactor common code into rustc_codegen_ssa some day. This PR changes the structure a bit with this canonicalization and static_addr_of_inner, so that's why I want to know your opinion on this.

Const qualifiers in other positions can be cast in or away anytime and (afaik) mean nothing.

Good to know. I worried about TBAA doing something here, but it's disabled in Rust anyway.

Why would you recommend to still do canonicalization? I try to keep the same structure as the LLVM codegen in order to keep up with the changes there [...]

I'm starting to understand your design intent. But as you probably already know, there are differences between LLVM and libgccjit's APIs, and sometimes we have to make compromise and take an approach unique to cg_gcc. And when we do, the best thing we can do to mitigate the impact is to contain it to a narrow scope.

Why did I add static_addr_of_inner? libgccjit doesn't have a counterpart of LLVMSetGlobalConstant, which means the immutability of a global variable must be realized by applying a const qualifier on its type. The variable type must be specified at the definition time, which means static_addr_of can't just call static_addr_of_mut and change the variable's immutability later, hence the addition of static_addr_of_inner.

Realizing immutability in this way has a side effect: the pointer taken from an immutable global variable now has a different type. In one case (#165 (comment)) this actually caused a type mismatch in a module that is seemingly unconnected to this change. This particular case was mitigated by #170, but we don't know to what extent this will cause problems in the present or the future. This is a problem specific to libgccjit, so cg_llvm will continue to assume that the pointer types of immutable global variables and mutable ones are identical, and whenever we port its changes to cg_gcc, we risk breakage because this assumption does not hold for libgccjit. This brings us to a solution: Cast the taken pointer immediately ("canonicalization"), thus localizing the problem to a module that is responsible for dealing with global variables.

In conclusion, doing canonicalization here keeps the code free of latent bugs, improves the code quality, and minimizes risks, which is why I'm still in favor of doing this.

That makes a lot of sense, so let's keep it that way.

You could please add these explanations in the code as comments?

I believe I could make the equivalent of LLVMSetGlobalConstant in libgccjit.
Do you think it would be worth it?

(Sorry for all the back-and-forth.)

Thanks for all your work!

I believe I could make the equivalent of LLVMSetGlobalConstant in libgccjit.
Do you think it would be worth it?

Yes, I think so. Though, I'm curious how willing the libgccjit maintainers are to accept the patch, especially considering that it's not strictly necessary and doesn't directly map to any of the existing GCC variable attributes.

There was no such concern when I added a new API to set the alignment while it was already possible to set it via the type, so my guess is that it's gonna be OK. (Sure, there's a GCC attribute for alignment, but still.)

In that case, I'd say go for it.

Ok, I'll do it in the following days.

Ok, I added the function in this commit and in the bindings as well.

I just wanted to make sure that using the new readonly API it gives the same result.
Is it the case?

Yes, it's a perfect match size-wise.

$ cargo new --bin hello
$ cd hello
$ echo '[profile.release]' >> Cargo.toml
$ echo 'panic = "abort"' >> Cargo.toml
$ echo 'codegen-units = 1' >> Cargo.toml
$ echo 'codegen-units = 1' >> rustc_codegen_gcc/build_sysroot/Cargo.toml

---- cg_gcc master ----
$ rustc_codegen_gcc/cargo.sh build --release
$ size target/release/hello
   text    data     bss     dec     hex filename
 311995   30920     392  343307   53d0b target/release/hello

---- cg_gcc 7bb9fd8 (#165 old) ----
$ size target/release/hello
   text    data     bss     dec     hex filename
 339755    1848       8  341611   5366b target/release/hello

---- cg_gcc b8c82b1 (#165 new) ----
$ size target/release/hello
   text    data     bss     dec     hex filename
 339755    1848       8  341611   5366b target/release/hello