Clarity on rules for bitfields in fp+int structs eligible for the FP calling convention about riscv-elf-psabi-doc HOT 11 CLOSED

I submitted #100 which documents the current behaviour. I think a better solution would probably be to always ignore zero-width bitfields, which seems consistent with how packed and aligned attributes don't affect a struct's potential eligibility for being passed by the floating-point calling convention.

from riscv-elf-psabi-doc.

I did an initial round of testing. I modified gcc to abort if it sees a struct that would be passed differently in C and C++. But since the C++ front end removes the zero-length bit-fields, this only works for the C compiler. I built newlib/elf and glibc/linux toolchains and ran the gcc testsuites. The only thing that triggered was 2 tests in our ABI compatibility testsuite, which is expected. Unfortunately, while we have support for testing one C compiler against another, and one C++ compiler against another, we don't have support for testing the C compiler against the C++ compiler. That looks like an oversight that should be fixed, though I doubt that I will have time to try to fix it myself.

I also did an open-embedded build, dropping in my gcc patch, and the image built without error. I built a core-image-lsb-sdk image that includes a native compiler, so quite a bit of stuff gets built, and it was all built successfully. I booted the image in qemu and verified that the system compiler called abort when fed the testcases. I don't know if there is a better open-embedded image for testing that will build more stuff.

At the moment, it looks like we can change the ABI and it won't have much affect. I'd modify the gcc RISC-V port to give a warning when it sees a struct whose ABI changed. I can add an option to choose whether people want the old ABI or the new ABI, and make it default to the new ABI. I'd need a nice name for the option if anyone wants to suggest one. Or we could just start adding numbers for different ABI versions, and have an option to chose the ABI version by number. The C++ front end supports a -fabi-version=X option to specify which C++ ABI version is supported. We could add a -mabi-version=X option for the RISC-V backend. Or we could add an option specific to this problem, something about zero-length bitfields.

I've got a riscv-gnu-toolchain regression build running which will test the default list of 26 or so multilibs but I'm not expecting a problem. And I will check to see if there is a bigger open-embedded image I can build to test more stuff.

from riscv-elf-psabi-doc.

from riscv-elf-psabi-doc.

If a struct has only one non-zero width field, then the gcc front end gives the struct the mode of that one field as this improves optimization. The gcc backend isn't checking for this case, so a struct with any number of zero width fields and exactly one non-zero width FP field gets treated as if it has only one FP field. This happens in riscv_get_arg_info when we call riscv_pass_mode_in_fpr_p which checks only the argument mode, but don't bother to check if it is a structure and if so how many fields are in it.

If a struct has more than 2 fields, then it is excluded as a candidate for passing in regs, even if one of those fields is a zero-size field, as we aren't checking for this case. This is a separate independent test than the one above. This happens in riscv_flatten_aggregate_field, which just counts the number of fields and doesn't exclude the zero-size ones. This is used by riscv_pass_aggregate_in_fpr_pair_p and riscv_pass_aggregate_in_fpr_and_gpr_p, so a zero size field plus two non-zero size fields prevents both the 2-fp reg passing case and the 1-fp reg+1gp reg passing case.

from riscv-elf-psabi-doc.

If a struct has only one non-zero width field, then the gcc front end gives the struct the mode of that one field as this improves optimization. The gcc backend isn't checking for this case, so a struct with any number of zero width fields and exactly one non-zero width FP field gets treated as if it has only one FP field. This happens in riscv_get_arg_info when we call riscv_pass_mode_in_fpr_p which checks only the argument mode, but don't bother to check if it is a structure and if so how many fields are in it.

If a struct has more than 2 fields, then it is excluded as a candidate for passing in regs, even if one of those fields is a zero-size fisize and eld, as we aren't checking for this case. This is a separate independent test than the one above. This happens in riscv_flatten_aggregate_field, which just counts the number of fields and doesn't exclude the zero-size ones. This is used by riscv_pass_aggregate_in_fpr_pair_p and riscv_pass_aggregate_in_fpr_and_gpr_p, so a zero size field plus two non-zero size fields prevents both the 2-fp reg passing case and the 1-fp reg+1gp reg passing case.

