Bugzilla – Bug 337
opt build fails in address.cc on Ubuntu 8.10
Last modified: 2008-10-02 19:09:03 EDT
x86 Ubuntu 8.10 alpha 5 cc1plus: warnings being treated as errors In function ‘void* memset(void*, int, size_t)’, inlined from ‘ns3::Address::Address()’ at ../src/node/address.cc:13: /usr/include/bits/string3.h:82: error: call to ‘__warn_memset_zero_len’ declared with attribute warning: memset used with constant zero length parameter; this could be due to transposed parameters In function ‘void* memset(void*, int, size_t)’, inlined from ‘ns3::Address::Address()’ at ../src/node/address.cc:13: /usr/include/bits/string3.h:82: error: call to ‘__warn_memset_zero_len’ declared with attribute warning: memset used with constant zero length parameter; this could be due to transposed parameters Since this memset says "fill in the first zero bytes with the value zero", it does absolutely nothing, so the fix is to remove that line.
Created attachment 251 [details] fix If there is no comment I will push by the end of the day.
Looking carefully at the code, there is something obviously wrong with it. For example, the following is completely braindead and there are many instances of it. memset (m_data, 0, m_len); memcpy (m_data, buffer, m_len); So, there is something fishy in this code (and yes, it's all mine so, it's all my fault) and we need to look at it carefully rather than just get rid of the compiler warning mindlessly. We should figure out what was the intent of the code.
This has the "signature" of a replace-string or sed type of error. It seems that the memset is trying to clear the data buffer and the memcpy is trying to copy the new (possibly fewer) bits in. Now, memset (m_data, 0, sizeof(m_data)); memcpy (m_data, buffer, m_len); would make a whole lot more sense. As Mathieu observes, though, was something else going on, or did sizeof(m_data) or the equivalent just get caught up in a sed script?
It looks like copy-paste type of error to me. Maybe you started by trying to enforce a good practice of initializing all of your memory (which was fine when m_len > 0 and you have no data to copy). Then the cases came up where you DID have data to copy, and a few copy and pastes later, that practice becomes erroneous and we have the current code. The alternative to the "just get rid of the compiler warning" solution then is remove all of these superfluous memsets. Craig I'm not sure I follow your comment...isn't sizeof(m_data) always a constant, the size of a pointer on your architecture (probably 4 bytes)?
Scratch that last comment Craig, I had a brain fart :-)
(In reply to comment #3) > memset (m_data, 0, sizeof(m_data)); > memcpy (m_data, buffer, m_len); I had something similar in mind with sizeof (m_data) replaced with MAX_LEN but I have not spent enough time looking at the code to figure out whether it would make sense.
Heh, I can see myself making exactly this error very clearly. Given, NS_ASSERT (m_len <= MAX_SIZE); memset (m_data, 0, sizeof(m_data)); memcpy (m_data, address.m_data, m_len); I might think to myself, why use sizeof(m_data) when I use MAX_SIZE on the line before. I'll just change it to make it clearer. s/sizeof(m_data)/ *ring* *ring* [conversation about buffer overflows and lengths] Where was I? Oh, about m_len and and that sizeof ... s/sizeof(m_data)/m_len/g :-)
The underlying issue is that the code in address.cc has redundant bit sets and has done this since the beginning of time (wrt this file). However it is generally harmless. The new GCC optimizer in 4.3.2 as configured by Ubuntu 8.10 seems to try and optimize one of the cases out and complains that it doesn't make sense to try and zero out zero bytes (which it doesn't). At this point, however, the cost/benefit analysis may tend to recommend that it is more risky to change it now and possibly uncover other strange dependencies than to try and make the optimizations turned on in alpha 5 of Ubuntu Insane Ibix work (vanill GCC 4.3.2 does work fine according to ML). We can chance it, or we can turn this bug down to P3 and add a release note saying that there is an optimized build issue with Ubuntu 8.10 After all of the recent chaos, I tend to agree with ML -- turn it down to P3 and don't take any more chances.
(In reply to comment #8) > The underlying issue is that the code in address.cc has redundant bit sets and > has done this since the beginning of time (wrt this file). However it is > generally harmless. The new GCC optimizer in 4.3.2 as configured by Ubuntu > 8.10 seems to try and optimize one of the cases out and complains that it > doesn't make sense to try and zero out zero bytes (which it doesn't). > Yes, something like this. Upon looking into the Ubuntu string.h header, there is in fact an additional warning attribute that says something along the lines of "check to make sure parameter such and such is not zero". I haven't looked, but I am guessing that other implementations don't have this warning, and I'm at a loss for why only opt builds trigger the warning... > At this point, however, the cost/benefit analysis may tend to recommend that it > is more risky to change it now and possibly uncover other strange dependencies > than to try and make the optimizations turned on in alpha 5 of Ubuntu Insane > Ibix work (vanill GCC 4.3.2 does work fine according to ML). I would think that if we somehow broke the Address class, we would see all sorts of changes in our traces, i.e. regression failures. The code as written is also clearly redundant but harmless. That said... > We can chance it, or we can turn this bug down to P3 and add a release note > saying that there is an optimized build issue with Ubuntu 8.10 > > After all of the recent chaos, I tend to agree with ML -- turn it down to P3 > and don't take any more chances. > I would be fine with this, provided the issue is fixed before ns-2.3.
erm, ns-3.3
Fix in 3.3
Verified fix on ns-intrepid-ubuntu