Please report new issues athttps://github.com/DOCGroup
> Context: I simulate the oes AMI call to the buckets with a simple work > interface. Everything is peachy when I do a remote invocation but core > dumps when I make a colocated call. > > > ------- > > > The idl for the interface is: > > boolean work(out Timing_Data data) > > where: > > struct Timing_Data > { > hrtime_t work_call_received; > hrtime_t work_call_returning; > }; > > In the the Clnt file we have the Buckets_Call::sendc_work AMI invocation > stub. Note the template argument type of variable _tao_retval: > > TAO::Arg_Traits<void>::ret_val _tao_retval; > > I don't see how this can possibly work with a colocated invocation. We > have: > > TAO::Argument *_the_tao_operation_signature [] = > { > &_tao_retval > }; > > This is supplied as an argument to the invocation adapter: > > TAO::Asynch_Invocation_Adapter _tao_call ( > this, > _the_tao_operation_signature, > 1, > "work", > 4, > this->the_TAO_Buckets_Call_Proxy_Broker_ > ); > > then invoke is called on this object: > > _tao_call.invoke ( > ami_handler, > &Timing_Test::AMI_Buckets_CallHandler::work_reply_stub > ACE_ENV_ARG_PARAMETER > ); > > > Later the base class invoke is called: > > Invocation_Adapter::invoke (0, 0 ACE_ENV_ARG_PARAMETER); > > which creates a TAO_Operation_Details using the args_ member which is > _the_tao_operator_signature mentioned above: > > > TAO_Operation_Details op_details (this->operation_, > this->op_len_, > this->number_args_ != 0, > this->args_, > this->number_args_, > ex_data, > ex_count); > > and invoke_i is called which in turn calls invoke_collocated_i since we > are making a colocated call. Continuing on, eventually the skeleton > calls the target operation using the args passed in thru the op_details: > > collocated_skel (servant, > args, > num_args > ACE_ENV_ARG_PARAMETER); > > This little gem: > ((TAO::Arg_Traits< ACE_InputCDR::to_boolean>::ret_val *) args[0])->arg > () = > reinterpret_cast< > POA_Timing_Test::Buckets_Call_ptr> ( > servant->_downcast ( > "IDL:Timing_Test/Buckets_Call:1.0" > ) > )->work ( > ((TAO::Arg_Traits< ::Timing_Test::Timing_Data>::out_arg_val *) > args[1])->arg () > ACE_ENV_ARG_PARAMETER > > calls the implementing function which proceeds to assign a value to the > argument. Since it is the wrong type everything is hosed and a > segmentation fault results. > > This approach seems perfectly sane for a remote invocation since the > args will be supplied at the remote skeleton. > > I can imagine how this should be fixed but can't really visualize it > yet. > > Bottom line, I think there is a bug in TAO. >
Adding Charles Frasch, the original reporter, to the CC list
removed myself as cc, I do get all e-mails from bugzilla already
Charles, can you provide an automated test to reproduce this?
added blocks of beta tao 2.0 bug
regression for this is in my workspace and will be added after x.5.1 is out
with my cvs head (this has some changes in it but not related to AMI) with borland C++ the test just exits normally without a crash.
regression is in the repo
I have been looking at bug 2503, the failing of AMI in collocation. The regression for this bug fails in the following code from PortableServer/get_arg.h template<typename T, typename OUT_ARG_TYPE> OUT_ARG_TYPE get_out_arg (TAO_Operation_Details const * details, TAO::Argument * const * skel_args, size_t i) { return details ? static_cast<typename TAO::Arg_Traits<T>::out_arg_val *> ( details->args ()[i])->arg () : static_cast<typename TAO::SArg_Traits<T>::out_arg_val *> ( skel_args[i])->arg (); } } In the case of AMI and collocation the operation details are a valid pointer, meaning the details->args part is used, but this is the client operation details where we have only have a return value. But, we have AMI, when invoking the servant we have a return value and an output argument for this test, we really should use the skel_args. The method above is called from the following in generated code. virtual void execute (ACE_ENV_SINGLE_ARG_DECL) { TAO::SArg_Traits< ::CORBA::Long>::out_arg_type arg_1 = TAO::Portable_Server::get_out_arg< ::CORBA::Long, TAO::SArg_Traits< ::CORBA::Long>::out_arg_type> ( this->operation_details_, this->args_, 1); this->servant_->the_operation ( arg_1 ACE_ENV_ARG_PARAMETER); ACE_CHECK; } When I replace here this->operation_details_ then things work fine. It seems the usage of the operation_details to determine whether what cast we need to do is not safe for the AMI case.
Created attachment 542 [details] AMI optimization for operations with no in/inout arguments -- UNTESTED
Created attachment 543 [details] Verify operation details pointer is non-zero before accessing members
Jeff, Johnny, I attached two *untested* patches for an optimization that prevents the IDL compiler from generating an argument list in the sendc_ method for the case where the IDL operation has no in/inout parameters. This should reduce footprint and improve sendc_ method performance, both very slightly but an improvement nonetheless. It should also address the specific problem described in this bug report (IDL operation without in/inout parameters) since operation_details should end up having a zero argument list pointer. The proposed patches, however, will not address the more general problem where the IDL operation has in/inout and out parameters. There also appears to be a problem in the Upcall_Wrapper code where the incorrect arguments may be passed to the sending interception point calls for the AMI case since out parameters and return values are not included, and similarly for the direct AMI collocation case. It seems like the right (somewhat hackish) thing to do may be to ensure that the TAO_IDL generated operation signature (TAO::Argument** list) for the sendc_ call contains the same number of arguments as the synchronous version. Parameters that do not apply to the asynchronous (sendc_) version, i.e. out parameters and return values (?) would simply have a zero placeholder in the argument list, e.g.: // IDL bool foo (in a, out b); // sendc_ stub TAO::Argument *_the_tao_operation_signature[] = { 0, &_tao_a, 0 }; The get_{ret,out}_arg<>() function templates should then check for a zero pointer. If zero, use the skel_args<>. I'm not yet sure how the "out" side of inout parameters would work in this approach, however.
from Ossama Another alternative may be to change the static_cast<> for the non-zero operation_details pointer case in the function templates in PortableServer/get_arg.h to a dynamic_cast<> and verify the result is non-zero before attempting to access the thru-POA based argument. Not as fast but I think it'll work.
The dynamic cast would probably work, but we then do a dynamic cast for each argument, shouldn't we just need to do this once for each invocation? Maybe we need a flag that determines we should use sarg or arg and store this in the server request and then use this? That would probably also fix the DII case.
For reference, the original implementation used run-time polymorphism to determine which argument to use. I'm guessing the problem described in this bug report didn't exist in that implementation. Unfortunately, that implementation was fairly heavy. The current function template based approach is much lighter.
I agree, the per-invocation flag seems like the way to go. Good call.
Adding the code lines below to each get_arg tempalte function made 2503 regression work for me. This has to be tested more in detail and more optimized if possible typename TAO::SArg_Traits<T>::ret_val *val = dynamic_cast <typename TAO::SArg_Traits<T>::ret_val *> (skel_args[0]); if (val != 0) return val->arg (); else
With things running now with a partly change I have made the test now in such a way that it ends itself when running fine, will commit that tomorrow
Added 2170 as blocks, is a bug related to this
Just had a look at patch 543, have that already locally. What about adding a flag to operation_details like use_args_for_skel that indicates whether we should use the arguments in the operations details to invoke to the skeleton? I think in get_arg we just don't have enough knowledge to handle also AMI/AMH/DII collocation directly. In the generated code we could then change this flag for some use cases. Also I wonder if we can't zap argument_flag_ from operation details, doesn't num_args_ has enough data?
This all looks good to me, including zapping argument_flag_.
Added depends on
Changed component to AMI
taking this over
accept
Thu Aug 10 09:43:56 UTC 2006 Johnny Willemsen <jwillemsen@remedy.nl> * tao/CodecFactory/IOP_Codec.pidl: * tao/CSD_Framework/CSD_Framework.pidl: Documentation improvements * tao/Connector_Registry.cpp: Improved some debug statements to mention the class and method where the debug message is coming from * tao/CSD_Framework/CSD_FW_Server_Request_Wrapper.cpp: Initialize some pointers with 0 * tao/CSD_Framework/CSD_FW_Server_Request_Wrapper.cpp: * tao/Invocation_Adapter.cpp: * tao/LocateRequest_Invocation_Adapter.cpp: * tao/operation_details.{h,inl}: Removed the argument_flag as member, we just determine whether we have arguments or not on the number of arguments. Changed the accessor to just check it when requested. Introduced a flag whether the stub arguments should be used or not, this can then be changed is some part of the code knows the stub arguments are not valid, for example with AMI. * tao/operation_details.h: Guarded the ft_* methods with TAO_HAS_INTERCEPTORS, the implementation did use the macro, just not the declaration * tao/extra_core.mpb: Removed a generated file from the list of files * tao/GIOP_Message_Generator_Parser_12.cpp: * tao/Service_Context.{h,inl}: is_service_id is now returning a real bool * tao/Messaging/Async_Invocation_Adapter.{h,cpp}: Overruled invoke_collocated_i to set the use_stub_args flag in the operations details to false, when using collocation we should use the skeleton arguments. This fixes bugzilla bug 2503, thanks to Charles Frasch <cfrash at atdesk dot com> for reporting this bug. * tao/PortableServer/get_arg.h: Use the operation_details stub_args method to determine which args should be used * TAO_IDL/be/be_visitor_operation/ami_cs.cpp: Small optimization for AMI operations with just out arguments as supplied by Ossama Othman as partly fix for bug 2503. * tao/PortableServer/Root_POA.cpp: Don't use POAManagerFactory with CORBA/e compact.