Bug 486

Created attachment 368 [details]
ns-3-dev icc fixups

Added second patch.

Comment on attachment 368 [details]
ns-3-dev icc fixups

Eh ? The following change can't possibly be right: what is the error message output by icc ?

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/core/object.h
>--- a/src/core/object.h	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/core/object.h	Wed Feb 04 13:08:04 2009 +0100
>@@ -403,15 +403,15 @@ Ptr<T> CreateObject (const AttributeList
> 
> template <typename T>
> Ptr<T> 
>-CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
>-              std::string n2 = "", const AttributeValue & v2 = EmptyAttributeValue (),
>-              std::string n3 = "", const AttributeValue & v3 = EmptyAttributeValue (),
>-              std::string n4 = "", const AttributeValue & v4 = EmptyAttributeValue (),
>-              std::string n5 = "", const AttributeValue & v5 = EmptyAttributeValue (),
>-              std::string n6 = "", const AttributeValue & v6 = EmptyAttributeValue (),
>-              std::string n7 = "", const AttributeValue & v7 = EmptyAttributeValue (),
>-              std::string n8 = "", const AttributeValue & v8 = EmptyAttributeValue (),
>-              std::string n9 = "", const AttributeValue & v9 = EmptyAttributeValue ())
>+CreateObject (std::string n1 , const AttributeValue & v1,
>+              std::string n2 , const AttributeValue & v2,
>+              std::string n3 , const AttributeValue & v3,
>+              std::string n4 , const AttributeValue & v4,
>+              std::string n5 , const AttributeValue & v5,
>+              std::string n6 , const AttributeValue & v6,
>+              std::string n7 , const AttributeValue & v7,
>+              std::string n8 , const AttributeValue & v8,
>+              std::string n9 , const AttributeValue & v9)
> {
>   AttributeList attributes;
>   if (n1 == "")

What warning are you fixing below ?

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/internet-stack/ipv4-l3-protocol.cc
>--- a/src/internet-stack/ipv4-l3-protocol.cc	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/internet-stack/ipv4-l3-protocol.cc	Wed Feb 04 13:08:04 2009 +0100
>@@ -1068,8 +1068,8 @@ Ipv4L3Protocol::SetUp (uint32_t i)
>   // If interface address and network mask have been set, add a route
>   // to the network of the interface (like e.g. ifconfig does on a
>   // Linux box)
>-  if ((interface->GetAddress ()) != (Ipv4Address ())
>-      && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
>+  if ( ! interface->GetAddress ().IsEqual (Ipv4Address ())
>+       && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
>     {
>       AddNetworkRouteTo (interface->GetAddress ().CombineMask (interface->GetNetworkMask ()),
>                          interface->GetNetworkMask (), i);

This is not really a warning fix, right ?

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/node/queue.cc
>--- a/src/node/queue.cc	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/node/queue.cc	Wed Feb 04 13:08:04 2009 +0100
>@@ -84,12 +84,12 @@ Queue::Dequeue (void)
> 
>   if (packet != 0)
>     {
>+      NS_ASSERT (m_nBytes >= packet->GetSize ());
>+      NS_ASSERT (m_nPackets > 0);
>+
>       m_nBytes -= packet->GetSize ();
>       m_nPackets--;
> 
>-      NS_ASSERT (m_nBytes >= 0);
>-      NS_ASSERT (m_nPackets >= 0);
>-
>       NS_LOG_LOGIC("m_traceDequeue (packet)");
> 
>       m_traceDequeue (packet);

What warning are you fixing below ? LLU is not very portable so, adding:
uint64_t pow10 (uint8_t n)
{
  NS_ASSERT (n > 0);
  uint64_t result = 1;
  for (uint8_t i = 0; i < n; i++)
    {
      result *= 10;
    }
  return result;
}
would be probably more portable.

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/simulator/time.cc
>--- a/src/simulator/time.cc	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/simulator/time.cc	Wed Feb 04 13:08:04 2009 +0100
>@@ -32,11 +32,11 @@ namespace ns3 {
> 
> namespace TimeStepPrecision {
> 
>-static const uint64_t MS_FACTOR = (uint64_t)pow(10,3);
>-static const uint64_t US_FACTOR = (uint64_t)pow(10,6);
>-static const uint64_t NS_FACTOR = (uint64_t)pow(10,9);
>-static const uint64_t PS_FACTOR = (uint64_t)pow(10,12);
>-static const uint64_t FS_FACTOR = (uint64_t)pow(10,15);
>+static const uint64_t MS_FACTOR = 1000LLU;
>+static const uint64_t US_FACTOR = 1000000LLU;
>+static const uint64_t NS_FACTOR = 1000000000LLU;
>+static const uint64_t PS_FACTOR = 1000000000000LLU;
>+static const uint64_t FS_FACTOR = 1000000000000000LLU;
> static uint64_t g_tsPrecFactor = NS_FACTOR;
> 
> static GlobalValue g_precisionDefaultValue ("TimeStepPrecision",

Yes, sorry, some are warnings some are errors.

(In reply to comment #2)
> (From update of attachment 368 [details])
> Eh ? The following change can't possibly be right: what is the error message
> output by icc ?

This is about the default arguments, they should only be declared in the function declaration.

../src/core/object.h(406): warning #845: specifying a default argument when redeclaring an unreferenced function template is nonstandard
  CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
  ^

../src/core/object.h(406): warning #350: redefinition of default argument
  CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
  ^

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/core/object.h
> >--- a/src/core/object.h	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/core/object.h	Wed Feb 04 13:08:04 2009 +0100
> >@@ -403,15 +403,15 @@ Ptr<T> CreateObject (const AttributeList
> > 
> > template <typename T>
> > Ptr<T> 
> >-CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
> >-              std::string n2 = "", const AttributeValue & v2 = EmptyAttributeValue (),
> >-              std::string n3 = "", const AttributeValue & v3 = EmptyAttributeValue (),
> >-              std::string n4 = "", const AttributeValue & v4 = EmptyAttributeValue (),
> >-              std::string n5 = "", const AttributeValue & v5 = EmptyAttributeValue (),
> >-              std::string n6 = "", const AttributeValue & v6 = EmptyAttributeValue (),
> >-              std::string n7 = "", const AttributeValue & v7 = EmptyAttributeValue (),
> >-              std::string n8 = "", const AttributeValue & v8 = EmptyAttributeValue (),
> >-              std::string n9 = "", const AttributeValue & v9 = EmptyAttributeValue ())
> >+CreateObject (std::string n1 , const AttributeValue & v1,
> >+              std::string n2 , const AttributeValue & v2,
> >+              std::string n3 , const AttributeValue & v3,
> >+              std::string n4 , const AttributeValue & v4,
> >+              std::string n5 , const AttributeValue & v5,
> >+              std::string n6 , const AttributeValue & v6,
> >+              std::string n7 , const AttributeValue & v7,
> >+              std::string n8 , const AttributeValue & v8,
> >+              std::string n9 , const AttributeValue & v9)
> > {
> >   AttributeList attributes;
> >   if (n1 == "")
> 
> What warning are you fixing below ?

It's an error, which I cannot tell you why it occurs:

../src/internet-stack/ipv4-l3-protocol.cc(1071): error: cast to type "ns3::Ipv4Address ()" is not allowed
    if ((interface->GetAddress ()) != (Ipv4Address ())
                                       ^

../src/internet-stack/ipv4-l3-protocol.cc(1072): error: expected an expression
        && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
        ^

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/internet-stack/ipv4-l3-protocol.cc
> >--- a/src/internet-stack/ipv4-l3-protocol.cc	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/internet-stack/ipv4-l3-protocol.cc	Wed Feb 04 13:08:04 2009 +0100
> >@@ -1068,8 +1068,8 @@ Ipv4L3Protocol::SetUp (uint32_t i)
> >   // If interface address and network mask have been set, add a route
> >   // to the network of the interface (like e.g. ifconfig does on a
> >   // Linux box)
> >-  if ((interface->GetAddress ()) != (Ipv4Address ())
> >-      && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
> >+  if ( ! interface->GetAddress ().IsEqual (Ipv4Address ())
> >+       && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
> >     {
> >       AddNetworkRouteTo (interface->GetAddress ().CombineMask (interface->GetNetworkMask ()),
> >                          interface->GetNetworkMask (), i);
> 
> This is not really a warning fix, right ?

Warning about comparision of unsigned int >= 0. I changed the code to actually compare something.

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/node/queue.cc
> >--- a/src/node/queue.cc	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/node/queue.cc	Wed Feb 04 13:08:04 2009 +0100
> >@@ -84,12 +84,12 @@ Queue::Dequeue (void)
> > 
> >   if (packet != 0)
> >     {
> >+      NS_ASSERT (m_nBytes >= packet->GetSize ());
> >+      NS_ASSERT (m_nPackets > 0);
> >+
> >       m_nBytes -= packet->GetSize ();
> >       m_nPackets--;
> > 
> >-      NS_ASSERT (m_nBytes >= 0);
> >-      NS_ASSERT (m_nPackets >= 0);
> >-
> >       NS_LOG_LOGIC("m_traceDequeue (packet)");
> > 
> >       m_traceDequeue (packet);
> 
> What warning are you fixing below ? LLU is not very portable so, adding:
> uint64_t pow10 (uint8_t n)
> {
>   NS_ASSERT (n > 0);
>   uint64_t result = 1;
>   for (uint8_t i = 0; i < n; i++)
>     {
>       result *= 10;
>     }
>   return result;
> }
> would be probably more portable.

Interesting to talk about portability between compilers here ;).

This is actually a runtime error. Any example program will segfault, because these static consts are incorrectly initialized, gdb says they are 0 at segfault time.

Your idea with pow10 did not work.

The issue is about initialization order: in my dump WifiMac::GetDefaultSifs() is called during initialization _before_ the pow10() is calculated. SIFS is returned with MicroSeconds(), which uses one of the factors.

static const uint64_t MS_FACTOR = (uint64_t)1000;
static const uint64_t US_FACTOR = (uint64_t)1000000;
static const uint64_t NS_FACTOR = (uint64_t)1000000 * (uint64_t)1000;
static const uint64_t PS_FACTOR = (uint64_t)1000000 * (uint64_t)1000000;
static const uint64_t FS_FACTOR = (uint64_t)1000000 * (uint64_t)1000000 * (uint64_t)1000;

The above works. The compilers (gcc/icc) seem to be able to fold the expression.

> >diff -r 3e819441bc75 -r 0ad61c84f3c6 src/simulator/time.cc
> >--- a/src/simulator/time.cc	Tue Feb 03 06:56:47 2009 -0800
> >+++ b/src/simulator/time.cc	Wed Feb 04 13:08:04 2009 +0100
> >@@ -32,11 +32,11 @@ namespace ns3 {
> > 
> > namespace TimeStepPrecision {
> > 
> >-static const uint64_t MS_FACTOR = (uint64_t)pow(10,3);
> >-static const uint64_t US_FACTOR = (uint64_t)pow(10,6);
> >-static const uint64_t NS_FACTOR = (uint64_t)pow(10,9);
> >-static const uint64_t PS_FACTOR = (uint64_t)pow(10,12);
> >-static const uint64_t FS_FACTOR = (uint64_t)pow(10,15);
> >+static const uint64_t MS_FACTOR = 1000LLU;
> >+static const uint64_t US_FACTOR = 1000000LLU;
> >+static const uint64_t NS_FACTOR = 1000000000LLU;
> >+static const uint64_t PS_FACTOR = 1000000000000LLU;
> >+static const uint64_t FS_FACTOR = 1000000000000000LLU;
> > static uint64_t g_tsPrecFactor = NS_FACTOR;
> > 
> > static GlobalValue g_precisionDefaultValue ("TimeStepPrecision", 
> 

Created attachment 369 [details]
ns-3-dev icc fixups

Updated patch.

(In reply to comment #0)
> Created an attachment (id=367) [details]
> icc flags for waf
> 
> These two patches enable compilation with icc.
> 
> The new waf has support for icpc, but the cflags extension in our patched waf
> does not. See waf-gjc-icc-cflags.patch for that. The patch does not add -Wall
> to CXXFLAGS because it makes icc very verbose.
> 

To be clear, the first patch isn't for ns-3-dev, right?  You are saying that it is a patch against Gustavo's fork of waf?
https://code.launchpad.net/~gjc/waf/cmd

If so, the first patch is orthogonal to ns-3.  I suggest Gustavo decide what to do with it (i.e. whether or not to merge it into his waf fork).  After doing so, he can generate a new standalone waf and include this in ns-3-dev.

From the perspective of ns-3, compilation with icc should probably be a not officially supported feature (simply a feature of waf, which we happen to use), meaning that the build could break anytime if you are using icc.  That said, if changes like the second patch are proposed for ns-3 which don't regress the gcc builds and generally make our code more standards compliant / cleaner, we should accept them.

Right on.

Yes the first patch is for Gustavo's waf.

icc is farly compatible with g++. The errors and warning it outputs generally lead to higher code quality.

Comment on attachment 367 [details]
icc flags for waf

applied to the waf/cmd tree on launchpad, will later sync to ns-3-dev's waf

Comment on attachment 369 [details]
ns-3-dev icc fixups

>diff -r 3e819441bc75 -r 0ad61c84f3c6 src/core/object.h
>--- a/src/core/object.h	Tue Feb 03 06:56:47 2009 -0800
>+++ b/src/core/object.h	Wed Feb 04 13:08:04 2009 +0100
>@@ -403,15 +403,15 @@ Ptr<T> CreateObject (const AttributeList
> 
> template <typename T>
> Ptr<T> 
>-CreateObject (std::string n1 = "", const AttributeValue & v1 = EmptyAttributeValue (),
>-              std::string n2 = "", const AttributeValue & v2 = EmptyAttributeValue (),
>-              std::string n3 = "", const AttributeValue & v3 = EmptyAttributeValue (),
>-              std::string n4 = "", const AttributeValue & v4 = EmptyAttributeValue (),
>-              std::string n5 = "", const AttributeValue & v5 = EmptyAttributeValue (),
>-              std::string n6 = "", const AttributeValue & v6 = EmptyAttributeValue (),
>-              std::string n7 = "", const AttributeValue & v7 = EmptyAttributeValue (),
>-              std::string n8 = "", const AttributeValue & v8 = EmptyAttributeValue (),
>-              std::string n9 = "", const AttributeValue & v9 = EmptyAttributeValue ())
>+CreateObject (std::string n1 , const AttributeValue & v1,
>+              std::string n2 , const AttributeValue & v2,
>+              std::string n3 , const AttributeValue & v3,
>+              std::string n4 , const AttributeValue & v4,
>+              std::string n5 , const AttributeValue & v5,
>+              std::string n6 , const AttributeValue & v6,
>+              std::string n7 , const AttributeValue & v7,
>+              std::string n8 , const AttributeValue & v8,
>+              std::string n9 , const AttributeValue & v9)
> {
>   AttributeList attributes;
>   if (n1 == "")


This chunk worries me.  You are removing the default parameter values; how does this not break existing code?

(In reply to comment #8)
> 
> 
> This chunk worries me.  You are removing the default parameter values; how does
> this not break existing code?
> 

As far as I know, the standard says that default parameters should only go in the signature of the specification of the method, not in the implementation.  Although putting them in the implementation signature improves readability (and GCC allows it), it is not C++ standard.

(In reply to comment #8)
> 
> This chunk worries me.  You are removing the default parameter values; how does
> this not break existing code?
> 

Don't look at just the diff. The default parameters are declared in the function's declaration a few lines above. They should not be repeated in the function's definition.

(In reply to comment #3)

> > What warning are you fixing below ?
> 
> It's an error, which I cannot tell you why it occurs:
> 
> ../src/internet-stack/ipv4-l3-protocol.cc(1071): error: cast to type
> "ns3::Ipv4Address ()" is not allowed
>     if ((interface->GetAddress ()) != (Ipv4Address ())
>                                        ^
> ../src/internet-stack/ipv4-l3-protocol.cc(1072): error: expected an expression
>         && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
>         ^

I stepped on this bug while compiling ns-3 with g++ 4.4 snapshot:

../src/internet-stack/ipv4-l3-protocol.cc: In member function ‘void ns3::Ipv4L3Protocol::SetUp(uint32_t)’:
../src/internet-stack/ipv4-l3-protocol.cc:1072: error: expected identifier before ‘(’ token
../src/internet-stack/ipv4-l3-protocol.cc:1072: warning: taking the address of a label is non-standard
../src/internet-stack/ipv4-l3-protocol.cc:1072: error: expected ‘)’ before ‘(’ token

icc is more informative here; at least it tells us that it expects something to be casted to Ipv4Mask() (how did it get that idea?). Adding another pair of parentheses quieted the compiler:

--- ns-3-dev/src/internet-stack/ipv4-l3-protocol.cc	2009-02-14 03:53:06.000000000 +0300
+++ ns-3-rev/src/internet-stack/ipv4-l3-protocol.cc	2009-02-14 03:45:42.000000000 +0300
@@ -1068,7 +1068,7 @@
   // If interface address and network mask have been set, add a route
   // to the network of the interface (like e.g. ifconfig does on a
   // Linux box)
-  if ((interface->GetAddress ()) != (Ipv4Address ())
+  if (((interface->GetAddress ()) != (Ipv4Address ()))
       && (interface->GetNetworkMask ()) != (Ipv4Mask ()))
     {
       AddNetworkRouteTo (interface->GetAddress ().CombineMask (interface->GetNetworkMask ()),

changeset da9be6abb1b2

same as last patch, except with change from Anatoly Kamchatnov