| Summary: | Any Recursive test fails | ||
|---|---|---|---|
| Product: | TAO | Reporter: | Johnny Willemsen <jwillemsen> |
| Component: | ORB | Assignee: | Johnny Willemsen <jwillemsen> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | CC: | ossama.othman |
| Priority: | P3 | ||
| Version: | 1.4.10 | ||
| Hardware: | All | ||
| OS: | All | ||
| Attachments: | plain patch until now | ||
|
Description
Johnny Willemsen
2005-12-05 14:11:00 CST
-------- Original Message -------- Subject: Any recursive tests Date: Tue, 6 Dec 2005 17:40:12 -0600 From: Jeff Parsons <j.parsons@vanderbilt.edu> To: Ossama Othman <oothman@isis.vanderbilt.edu>, buildczar@remedy.nl CC: Douglas Schmidt <schmidt@isis.vanderbilt.edu> Hi Ossama, This test has been failing, so I decided to look into it in case it was a problem with Anys. It doesn't appear to be ;-). It seems the recursive union test is throwing a CORBA::MARSHAL exception on the server side. It happens during demarshaling of the recursive union typecode. I've tracked it down as far as tc_demarshal_indirection(), specifically the line if (!(indir_stream >> kind) The extraction succeeds, but 'kind' is a garbage value, so the subsequent check for a match with tk_struct, tk_union, tk_valuetype or tk_event fails. Looks like an alignment problem. No fun, but maybe I've at least saved you some time by taking it this far. thanks, Jeff reassinging this to Ossama, it is already on his todo list Mine. updated version Did some testing. It looks like it goes in the code part of the diff below, the
diff is not correct, but we are marshaling the cases of the union. In the code
in cvs the output cdr end is used but this already contains data, so the call
enc.total_length() gives a high number back (let say 76). The call to c.marshal
ends up in TAO::TypeCode::Case<StringType, TypeCodeType>::marshal where also
cdr.total_length is added to the offset, this is then let say 100, so we are
already then at offset 176). At the end after marshaling the union the offset
stored is 260, but the full message send is 336 bytes, meaning this offset is
not correct. Shouldn't for the marshaling of the union cases a new cdr be used
so that the offset doesn't get increment too much? The patch below doesn't work
but at least I get a more realisted offset.
Index: Union_TypeCode_Static.cpp
===================================================================
RCS file:
/project/cvs-repository/ACE_wrappers-repository/TAO/tao/AnyTypeCode/Union_TypeCode_Static.cpp,v
retrieving revision 1.4
diff -u -u -w -b -r1.4 Union_TypeCode_Static.cpp
--- Union_TypeCode_Static.cpp 3 Nov 2005 17:38:44 -0000 1.4
+++ Union_TypeCode_Static.cpp 2 Mar 2006 13:21:38 -0000
@@ -59,13 +59,19 @@
return false;
// Note that we handle the default case below, too.
+ offset += enc.total_length ();
for (unsigned int i = 0; i < this->ncases_; ++i)
{
+ TAO_OutputCDR default_case_enc;
+ offset = ACE_align_binary (offset,
+ ACE_CDR::LONG_ALIGN);
case_type const & c = *this->cases_[i];
- if (!c.marshal (enc, offset + enc.total_length ()))
+ if (!c.marshal (default_case_enc, offset))
return false;
+
+ cdr.write_octet_array_mb (enc.begin ());
}
return
the code from the marshal method is below, see here that offset is also
increment with cdr.total_length().
template <typename StringType, typename TypeCodeType>
ACE_INLINE bool
TAO::TypeCode::Case<StringType, TypeCodeType>::marshal (
TAO_OutputCDR & cdr,
CORBA::ULong offset) const
{
return
this->marshal_label (cdr)
&& (cdr << TAO_OutputCDR::from_string (
Traits<StringType>::get_string (this->name_), 0))
&& TAO::TypeCode::marshal (cdr,
Traits<StringType>::get_typecode (this->type_),
offset + cdr.total_length ());
}
with the diff below the recursive union gets from client to server and then also
a reply is send back but then I get a marshal exception on the client, seems we
have another issue.
cvs -z9 -w -q diff -u -wb -- Union_TypeCode_Static.cpp (in directory
C:\ACE\latest\ACE_wrappers\TAO\tao\AnyTypeCode\)
Index: Union_TypeCode_Static.cpp
===================================================================
RCS file:
/project/cvs-repository/ACE_wrappers-repository/TAO/tao/AnyTypeCode/Union_TypeCode_Static.cpp,v
retrieving revision 1.4
diff -u -u -w -b -r1.4 Union_TypeCode_Static.cpp
--- Union_TypeCode_Static.cpp 3 Nov 2005 17:38:44 -0000 1.4
+++ Union_TypeCode_Static.cpp 2 Mar 2006 13:55:26 -0000
@@ -59,13 +59,19 @@
return false;
// Note that we handle the default case below, too.
+ offset += enc.total_length ();
for (unsigned int i = 0; i < this->ncases_; ++i)
{
+ TAO_OutputCDR default_case_enc;
+ offset = ACE_align_binary (offset,
+ ACE_CDR::LONG_ALIGN);
case_type const & c = *this->cases_[i];
- if (!c.marshal (enc, offset + enc.total_length ()))
+ if (!c.marshal (default_case_enc, offset))
return false;
+
+ enc.write_octet_array_mb (default_case_enc.begin ());
}
another change related to this, same concept as the previous patch, just another
part of the code, this was called for the reply. With this also applied the any
recursive test now succeeds.
cvs -z9 -w -q diff -u -wb -- Union_TypeCode.cpp (in directory
C:\ACE\latest\ACE_wrappers\TAO\tao\AnyTypeCode\)
Index: Union_TypeCode.cpp
===================================================================
RCS file:
/project/cvs-repository/ACE_wrappers-repository/TAO/tao/AnyTypeCode/Union_TypeCode.cpp,v
retrieving revision 1.5
diff -u -u -w -b -r1.5 Union_TypeCode.cpp
--- Union_TypeCode.cpp 3 Nov 2005 17:38:44 -0000 1.5
+++ Union_TypeCode.cpp 2 Mar 2006 14:27:48 -0000
@@ -58,12 +58,20 @@
if (!success)
return false;
+ offset += enc.total_length ();
+
for (CORBA::ULong i = 0; i < this->ncases_; ++i)
{
+ TAO_OutputCDR default_case_enc;
+ offset = ACE_align_binary (offset,
+ ACE_CDR::LONG_ALIGN);
+
case_type const & c = *this->cases_[i];
- if (!c.marshal (enc, offset + enc.total_length ()))
+ if (!c.marshal (default_case_enc, offset))
return false;
+ enc.write_octet_array_mb (default_case_enc.begin ());
+
}
return
Ossama had some remarks on the patch, based on that the test is extended to the
following recursive union, this again triggers now the marshal error,
reinvestigating this more and taking over this bug from Ossama
union RecursiveUnion switch (short)
{
case 0:
RecursiveUnionSeq recursive_unions;
case 1:
short a;
case 2:
RecursiveUnionSeq recursive_unions_second;
default:
long i;
};
problem with extended test seems to be in demarshaling. when we are marshaling
again in the method below for the 2nd recursive union member
TAO::TypeCode::Union<StringType,
TypeCodeType,
CaseArrayType,
RefCountPolicy>::tao_marshal (
this code fails:
bool const success =
(enc << TAO_OutputCDR::from_boolean (TAO_ENCAP_BYTE_ORDER))
&& (enc << TAO_OutputCDR::from_string (this->base_attributes_.id (), 0))
&& (enc << TAO_OutputCDR::from_string (this->base_attributes_.name (), 0))
&& marshal (enc,
Traits<StringType>::get_typecode (this->discriminant_type_),
offset + enc.total_length ())
&& (enc << this->default_index_)
&& (enc << this->ncases_);
this because name() is null. This is not filled during demarshaling in the typecode
problem is in :
bool
TAO::TypeCodeFactory::tc_union_factory (CORBA::TCKind /* kind */,
TAO_InputCDR & cdr,
CORBA::TypeCode_ptr & tc,
TC_Info_List & infos)
here the following code only expects one case that is recursive, propose to
change find_recursive-tc to return a possible array
if (find_recursive_tc (id.in (), tc, infos))
{
// Set remaining parameters.
typedef TAO::TypeCode::Recursive_Type<typecode_type,
CORBA::TypeCode_var,
case_array_type>
recursive_typecode_type;
recursive_typecode_type * const rtc =
dynamic_cast<recursive_typecode_type *> (tc);
ACE_ASSERT (rtc);
rtc->union_parameters (name.in (),
discriminant_type,
cases, // Will be copied.
ncases,
default_index);
}
more patches, changed the code that multiple types could have multiple recursive
members, now the union gets from client to server but after the reply now it
fails on the client again. byte dump of the server
Index: TypeCode_CDR_Extraction.cpp
===================================================================
RCS file:
/project/cvs-repository/ACE_wrappers-repository/TAO/tao/AnyTypeCode/TypeCode_CDR_Extraction.cpp,v
retrieving revision 1.6
diff -u -u -w -b -r1.6 TypeCode_CDR_Extraction.cpp
--- TypeCode_CDR_Extraction.cpp 12 Jan 2006 00:45:13 -0000 1.6
+++ TypeCode_CDR_Extraction.cpp 3 Mar 2006 11:34:57 -0000
@@ -105,7 +105,7 @@
TAO::TypeCodeFactory::TC_Info_List & infos);
bool find_recursive_tc (char const * id,
- CORBA::TypeCode_ptr & tc,
+ TAO::TypeCodeFactory::TC_Info_List & tcs,
TAO::TypeCodeFactory::TC_Info_List & infos)
{
// See comments above for rationale behind using an array instead
@@ -119,12 +119,18 @@
if (ACE_OS::strcmp (info.id, id) == 0)
{
- tc = info.type;
- return true;
+ // We have a mathing id, so store the typecode in the out array
+ // and then compare the others.
+ size_t const old_size = tcs.size ();
+ if (tcs.size (old_size + 1) == -1) // Incremental growth -- *sigh*
+ return false;
+
+ TAO::TypeCodeFactory::TC_Info & new_info = tcs[old_size];
+ new_info.type = info.type;
}
}
- return false;
+ return (tcs.size () > 0) ;
}
}
@@ -409,7 +415,8 @@
// Check if struct TypeCode is recursive.
- if (kind == CORBA::tk_struct && find_recursive_tc (id.in (), tc, infos))
+ TAO::TypeCodeFactory::TC_Info_List recursive_tc;
+ if (kind == CORBA::tk_struct && find_recursive_tc (id.in (), recursive_tc,
infos))
{
// Set remaining parameters.
@@ -418,8 +425,14 @@
member_array_type>
recursive_typecode_type;
+ size_t const len = recursive_tc.size ();
+
+ for (size_t i = 0; i < len; ++i)
+ {
+ TAO::TypeCodeFactory::TC_Info & info = recursive_tc[i];
+
recursive_typecode_type * const rtc =
- dynamic_cast<recursive_typecode_type *> (tc);
+ dynamic_cast<recursive_typecode_type *> (info.type);
ACE_ASSERT (rtc);
@@ -427,6 +440,9 @@
fields,
nfields);
}
+
+ tc = recursive_tc[0].type;
+ }
else
{
ACE_NEW_RETURN (tc,
@@ -496,7 +512,7 @@
{
elem_type & member = cases[i];
- TAO::TypeCode::Case<CORBA::String_var, CORBA::TypeCode_var> * the_case;
+ TAO::TypeCode::Case<CORBA::String_var, CORBA::TypeCode_var> * the_case = 0;
// Ugly. *sigh*
switch (discriminant_kind)
@@ -647,8 +663,9 @@
case_array_type,
TAO::True_RefCount_Policy> typecode_type;
- // Check if union TypeCode is recursive.
- if (find_recursive_tc (id.in (), tc, infos))
+ // Check if we have recursive members, this could be multiple
+ TAO::TypeCodeFactory::TC_Info_List recursive_tc;
+ if (find_recursive_tc (id.in (), recursive_tc, infos))
{
// Set remaining parameters.
@@ -657,8 +674,14 @@
case_array_type>
recursive_typecode_type;
+ size_t const len = recursive_tc.size ();
+
+ for (size_t i = 0; i < len; ++i)
+ {
+ TAO::TypeCodeFactory::TC_Info & info = recursive_tc[i];
+
recursive_typecode_type * const rtc =
- dynamic_cast<recursive_typecode_type *> (tc);
+ dynamic_cast<recursive_typecode_type *> (info.type);
ACE_ASSERT (rtc);
@@ -668,6 +691,9 @@
ncases,
default_index);
}
+
+ tc = recursive_tc[0].type;
+ }
else
{
ACE_NEW_RETURN (tc,
@@ -993,7 +1019,8 @@
TAO::True_RefCount_Policy> typecode_type;
// Check if valuetype/eventtype TypeCode is recursive.
- if (find_recursive_tc (id.in (), tc, infos))
+ TAO::TypeCodeFactory::TC_Info_List recursive_tc;
+ if (find_recursive_tc (id.in (), recursive_tc, infos))
{
// Set remaining parameters.
@@ -1002,8 +1029,14 @@
member_array_type>
recursive_typecode_type;
+ size_t const len = recursive_tc.size ();
+
+ for (size_t i = 0; i < len; ++i)
+ {
+ TAO::TypeCodeFactory::TC_Info & info = recursive_tc[i];
+
recursive_typecode_type * const rtc =
- dynamic_cast<recursive_typecode_type *> (tc);
+ dynamic_cast<recursive_typecode_type *> (info.type);
ACE_ASSERT (rtc);
@@ -1013,6 +1046,8 @@
fields, // Will be copied.
nfields);
}
+ tc = recursive_tc[0].type;
+ }
else
{
ACE_NEW_RETURN (tc,
below is the hexdump of the reply, this is really too large, seems there is duplicate info in it. TAO (5128|4264) - Codeset_Manager_i::process_service_context, using tcsc = 00010 001, tcsw = 00010109 TAO (5128|4264) - GIOP_Message_Base::dump_msg, send GIOP v1.2 msg, 732 data byte s, my endian, Type Reply[1] GIOP message - HEXDUMP 744 bytes 47 49 4f 50 01 02 01 01 dc 02 00 00 01 00 00 00 GIOP............ 00 00 00 00 00 00 00 00 10 00 00 00 c0 02 00 00 ................ 01 67 68 32 1c 00 00 00 49 44 4c 3a 54 65 73 74 .gh2....IDL:Test 2f 52 65 63 75 72 73 69 76 65 55 6e 69 6f 6e 3a /RecursiveUnion: 31 2e 30 00 0f 00 00 00 52 65 63 75 72 73 69 76 1.0.....Recursiv 65 55 6e 69 6f 6e 00 00 02 00 00 00 03 00 00 00 eUnion.......... 04 00 00 00 00 00 68 32 11 00 00 00 72 65 63 75 ......h2....recu 72 73 69 76 65 5f 75 6e 69 6f 6e 73 00 00 00 00 rsive_unions.... 15 00 00 00 58 00 00 00 01 67 68 32 1f 00 00 00 ....X....gh2.... 49 44 4c 3a 54 65 73 74 2f 52 65 63 75 72 73 69 IDL:Test/Recursi 76 65 55 6e 69 6f 6e 53 65 71 3a 31 2e 30 00 00 veUnionSeq:1.0.. 12 00 00 00 52 65 63 75 72 73 69 76 65 55 6e 69 ....RecursiveUni 6f 6e 53 65 71 00 00 00 13 00 00 00 10 00 00 00 onSeq........... 01 67 68 32 ff ff ff ff 40 ff ff ff 00 00 00 00 .gh2....@....... 01 00 68 32 02 00 00 00 61 00 00 00 02 00 00 00 ..h2....a....... 02 00 68 32 18 00 00 00 72 65 63 75 72 73 69 76 ..h2....recursiv 65 5f 75 6e 69 6f 6e 73 5f 73 65 63 6f 6e 64 00 e_unions_second. 15 00 00 00 b8 01 00 00 01 67 68 32 1f 00 00 00 .........gh2.... 49 44 4c 3a 54 65 73 74 2f 52 65 63 75 72 73 69 IDL:Test/Recursi 76 65 55 6e 69 6f 6e 53 65 71 3a 31 2e 30 00 00 veUnionSeq:1.0.. 12 00 00 00 52 65 63 75 72 73 69 76 65 55 6e 69 ....RecursiveUni 6f 6e 53 65 71 00 00 00 13 00 00 00 70 01 00 00 onSeq.......p... 01 67 68 32 10 00 00 00 60 01 00 00 01 67 68 32 .gh2....`....gh2 1c 00 00 00 49 44 4c 3a 54 65 73 74 2f 52 65 63 ....IDL:Test/Rec 75 72 73 69 76 65 55 6e 69 6f 6e 3a 31 2e 30 00 ursiveUnion:1.0. 0f 00 00 00 52 65 63 75 72 73 69 76 65 55 6e 69 ....RecursiveUni 6f 6e 00 00 02 00 00 00 03 00 00 00 04 00 00 00 on.............. 00 00 68 32 11 00 00 00 72 65 63 75 72 73 69 76 ..h2....recursiv 65 5f 75 6e 69 6f 6e 73 00 00 00 00 15 00 00 00 e_unions........ 58 00 00 00 01 67 68 32 1f 00 00 00 49 44 4c 3a X....gh2....IDL: 54 65 73 74 2f 52 65 63 75 72 73 69 76 65 55 6e Test/RecursiveUn 69 6f 6e 53 65 71 3a 31 2e 30 00 00 12 00 00 00 ionSeq:1.0...... 52 65 63 75 72 73 69 76 65 55 6e 69 6f 6e 53 65 RecursiveUnionSe 71 00 00 00 13 00 00 00 10 00 00 00 01 67 68 32 q............gh2 ff ff ff ff 40 ff ff ff 00 00 00 00 01 00 68 32 ....@.........h2 02 00 00 00 61 00 00 00 02 00 00 00 02 00 68 32 ....a.........h2 18 00 00 00 72 65 63 75 72 73 69 76 65 5f 75 6e ....recursive_un 69 6f 6e 73 5f 73 65 63 6f 6e 64 00 15 00 00 00 ions_second..... 58 00 00 00 01 67 68 32 1f 00 00 00 49 44 4c 3a X....gh2....IDL: 54 65 73 74 2f 52 65 63 75 72 73 69 76 65 55 6e Test/RecursiveUn 69 6f 6e 53 65 71 3a 31 2e 30 00 00 12 00 00 00 ionSeq:1.0...... 52 65 63 75 72 73 69 76 65 55 6e 69 6f 6e 53 65 RecursiveUnionSe 71 00 00 00 13 00 00 00 10 00 00 00 01 67 68 32 q............gh2 ff ff ff ff b0 fe ff ff 00 00 00 00 00 80 68 32 ..............h2 02 00 00 00 69 00 00 00 03 00 00 00 00 00 00 00 ....i........... 00 80 68 32 02 00 00 00 69 00 00 00 03 00 00 00 ..h2....i....... 00 80 00 00 35 a5 03 00 ....5... There seems another but in the code below. For the second recursive union member
the boolean in recursion is false, meaning the typecode is again put in the
stream, when I change things in the debugger to also for the second recursive
member to have the value true things work fine and the return message is also a
lot smaller.
template <class TypeCodeBase, typename TypeCodeType, typename MemberArrayType>
bool
TAO::TypeCode::Recursive_Type<TypeCodeBase,
TypeCodeType,
MemberArrayType>::tao_marshal_kind (
TAO_OutputCDR & cdr) const
{
ACE_GUARD_RETURN (TAO_SYNCH_RECURSIVE_MUTEX,
guard,
this->lock_,
false);
// Top-level TypeCode case.
if (!(this->in_recursion_))
return this->ACE_NESTED_CLASS (CORBA, TypeCode)::tao_marshal_kind (cdr);
// Recursive/indirected TypeCode case.
CORBA::ULong const indirection_kind = 0xffffffff;
return (cdr << indirection_kind);
}
The flag is set in:
template <class TypeCodeBase, typename TypeCodeType, typename MemberArrayType>
bool
TAO::TypeCode::Recursive_Type<TypeCodeBase,
TypeCodeType,
MemberArrayType>::tao_marshal (
TAO_OutputCDR & cdr,
CORBA::ULong offset) const
{
Seems the usage of bool for in recursion is not correct, looking at first sight
we should make it a long that we increment/decrement. When bigger then 0 we are
in recursiion
the conversion from bool to unsigned long is not working. I think in the method
below there should be a check if there is already a tc for the given id. what I
see in the debugger that there are in fact 2 typecodes for the same structure.
bool
tc_demarshal_indirection (TAO_InputCDR & cdr,
CORBA::TypeCode_ptr & tc,
TAO::TypeCodeFactory::TC_Info_List & infos)
recursive union is now working, problem was now in
bool
tc_demarshal_indirection (TAO_InputCDR & cdr,
CORBA::TypeCode_ptr & tc,
TAO::TypeCodeFactory::TC_Info_List & infos)
the problem was that each time a new typecode was created, but we should inspect
infos first to see if we have one already, if yes, use that, else create a new one.
another bug, change in test.idl the recursive struct to:
struct RecursiveStruct
{
RecursiveStructSeq recursive_structs;
RecursiveStructSeq recursive_structs_second;
long i;
};
and it fails :-)
Created attachment 480 [details]
plain patch until now
the recursive struct test needed an update because of the added member. After doing this it works again. Mon Mar 06 15:32:12 2006 Johnny Willemsen <jwillemsen@remedy.nl> * tests/Any/Recursive/client.cpp: * tests/Any/Recursive/Test.idl: Extended this test by making the IDL even a little bit more complex * tao/AnyTypeCode/Any.cpp: * tao/AnyTypeCode/Any_Array_Impl_T.cpp: * tao/AnyTypeCode/Any_Basic_Impl.cpp: * tao/AnyTypeCode/Any_Basic_Impl_T.cpp: * tao/AnyTypeCode/Any_Dual_Impl_T.cpp: * tao/AnyTypeCode/Any_Impl_T.cpp: * tao/AnyTypeCode/Any_Special_Impl_T.cpp: * tao/AnyTypeCode/Any_SystemException.cpp: * tao/AnyTypeCode/Any_Unknown_IDL_Type.cpp: * tao/AnyTypeCode/Empty_Param_TypeCode.cpp: * tao/AnyTypeCode/Enum_TypeCode.cpp: * tao/AnyTypeCode/Enum_TypeCode_Static.cpp: * tao/AnyTypeCode/ExceptionA.cpp: * tao/AnyTypeCode/TypeCode.cpp: * tao/AnyTypeCode/TypeCode_Case_T.cpp: * tao/AnyTypeCode/TypeCode_CDR_Extraction.cpp: * tao/AnyTypeCode/Union_TypeCode.cpp: * tao/AnyTypeCode/Union_TypeCode_Static.cpp: * tao/AnyTypeCode/Value_TypeCode.cpp: * tao/AnyTypeCode/Value_TypeCode_Static.cpp: Initialise some pointers with 0, made some local variables const, use true/false instead of 1/0. Fixed bugzilla bug 2323 by: - When marshaling recursive unions for each case use a seperate stream so that the offsets do get calculated - For each recursive type be able to handle multiple members of the recursive type, we could only handle one occurence. For recursive types we should have more test cases, probably there are a few small bugs left in this code but these aren't catched by the current regression test suite. |