Fri 19 Sep 2008 01:23:41 PM UTC, comment #11:
I re-assign the bug to Petr since he was faster
at least this time :=)
|
Thu 18 Sep 2008 02:42:56 PM UTC, comment #10:
Fixed.
|
Thu 18 Sep 2008 10:10:41 AM UTC, comment #9:
You can get rid of typedef which are unused
and not part of external HLA interface
which should be the case for the type
defined inside certi:: namespace like those.
Applications should not use certi:: types,
the Billard does (certi::MessageBuffer)
but the HLATestsSuite and HLA_Tutorial do not.
I suggest not to reuse those type unless it's appropriate.
For example, I think:
typedef char *AttributeValue
should be deleted and replaced with
typedef std::string AttributeValue_t
I'd like to use _t suffix in order
to make it clear it's a type.
If you encounter type which are still in use
(like typedef char *ObjectName ; )
but may be replace with std::string do not
change them with this patch.
We should do it separatly in order not to include
two much changes in a single patch, this would
make it harder to track regression (if any).
|
Thu 18 Sep 2008 09:32:08 AM UTC, comment #8:
Give me few hours. I made the update you suggested, but:
(1) I got a new segmentation fault that needs to be fixed.
(2) I have a question. ;-)
In certi.hh I found some typedefs that are not used and that may be re-used to solve the issue no.1 you mentioned.
Namely:
// Deprecated HLA types still in use
typedef char *AttributeValue ;
// typedef std::vector<char> AttributeValue;
typedef char *ObjectName ;
typedef AttributeValue ParameterValue ;
typedef char *ParameterName ;
typedef AttributeValue ValueName ;
struct Value {
ValueName name ;
ValueName value ;
ValueName type ;
ULong length ;
};
Can the unused typedefs be deleted or re-used, or are there some applications that might use these?
|
Wed 17 Sep 2008 09:15:56 PM UTC, comment #7:
Petr,
If you don't have time to update your patch,
apply-it and I will do the suggested update myself.
The suggested change have no functionnal impact so your
patch is OK.
|
Wed 17 Sep 2008 12:12:17 PM UTC, comment #6:
A typo was in my preceding comment
valueArray.erase(Rank);
should be
valueArray.erase(valueArray.begin()+Rank);
since the argument of erase should be an iterator.
|
Wed 17 Sep 2008 12:07:00 PM UTC, comment #5:
Hi Petr,
Fast and nice fix thanks,
I propose you some pruning.
My modification proposal would be:
1) Use meaningfull type name if possible
typedef std::string AttributeValue_t
typedef std::string ParameterValue_t
such that
vector<ParameterValue_t> will have more meaning
than
vector<std::string> in a method signature
2) Use MessageBuffer::read_string/write_string API
instead of the read_int64 + read_bytes
since it already does this (with an int32)
You should update MessageBuffer.cc since
there was a bug precisely inside the xxxx_string API :=)
3) Since we now have plain vector some of the loop (if not all)
in NetworkMessage::removeXXXX(USHort Rank)
may be replaced with STLish equivalent:
valueArray.erase(Rank);
What do you think?
|
Wed 17 Sep 2008 10:19:40 AM UTC, comment #4:
Fixed.
Attached patch fixes this bug and simplifies the code. (I even believe it's more efficient than before.) It works for me, so I'm happy. You can try it if you want.
I will commit (or update) this patch once my previous suggestion is accepted (or not).
(file #16509)
|
Wed 17 Sep 2008 07:53:02 AM UTC, comment #3:
When discussing bug #23150 we said that std::string is preferred wherever the new/delete char[] causes problems.
I suggest to replace char* in NetworkMessage by std::string. (If you agree, I'll make the change.)
Migrating to std::string wouldn't cause a big penalty in this case because the Values are anyway duplicated everywhere, so the performance is comparable to std::string.
|
Tue 16 Sep 2008 06:19:23 PM UTC, comment #2:
I think help is welcome,
Jean-Loup is working on it however as a "part time work".
Yes there is too many new char[xxx],
May be the API message->getValue() is a bit flawed too.
Depending on time-to-spend on this,
the fix may be small redesign (this should be an internal CERTI API) or a [may be not so] easy, avoid NULL and mem leak :=)
|
Tue 16 Sep 2008 04:39:27 PM UTC, comment #1:
Any update on this? Do you need a helping hand?
I used valgrind and found several strange things in the code:
(1) valgrind reports wrong memory access to a VERY_LOW address number
NetworkMessage.cc:131 and NetworkMessage.cc:138
memcpy(..., (char )(ValueArray[Rank].value), *length)
Couldn't this be caused by a (strange) dereference of ValueArray[Rank].value? It seems to me you're dereferencing char* and casting it back to (char *)
(2) after fixing this, valgrind reported wrong memory access in ObjectClassBroadcastList.cc:111
memcpy(tempValue, buffer, length)
I think buffer is NULL. I think the buffer should have been set by the message->getValue() few lines above, however when buffer is NULL, the getValue RETURNS the newly allocated buffer value. I guess this value is lost here and buffer is still NULL
(3) isn't there too many new char[length]? The buffer is allocated in message->getValue(), then it's allocated before ObjectClassBroadcastList.cc:111 again. What happens to the value allocated in message->getValue()?
|
Thu 24 Jul 2008 08:56:35 AM UTC, original submission:
Crashed on updateAttributeValues
Here are more than one federate,If federate A publishs two attributes,and federate B only subscribes
one attribute,when federate A call updateAttributeValue function, an error will be occured, and rtig will be Crashed.
my plateform is windows XP,and CERTI is compiled by vs2005.
|