Meditating on the code a little I would like some feedback... The fact that put32, putbe32, and their get equivalents all have 32 in the name, I am guessing that it is safe to assume that the intention of the code was to deal specifically with 32 bit objects. If that is the case, then it makes sense why we are having some 32/64 bit issues in places like tcpmem.cc: 1486.
I would like a little feedback on something before doing some serious splunking in the code: Since that body of the code is C++, one way to handle it is to template it and deal appropriately with 32/64 bit objects. On the other hand, it appears to be inherently 32bit objects, then it will be a lot easier to hack to deal with int32's etc. Is it necessary to port it specifically to deal with 64bit objects, or is it sufficient to keep it 32bit? I'm hoping the latter. I'll try to take a poke and give it another try. Since I do not have a sufficient test bench to fully test this, I will send patch sets off to the list (or specific people) to verify that it is working on 32 and 32/64 setups. EBo -- On Apr 21 2014 11:43 PM, Sebastian Kuzminsky wrote: > On 04/21/2014 08:51 PM, EBo wrote: >> I changed diag_info_buf and temp_buf to unsigned int32 to shut up >> the >> majority of the warnings (which required them being casted back to >> char >> in a couple of places). This seemed the most straight forward way >> at >> the time. When I looked at the code it seemed that most of the >> places >> the temp_buf was used to index the long (ie u_int32's) > > Ah, right, i see what you did. In my haste i missed the bottom of > your > patch, sorry. > > Note that this trick only works because those variables are *only* > accessed as uint32_t and char. If they were also accessed as (for > example) uint16_t, then you'd need to do the normal thing of using > memcpy() to explicitly transfer the memory between the data types you > need. > > > You have to be very careful when changing the data types of existing > variables like that. After applying libnml_buffer_u_long_conv.patch, > there is now a new bug on line 182 of tcpmem.cc due to diag_info_buf > changing type, and on line 328 due to temp_buffer changing type. > There > may be others, those are the first ones i noticed. > > > If you're going to fix up these patches, I would suggest that you > also > simplify the uint32_t accesses to those variables. Replace code like > this: >> status = (CMS_STATUS) ntohl(*((uint32_t *) temp_buffer + 1)); > > With code like this: >> status = (CMS_STATUS) ntohl(temp_buffer[1]); > > > Finally, on a different note, why doesn't line 351/352 of tcpmem.cc > trigger the same strict-aliasing warning? > > > http://git.linuxcnc.org/gitweb?p=linuxcnc.git;a=blob;f=src/libnml/buffer/tcpmem.cc;h=2520ac978f5a2bd99fea8c6f86eb3cc4cf8a1f7b;hb=9a9a5bf75e1dcb3b8f76cc720635556d71b7acf1#l351 > > > Again, thanks for tackling the thankless job of fixing this... ------------------------------------------------------------------------------ Start Your Social Network Today - Download eXo Platform Build your Enterprise Intranet with eXo Platform Software Java Based Open Source Intranet - Social, Extensible, Cloud Ready Get Started Now And Turn Your Intranet Into A Collaboration Platform http://p.sf.net/sfu/ExoPlatform _______________________________________________ Emc-developers mailing list Emc-developers@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/emc-developers
