Issues with CWriter::printGEPExpression

[bcoppens]

I have some issues with the else branch of if (!isConstantNull(FirstOp)) in CWriter::printGEPExpression.

1. As far as I understand, LLVM17 switched to no longer supporting non-opaque pointers, which (unfortunately) allows them to throw away all bitcasts, which has an impact on the generated code. Consider the following code (probably even simpler test cases exist, but this is a reduced case of what triggered it):

   
   #include <string>

struct A { std::string s; A() {} };

int main() { A a; return 6; }

While with LLVM14 (which needs g++ to link, but that's beside the point here :)) this results in code such as this:

```llvm
%struct.A = type { %"class.std::__cxx11::basic_string" }
%"class.std::__cxx11::basic_string" = type { %"struct.std::__cxx11::basic_string<char>::_Alloc_hider", i64, %union.anon }
%union.anon = type { i64, [8 x i8] }
; [...]
%1 = alloca %struct.A, align 8
%3 = getelementptr inbounds %struct.A, %struct.A* %1, i64 0, i32 0, i32 2
%4 = bitcast %struct.A* %1 to %union.anon**                                                   
store %union.anon* %3, %union.anon** %4, align 8, !tbaa !5

However, with clang++/LLVM17, we get something like:

; same types [...]
%a = alloca %struct.A, align 8
%0 = getelementptr inbounds %"class.std::__cxx11::basic_string", ptr %a, i64 0, i32 2
store ptr %0, ptr %a, align 8, !tbaa !5

So the code with clang++/LLVM17 immediately accesses %a as if it were a basic_string without bothering to cast the %struct.A %a to it. Which results in:

/tmp/pytest-of-bcoppens/pytest-265/test_consistent_return_value_c0/cbe.c: In function ‘main’:
/tmp/pytest-of-bcoppens/pytest-265/test_consistent_return_value_c0/cbe.c:63:32: error: ‘struct l_struct_struct_OC_A’ has no member named ‘field2’; did you mean ‘field0’?
   63 |   void* _1 = ((&(&llvm_cbe_a)->field2));
      |                                ^~~~~~
      |                                field0
/tmp/pytest-of-bcoppens/pytest-265/test_consistent_return_value_c0/cbe.c:66:34: error: ‘struct l_struct_struct_OC_A’ has no member named ‘field1’; did you mean ‘field0’?
   66 |   *(uint64_t*)(((&(&llvm_cbe_a)->field1))) = 1;
      |                                  ^~~~~~
      |                                  field0

2. Another issue, which I also trigger with clang/LLVM 14, is the following (again it probably can be simplified, but this is already significantly simplified from the original code :)):

struct A { int a; };
struct B { struct A a; char b[4]; void* c; char d[7]; void* e; void* f; void* g; char h; char i[256]; char j[256]; char k; char l[6]; };
struct B b = { .j[10] = 6 };

int main() {
  return b.j[10];
}

Which results in

           /tmp/pytest-of-bcoppens/pytest-314/test_consistent_return_value_c0/cbe.c: In function ‘main’:
           /tmp/pytest-of-bcoppens/pytest-314/test_consistent_return_value_c0/cbe.c:101:46: error: ‘struct l_unnamed_2’ has no member named ‘array’
             101 |   uint8_t _2 = *(uint8_t*)(((&(&(&b)->field9)->array[((int64_t)10)])));

Not sure what is going on there, struct l_unnamed_2 field9 is

struct l_unnamed_2 {
  struct l_array_11_uint8_t field0;
  struct l_array_245_uint8_t field1;
} __attribute__ ((packed));
struct l_array_11_uint8_t {
  uint8_t array[11];
};
struct l_array_245_uint8_t {
  uint8_t array[245];
};

Which indeed doesn't have an array (but its field0does). This comes from b's struct l_unnamed_1. However, struct l_struct_struct_OC_B, which is what the GEP really does use as a type, does have a correct field9 (struct l_array_256_uint8_t field9).

3. I initially had another (related) test case (from which I distilled the second one) where it didn't result in a compile error, but in CBE indexing not the array element (badly), but in getting the 10'th total array itself and indexing that. This resulted in a successful compilation, but where it accessed the memory at 10*256-(256-10) bytes too far. Unfortunately I somehow lost that test case and cannot reconstruct it. I assume it was at least heavily-related to the above issue.

Now, trying to debug this in CWriter::printGEPExpression, the else branch of if (!isConstantNull(FirstOp)) seems to be (based on the comments) only to print 'more idiomatic' C code.

If I just always take the true branch (and thus less idiomatic code is generated), all the above test cases seem to work fine. (The first and second issue because that branch just always first casts to the correct base type of the GEP itself, the third issue I don't quite know why at the moment, but then again I cannot really reproduce it unfortunately.)

Given that that entire piece of code is somewhat underdocumented, I was going to propose just dropping the printing of more idiomatic code. Unfortunately, doing so makes test_empty_array_geps and test_empty_array_geps_struct fail (with errors such as 'dereferencing ‘void *’ pointer' and 'error: request for member ‘field0’ in something not a structure or union', so clearly the else branch is not just for making it look prettier.

So before trying to look into it myself, I was wondering if anyone else would perhaps immediately know what could cause this?

[vtjnash]

I think printGEPExpression is just very naive about how type casts in C work, so it needs someone to teach it when "more idiomatic" code still needs casts and how to correctly print casts for "less idiomatic" code also

[hikari-no-yume]

Hi, I've touched this function so I'm probably partly responsible for this mess.

It was written for when LLVM didn't have opaque pointers, so it could assume that the base pointer had the right type. Now that LLVM has opaque pointers, it will need to emit a cast of the base pointer always.

Producing readable and correct C code is very hard within a single-pass translator, and I gave up on it in the end. To get good C code out of a C backend, it would need to build some kind of IR or AST, not print strings directly.