Hi Jim, @rofirrim reports that there seems to be inconsistency between C/C++ mode riscv64-linux-gcc (tested on 9.1.0), even when extern "C" is used the following struct is passed differently on depending on -x c vs -x c++ with the lp64d ABI:

struct s1 { int : 0; float f; long long int i; int : 0; };

void dummy(float, long long int);

void f(struct s1 s) {
  dummy(s.f + 1.0, s.i + 1);
} 

For C the struct is passed in a0 and a1, while for C++ it's passed in fa0 and a0.

Can you please clarify which behaviour we should consider correct?

from riscv-elf-psabi-doc.

The fundamental problem as mentioned before is that the RISC-V backend isn't checking for and handling zero-length fields. Looking at this problem, I see that the C++ front end is deliberately removing all zero-length fields after doing layout, i.e. after assigning bit offsets to each field, as zero-length fields are not needed after this point. The C front end does not do this. That means in the RISC-V backend, we are seeing two slightly different types, one with zero-length fields for C and one without for C++, which means we get different calling convention behavior for C and C++ code. Using extern "C" doesn't have any effect on this, as the struct still goes through the C++ front end.

The C++ front end code in question is the remove_zero_width_bit_fields function in gcc/cp/class.c. This was added in December 1999. Comments in the email suggest it is related to the new abi class layout support.
https://gcc.gnu.org/ml/gcc-patches/1999-12n/msg00589.html
https://gcc.gnu.org/ml/gcc-patches/1999-12n/msg00641.html
There was a name change along the way, so it is bitfields not bit_fields in this patch.

Different ABI support for C and C++ is a problem, and should be fixed, but unfortunately we have two bad choices here now. We can try to revert the 20 year old C++ front end change, maybe conditionally for RISC-V, which is likely to be hard and raise a lot of objections. Or we can fix the RISC-V backend to ignore zero-length fields, which means an ABI change for the C compiler, which is easy to implement but may be hard for others to handle, especially if something important like glibc is affected.

Something I can try doing is instrumenting gcc and building OpenEmbedded to see if the ABI change will actually trigger on real code. This could take a few days.

from riscv-elf-psabi-doc.

Closely related to this, given a testcase like this

struct s1 { int : 0; float f; long long int i; int : 0; };

void dummy(float, long long int);

void f(struct s1 s) {
  dummy(s.f + 1.0, s.i + 1);
}

the C and C++ compiler will pass them differently too. So this issue exists for both the case where we use two fp regs, and the case where we use one int reg and one fp reg.

from riscv-elf-psabi-doc.

I just noticed that my second testcase is identical to the first one. It was meant to be a testcase with two float fields instead of one float field and one int field.

I've filed an FSF GCC bug report for the ABI change
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91229

I haven't found any real code effected by the change as yet. One of the tests I did was an open-embedded "world" build, not everything builds because not everything has RISC-V support yet, but I did get over 30,000 tasks run before it failed, and no hits on a abort I added to check for code affected by the change.

from riscv-elf-psabi-doc.

I just committed the psABI fix for zero-length bitfields.

Do we still need clarification for non-zero-length bitfields? The docs say
A struct containing one floating-point real and one integer (or bitfield), in either order, is passed in a floating-point register and an integer register, without extending the integer to XLEN bits, provided the floating-point real is no more than FLEN bits wide and the integer is no more than XLEN bits wide, and at least one floating-point argument register and at least one integer argument register is available. Otherwise, it is passed according to the integer calling convention.

I think the issue here is that with "long long int i : 32;" compiling for rv32, the question is whether the length is 64 because of the type or the length is 32 because of the bit-field size. I would assume the length is 32 because it is a bit-field. Gcc appears to agree with this interpretation. I don't see how the length can be 64 because it is a bit-field, but we could add some extra text to try to clarify this.

from riscv-elf-psabi-doc.

Clang rejects bitfields that are wider than the type in C and truncates them for C++ (emitting a warning). I think the original point of this issue has been resolve (many thanks to @jim-wilson for resolving the issue on the GCC side). It's not clear if we need further clarification about larger-than-type bitfields in the psABI, but if we do it's probably a separate issue.

I'm happy to close this one - any views on the larger-than-type bitfields question?

from riscv-elf-psabi-doc.

@asb Apparently I missed this before, I am gonna close this and add a clarification for larger-than-type bitfields #332

from riscv-elf-psabi-doc.