Hi Eli,
Thank you for your reviewing.
> Your patch is missing a testcase.
> Please submit patches based on trunk
I attach testcase file "recursive_struct.c". Before making patch file based on
trunk, I would like to explain the reason why I modified ConvertType not
FuctionType code. After reading the Clang code, I thought current Clang
generates struct type as type holder to cut the cycle of types on struct type
and function type. Type holder of struct type can be changed to correct struct
type later but one of function type can't be changed to correct function type
becase type holder is struct type. So I removed type holder generation on
function type and made only struct type cut the cycle of types. There is one
example to explain this situation.
struct foo1 {
struct foo2* a;
struct foo3* b;
};
struct foo2 {
void (*func1)(struct foo1*);
};
struct foo3 {
void (*func1)(struct foo1*);
};
struct foo2 e;
Cycle of types can be generated by struct type, function type and pointer type.
Type dependence graph of above code is as following.
---> struct foo2
| |
| V
| void (*func1)(struct foo1*) <----
1 | | |
| V |
| void (func1)(struct foo1 *) |
| | |
| V |
| struct foo1* |
| | | 2
| V |
| struct foo1 |
|____ _| |
| |
struct foo3* |
| |
struct foo |
|___________________|
Current Clang generates type holder at "struct foo2" to cut cycle 1 and at
"void (func1)(struct foo1 *)" to cut cycle 2. My patch is to use type holder of
struct type to remove cycle 2 not type holder of function type. In other words,
function type and pointer type return null when they have cycle and only struct
type generates type holder. For example, on above graph, "void (func1)(struct
foo1)" know that it has cycle so returns null and defers its type translation,
and "void (func1)(struct foo1)" also returns null and finally "struct foo"
generates type holder. To implement this concept on current Clang code, I
modifed ConvertType function. What do you think about this? I couldn't find
other solutions without modifying llvm vmcore to support type holder for
function type. If modifying of ConvertType function is not problem, I will make
patch base on trunk.
Thanks,
Jin-Gu Kang
________________________________________
From: Eli Friedman [[email protected]]
Sent: Thursday, November 08, 2012 8:55 AM
To: Jin Gu Kang
Cc: [email protected]
Subject: Re: [cfe-commits] [cfe-dev][patch] Strange LLVM IR result on recursive
type
On Wed, Nov 7, 2012 at 12:56 AM, Jin Gu Kang <[email protected]> wrote:
> Hi all,
>
> I found a strange result on clang-3.1 when recursive type was converted. A
> example is as following.
> C language source code:
> struct foo1 {
> struct foo2* a;
> struct foo3* b;
> };
> struct foo2 {
> void (*func1)(struct foo1*);
> };
> struct foo3 {
> void (*func1)(struct foo1*);
> };
> struct foo2 e;
>
> Converted result to llvm IR:
> %struct.foo2 = type { void (%struct.foo1*)* }
> %struct.foo1 = type { %struct.foo2*, %struct.foo3* }
> %struct.foo3 = type { {}* }
> @e = common global %struct.foo2 zeroinitializer, align 4
>
> struct.foo3 type has "{}*". The reason of strange result is that clang
> generates struct type for function type with parameter of cycled type as
> following.
> File: clang/lib/CodeGen/CodeGenTypes.cpp
> 463 // While we're converting the argument types for a function, we don't
> want
> 464 // to recursively convert any pointed-to structs. Converting
> directly-used
> 465 // structs is ok though.
> 466 if (!RecordsBeingLaidOut.insert(Ty)) {
> 467 ResultType = llvm::StructType::get(getLLVMContext());
> 468
> 469 SkippedLayout = true;
> 470 break;
> 471 }
>
> I think that cycle of types should be cut to translate cycled clang types
> into llvm IR so clang generates struct type as type holder. Is it right? What
> do you think about this?
>
> I made a simple patch for clang-3.1 to fix this problem. Resolved result is
> as following.
> Resolved result:
> %struct.foo2 = type { void (%struct.foo1*)* }
> %struct.foo1 = type { %struct.foo2*, %struct.foo3* }
> %struct.foo3 = type { void (%struct.foo1*)* }
> @e = common global %struct.foo2 zeroinitializer, align 4
>
> Please review this patch.
Your patch is missing a testcase.
Please submit patches based on trunk.
I don't think it's a good idea to make ConvertType return null. And I
think you should be handling an issue with FunctionTypes in the
FunctionType code.
-Elistruct foo1 {
struct foo2* a;
struct foo3* b;
};
struct foo2 {
void (*func1)(struct foo1*);
};
struct foo3 {
void (*func1)(struct foo1*);
};
struct foo2 e;
---> struct foo2
| |
| |
| V
| void (*func1)(struct foo1*) <----
| | |
1 | | |
| V |
| struct foo1* |
| | | 2
| | |
| V |
| struct foo1 |
| | |
|________| |
| |
| |
struct foo3* |
| |
| |
struct foo |
| |
|_________________________|
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
