> On Mar 10, 2023, at 2:54 AM, Richard Biener <rguent...@suse.de> wrote: > > On Thu, 9 Mar 2023, Qing Zhao wrote: > >> >> >>> On Mar 9, 2023, at 7:20 AM, Richard Biener <rguent...@suse.de> wrote: >>> >>> On Fri, 24 Feb 2023, Qing Zhao wrote: >>> >>>> GCC extension accepts the case when a struct with a C99 flexible array >>>> member >>>> is embedded into another struct or union (possibly recursively). >>>> __builtin_object_size should treat such struct as flexible size. >>>> >>>> gcc/c/ChangeLog: >>>> >>>> PR tree-optimization/101832 >>>> * c-decl.cc (finish_struct): Set TYPE_INCLUDE_FLEXARRAY for >>>> struct/union type. >>> >>> I can't really comment on the correctness of this part but since >>> only the C frontend will ever set this and you are using it from >>> addr_object_size which is also used for other C family languages >>> (at least), I wonder if you can really test >>> >>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v))) >>> >>> there. >> >> You mean for C++ and also other C family languages (other than C), the above >> bit cannot be set? >> Yes, that’s true. The bit is only set for C. So is the bit >> DECL_NOT_FLEXARRAY, which is only set for C too. >> So, I am wondering whether the bit DECL_NOT_FLEXARRAY should be also set in >> middle end? Or we can set DECL_NOT_FLEXARRAY in C++ FE too? And then we can >> set TYPE_INCLUDE_FLEXARRAY also in C++ FE? >> What’s your suggestion? >> >> (I personally feel that DECL_NOT_FLEXARRAY and TYPE_INCLUDE_FLEXARRAY should >> be set in the same places). > > I was wondering if the above test errors on the conservative side > correctly - it will now, for all but C, cut off some thing where it > didn't before?
As long as the default value of TYPE_INCLUDE_FLEXARRAY reflects the correct conservative behavior, then the testing should be correct, right? The default value of TYPE_INCLUDE_FLEXARRAY is 0, i.e, FALSE, means that the TYPE does not include a flex array by default, this is the correct behavior. Only when the TYPE does include a flexiarray, it will be set to TRUE. So, I think it’s correct. This is a different situation for DECL_NOT_FLEXARRAY, by default, the compiler will treat ALL trailing arrays as FMA, so in order to keep the correct conservative behavior, we should keep the default value for DECL_NOT_FLEXARRAY as it’s a FMA, i.e, DECL_NOT_FLEXARRAY being FALSE, by default. Only when the array is NOT a FMA, we set it to true. So, the default value for TYPE_INCLUDE_FLEXARRAY is correct. > >>> >>> Originally I was suggesting to set this flag in stor-layout.cc >>> which eventually all languages funnel their types through and >>> if there's language specific handling use a langhook (with the >>> default implementation preserving the status quo). >> >> If we decide to set the bits in stor-layout.cc, where is the best place to >> do it? I checked the star-layout.cc code, looks like “layout_type” might be >> the place where we can set these bits for RECORD_TYPE, UNION_TYPE? > > Yes, it would be layout_type. > >>> >>> Some more comments below ... >>> >>>> gcc/cp/ChangeLog: >>>> >>>> PR tree-optimization/101832 >>>> * module.cc (trees_out::core_bools): Stream out new bit >>>> type_include_flexarray. >>>> (trees_in::core_bools): Stream in new bit type_include_flexarray. >>>> >>>> gcc/ChangeLog: >>>> >>>> PR tree-optimization/101832 >>>> * print-tree.cc (print_node): Print new bit type_include_flexarray. >>>> * tree-core.h (struct tree_type_common): New bit >>>> type_include_flexarray. >>>> * tree-object-size.cc (addr_object_size): Handle structure/union type >>>> when it has flexible size. >>>> * tree-streamer-in.cc (unpack_ts_type_common_value_fields): Stream >>>> in new bit type_include_flexarray. >>>> * tree-streamer-out.cc (pack_ts_type_common_value_fields): Stream >>>> out new bit type_include_flexarray. >>>> * tree.h (TYPE_INCLUDE_FLEXARRAY): New macro >>>> TYPE_INCLUDE_FLEXARRAY. >>>> >>>> gcc/testsuite/ChangeLog: >>>> >>>> PR tree-optimization/101832 >>>> * gcc.dg/builtin-object-size-pr101832.c: New test. >>>> --- >>>> gcc/c/c-decl.cc | 12 ++ >>>> gcc/cp/module.cc | 2 + >>>> gcc/print-tree.cc | 5 + >>>> .../gcc.dg/builtin-object-size-pr101832.c | 134 ++++++++++++++++++ >>>> gcc/tree-core.h | 4 +- >>>> gcc/tree-object-size.cc | 79 +++++++---- >>>> gcc/tree-streamer-in.cc | 1 + >>>> gcc/tree-streamer-out.cc | 1 + >>>> gcc/tree.h | 6 + >>>> 9 files changed, 215 insertions(+), 29 deletions(-) >>>> create mode 100644 gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c >>>> >>>> diff --git a/gcc/c/c-decl.cc b/gcc/c/c-decl.cc >>>> index 08078eadeb8..f589a2f5192 100644 >>>> --- a/gcc/c/c-decl.cc >>>> +++ b/gcc/c/c-decl.cc >>>> @@ -9284,6 +9284,18 @@ finish_struct (location_t loc, tree t, tree >>>> fieldlist, tree attributes, >>>> /* Set DECL_NOT_FLEXARRAY flag for FIELD_DECL x. */ >>>> DECL_NOT_FLEXARRAY (x) = !is_flexible_array_member_p (is_last_field, >>>> x); >>>> >>>> + /* Set TYPE_INCLUDE_FLEXARRAY for the context of x, t >>>> + * when x is an array. */ >>>> + if (TREE_CODE (TREE_TYPE (x)) == ARRAY_TYPE) >>>> + TYPE_INCLUDE_FLEXARRAY (t) = flexible_array_member_type_p (TREE_TYPE >>>> (x)) ; >>>> + /* Recursively set TYPE_INCLUDE_FLEXARRAY for the context of x, t >>>> + when x is the last field. */ >>>> + else if ((TREE_CODE (TREE_TYPE (x)) == RECORD_TYPE >>>> + || TREE_CODE (TREE_TYPE (x)) == UNION_TYPE) >>>> + && TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (x)) >>>> + && is_last_field) >>>> + TYPE_INCLUDE_FLEXARRAY (t) = true; >>>> + >>>> if (DECL_NAME (x) >>>> || RECORD_OR_UNION_TYPE_P (TREE_TYPE (x))) >>>> saw_named_field = true; >>>> diff --git a/gcc/cp/module.cc b/gcc/cp/module.cc >>>> index ac2fe66b080..c750361b704 100644 >>>> --- a/gcc/cp/module.cc >>>> +++ b/gcc/cp/module.cc >>>> @@ -5371,6 +5371,7 @@ trees_out::core_bools (tree t) >>>> WB (t->type_common.lang_flag_5); >>>> WB (t->type_common.lang_flag_6); >>>> WB (t->type_common.typeless_storage); >>>> + WB (t->type_common.type_include_flexarray); >>>> } >>>> >>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON)) >>>> @@ -5551,6 +5552,7 @@ trees_in::core_bools (tree t) >>>> RB (t->type_common.lang_flag_5); >>>> RB (t->type_common.lang_flag_6); >>>> RB (t->type_common.typeless_storage); >>>> + RB (t->type_common.type_include_flexarray); >>>> } >>>> >>>> if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON)) >>>> diff --git a/gcc/print-tree.cc b/gcc/print-tree.cc >>>> index 1f3afcbbc86..efacdb7686f 100644 >>>> --- a/gcc/print-tree.cc >>>> +++ b/gcc/print-tree.cc >>>> @@ -631,6 +631,11 @@ print_node (FILE *file, const char *prefix, tree >>>> node, int indent, >>>> && TYPE_CXX_ODR_P (node)) >>>> fputs (" cxx-odr-p", file); >>>> >>>> + if ((code == RECORD_TYPE >>>> + || code == UNION_TYPE) >>>> + && TYPE_INCLUDE_FLEXARRAY (node)) >>>> + fputs (" include-flexarray", file); >>>> + >>>> /* The transparent-union flag is used for different things in >>>> different nodes. */ >>>> if ((code == UNION_TYPE || code == RECORD_TYPE) >>>> diff --git a/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c >>>> b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c >>>> new file mode 100644 >>>> index 00000000000..60078e11634 >>>> --- /dev/null >>>> +++ b/gcc/testsuite/gcc.dg/builtin-object-size-pr101832.c >>>> @@ -0,0 +1,134 @@ >>>> +/* PR 101832: >>>> + GCC extension accepts the case when a struct with a C99 flexible array >>>> + member is embedded into another struct (possibly recursively). >>>> + __builtin_object_size will treat such struct as flexible size. >>>> + However, when a structure with non-C99 flexible array member, i.e, >>>> trailing >>>> + [0], [1], or [4], is embedded into anther struct, the stucture will not >>>> + be treated as flexible size. */ >>>> +/* { dg-do run } */ >>>> +/* { dg-options "-O2" } */ >>>> + >>>> +#include "builtin-object-size-common.h" >>>> + >>>> +#define expect(p, _v) do { \ >>>> + size_t v = _v; \ >>>> + if (p == v) \ >>>> + __builtin_printf ("ok: %s == %zd\n", #p, p); \ >>>> + else {\ >>>> + __builtin_printf ("WAT: %s == %zd (expected %zd)\n", #p, p, v); \ >>>> + FAIL (); \ >>>> + } \ >>>> +} while (0); >>>> + >>>> + >>>> +struct A { >>>> + int n; >>>> + char data[]; >>>> +}; >>>> + >>>> +struct B { >>>> + int m; >>>> + struct A a; >>>> +}; >>>> + >>>> +struct C { >>>> + int q; >>>> + struct B b; >>>> +}; >>>> + >>>> +struct A0 { >>>> + int n; >>>> + char data[0]; >>>> +}; >>>> + >>>> +struct B0 { >>>> + int m; >>>> + struct A0 a; >>>> +}; >>>> + >>>> +struct C0 { >>>> + int q; >>>> + struct B0 b; >>>> +}; >>>> + >>>> +struct A1 { >>>> + int n; >>>> + char data[1]; >>>> +}; >>>> + >>>> +struct B1 { >>>> + int m; >>>> + struct A1 a; >>>> +}; >>>> + >>>> +struct C1 { >>>> + int q; >>>> + struct B1 b; >>>> +}; >>>> + >>>> +struct An { >>>> + int n; >>>> + char data[8]; >>>> +}; >>>> + >>>> +struct Bn { >>>> + int m; >>>> + struct An a; >>>> +}; >>>> + >>>> +struct Cn { >>>> + int q; >>>> + struct Bn b; >>>> +}; >>>> + >>>> +volatile void *magic1, *magic2; >>>> + >>>> +int main (int argc, char *argv[]) >>>> +{ >>>> + struct B *outer; >>>> + struct C *outest; >>>> + >>>> + /* Make sure optimization can't find some other object size. */ >>>> + outer = (void *)magic1; >>>> + outest = (void *)magic2; >>>> + >>>> + expect (__builtin_object_size (&outer->a, 1), -1); >>>> + expect (__builtin_object_size (&outest->b, 1), -1); >>>> + expect (__builtin_object_size (&outest->b.a, 1), -1); >>>> + >>>> + struct B0 *outer0; >>>> + struct C0 *outest0; >>>> + >>>> + /* Make sure optimization can't find some other object size. */ >>>> + outer0 = (void *)magic1; >>>> + outest0 = (void *)magic2; >>>> + >>>> + expect (__builtin_object_size (&outer0->a, 1), sizeof (outer0->a)); >>>> + expect (__builtin_object_size (&outest0->b, 1), sizeof (outest0->b)); >>>> + expect (__builtin_object_size (&outest0->b.a, 1), sizeof >>>> (outest0->b.a)); >>>> + >>>> + struct B1 *outer1; >>>> + struct C1 *outest1; >>>> + >>>> + /* Make sure optimization can't find some other object size. */ >>>> + outer1 = (void *)magic1; >>>> + outest1 = (void *)magic2; >>>> + >>>> + expect (__builtin_object_size (&outer1->a, 1), sizeof (outer1->a)); >>>> + expect (__builtin_object_size (&outest1->b, 1), sizeof (outest1->b)); >>>> + expect (__builtin_object_size (&outest1->b.a, 1), sizeof >>>> (outest1->b.a)); >>>> + >>>> + struct Bn *outern; >>>> + struct Cn *outestn; >>>> + >>>> + /* Make sure optimization can't find some other object size. */ >>>> + outern = (void *)magic1; >>>> + outestn = (void *)magic2; >>>> + >>>> + expect (__builtin_object_size (&outern->a, 1), sizeof (outern->a)); >>>> + expect (__builtin_object_size (&outestn->b, 1), sizeof (outestn->b)); >>>> + expect (__builtin_object_size (&outestn->b.a, 1), sizeof >>>> (outestn->b.a)); >>>> + >>>> + DONE (); >>>> + return 0; >>>> +} >>>> diff --git a/gcc/tree-core.h b/gcc/tree-core.h >>>> index acd8deea34e..705d5702b9c 100644 >>>> --- a/gcc/tree-core.h >>>> +++ b/gcc/tree-core.h >>>> @@ -1718,7 +1718,9 @@ struct GTY(()) tree_type_common { >>>> unsigned empty_flag : 1; >>>> unsigned indivisible_p : 1; >>>> RECORD_OR_UNION_CHECK >>> >>> it looks like the bit could be eventually shared with >>> no_named_args_stdarg_p (but its accessor doesn't use >>> FUNC_OR_METHOD_CHECK) >> You mean to let “type_include_flexarray” share the same bit with >> “no_named_args_stdarg_p” in order to save 1-bit space in IR? >> Then change the >> >> /* True if this is a stdarg function with no named arguments (C2x >> (...) prototype, where arguments can be accessed with va_start and >> va_arg), as opposed to an unprototyped function. */ >> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \ >> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p) >> >> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member >> at the last field recursively. */ >> #define TYPE_INCLUDE_FLEXARRAY(NODE) \ >> (TYPE_CHECK (NODE)->type_common.type_include_flexarray) >> >> To: >> >> union { >> unsigned no_named_args_stdarg_p : 1; >> /* TYPE_INCLUDE_FLEXARRAY flag for RECORD_TYPE and UNION_TYPE. */ >> unsigned int type_include_flexarray : 1; >> } shared_bit; >> unsigned spare : 15; >> >> /* True if this is a stdarg function with no named arguments (C2x >> (...) prototype, where arguments can be accessed with va_start and >> va_arg), as opposed to an unprototyped function. */ >> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \ >> (FUNC_OR_METHOD_CHECK (NODE)->type_common.shared_bit.no_named_args_stdarg_p) >> >> /* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member >> at the last field recursively. */ >> #define TYPE_INCLUDE_FLEXARRAY(NODE) \ >> (RECORD_OR_UNION_CHECK(NODE)->type_common.shared_bit.type_include_flexarray) > > No, we're just overloading bits by using the same name for different > purposes. I don't think such a union would pack nicely. We overload > by documenting the uses, in the above cases the uses are separated > by the kind of the tree node, RECORD/UNION_TYPE vs. FUNCTION/METHOD_TYPE > and the accessor macros should be updated to use the appropriate > tree check macros covering that so we don't try to inspect the > "wrong bit”. Okay, I see. Then should I change the name of that bit to one that reflect two usages? > >>> >>>> alias_set_type alias_set; >>>> tree pointer_to; >>>> diff --git a/gcc/tree-object-size.cc b/gcc/tree-object-size.cc >>>> index 9a936a91983..22b3c72ea6e 100644 >>>> --- a/gcc/tree-object-size.cc >>>> +++ b/gcc/tree-object-size.cc >>>> @@ -633,45 +633,68 @@ addr_object_size (struct object_size_info *osi, >>>> const_tree ptr, >>>> v = NULL_TREE; >>>> break; >>>> case COMPONENT_REF: >>>> - if (TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE) >>>> + /* When the ref is not to an array, a record or a union, it >>>> + will not have flexible size, compute the object size >>>> + directly. */ >>>> + if ((TREE_CODE (TREE_TYPE (v)) != ARRAY_TYPE) >>>> + && (TREE_CODE (TREE_TYPE (v)) != RECORD_TYPE) >>>> + && (TREE_CODE (TREE_TYPE (v)) != UNION_TYPE)) >>>> { >>>> v = NULL_TREE; >>>> break; >>>> } >>>> - is_flexible_array_mem_ref = array_ref_flexible_size_p (v); >>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF) >>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> - != UNION_TYPE >>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> - != QUAL_UNION_TYPE) >>>> - break; >>>> - else >>>> - v = TREE_OPERAND (v, 0); >>>> - if (TREE_CODE (v) == COMPONENT_REF >>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> - == RECORD_TYPE) >>>> + if (TREE_CODE (TREE_TYPE (v)) == RECORD_TYPE >>>> + || TREE_CODE (TREE_TYPE (v)) == UNION_TYPE) >>>> + /* if the record or union does not include a flexible array >>>> + recursively, compute the object size directly. */ >>>> { >>>> - /* compute object size only if v is not a >>>> - flexible array member. */ >>>> - if (!is_flexible_array_mem_ref) >>>> + if (!TYPE_INCLUDE_FLEXARRAY (TREE_TYPE (v))) >>>> { >>>> v = NULL_TREE; >>>> break; >>>> } >>>> - v = TREE_OPERAND (v, 0); >>>> + else >>>> + v = TREE_OPERAND (v, 0); >>>> } >>>> - while (v != pt_var && TREE_CODE (v) == COMPONENT_REF) >>>> - if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> - != UNION_TYPE >>>> - && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> - != QUAL_UNION_TYPE) >>>> - break; >>>> - else >>>> - v = TREE_OPERAND (v, 0); >>>> - if (v != pt_var) >>>> - v = NULL_TREE; >>>> else >>>> - v = pt_var; >>>> + { >>>> + /* Now the ref is to an array type. */ >>>> + is_flexible_array_mem_ref >>>> + = array_ref_flexible_size_p (v); >>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF) >>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> + != UNION_TYPE >>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> + != QUAL_UNION_TYPE) >>>> + break; >>>> + else >>>> + v = TREE_OPERAND (v, 0); >>>> + if (TREE_CODE (v) == COMPONENT_REF >>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> + == RECORD_TYPE) >>>> + { >>>> + /* compute object size only if v is not a >>>> + flexible array member. */ >>>> + if (!is_flexible_array_mem_ref) >>>> + { >>>> + v = NULL_TREE; >>>> + break; >>>> + } >>>> + v = TREE_OPERAND (v, 0); >>>> + } >>>> + while (v != pt_var && TREE_CODE (v) == COMPONENT_REF) >>>> + if (TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> + != UNION_TYPE >>>> + && TREE_CODE (TREE_TYPE (TREE_OPERAND (v, 0))) >>>> + != QUAL_UNION_TYPE) >>>> + break; >>>> + else >>>> + v = TREE_OPERAND (v, 0); >>>> + if (v != pt_var) >>>> + v = NULL_TREE; >>>> + else >>>> + v = pt_var; >>>> + } >>>> break; >>>> default: >>>> v = pt_var; >>>> diff --git a/gcc/tree-streamer-in.cc b/gcc/tree-streamer-in.cc >>>> index d4dc30f048f..c19ede0631d 100644 >>>> --- a/gcc/tree-streamer-in.cc >>>> +++ b/gcc/tree-streamer-in.cc >>>> @@ -390,6 +390,7 @@ unpack_ts_type_common_value_fields (struct bitpack_d >>>> *bp, tree expr) >>>> TYPE_TRANSPARENT_AGGR (expr) = (unsigned) bp_unpack_value (bp, 1); >>>> TYPE_FINAL_P (expr) = (unsigned) bp_unpack_value (bp, 1); >>>> TYPE_CXX_ODR_P (expr) = (unsigned) bp_unpack_value (bp, 1); >>>> + TYPE_INCLUDE_FLEXARRAY (expr) = (unsigned) bp_unpack_value (bp, 1); >>>> } >>>> else if (TREE_CODE (expr) == ARRAY_TYPE) >>>> TYPE_NONALIASED_COMPONENT (expr) = (unsigned) bp_unpack_value (bp, 1); >>>> diff --git a/gcc/tree-streamer-out.cc b/gcc/tree-streamer-out.cc >>>> index d107229da5c..73e4b4e547c 100644 >>>> --- a/gcc/tree-streamer-out.cc >>>> +++ b/gcc/tree-streamer-out.cc >>>> @@ -357,6 +357,7 @@ pack_ts_type_common_value_fields (struct bitpack_d >>>> *bp, tree expr) >>>> bp_pack_value (bp, flag_wpa && TYPE_CANONICAL (expr) >>>> ? TYPE_CXX_ODR_P (TYPE_CANONICAL (expr)) >>>> : TYPE_CXX_ODR_P (expr), 1); >>>> + bp_pack_value (bp, TYPE_INCLUDE_FLEXARRAY (expr), 1); >>>> } >>>> else if (TREE_CODE (expr) == ARRAY_TYPE) >>>> bp_pack_value (bp, TYPE_NONALIASED_COMPONENT (expr), 1); >>>> diff --git a/gcc/tree.h b/gcc/tree.h >>>> index 92ac0e6a214..ab1cdc3dc85 100644 >>>> --- a/gcc/tree.h >>>> +++ b/gcc/tree.h >>>> @@ -778,6 +778,12 @@ extern void omp_clause_range_check_failed >>>> (const_tree, const char *, int, >>>> #define TYPE_NO_NAMED_ARGS_STDARG_P(NODE) \ >>>> (TYPE_CHECK (NODE)->type_common.no_named_args_stdarg_p) >>>> >>>> +/* True if this RECORD_TYPE or UNION_TYPE includes a flexible array member >>>> + at the last field recursively. */ >>>> +#define TYPE_INCLUDE_FLEXARRAY(NODE) \ >>>> + (TYPE_CHECK (NODE)->type_common.type_include_flexarray) >>> >>> Please use RECORD_OR_UNION_CHECK. >> Okay. >>> The comment "at the last field" >>> doesn't seem to match implementation? (at least not obviously) >> The current implementation (in gcc/c/c-decl.cc) guarantees that it’s at the >> last field. (And recursively). >>> Given this is a generic header expanding on what a "flexible array member" >>> is to the middle-end here would be good. Especially the guarantees >>> made when the bit is _not_ set (we've evaded that with DECL_NOT_FLEXARRAY >>> vs. DECL_FLEXARRAY). >> >> The bit TYPE_INCLUDE_FLEXARRAY default is FALSE, i.e, not including a flex >> array by default. >> It’s only set to TRUE when the struct/union includes a flex array at the >> last field (and recursively). So, I think it should be correct. >> Let me know if I missed anything here. > > See above - I was looking at the use (but I'm not familiar with that > code), and wondered how the change affects uses from other frontends. Please see my explanation above, I think the default behavior from other FE should be kept correctly with this patch. thanks. Qing > > Richard.