net/sctp/sm_make_chunk.c alignment problems on parisc64

net/sctp/sm_make_chunk.c alignment problems on parisc64

[Arnaldo Carvalho de Melo]

Latest 2.6 bk tree, but I think this is hanging there for quite a while.

CC [M]  net/sctp/sm_make_chunk.o
{standard input}: Assembler messages:
{standard input}:2386: Error: Field not properly aligned [8] (52).
{standard input}:2386: Error: Invalid operands
{standard input}:2398: Error: Field not properly aligned [8] (52).
{standard input}:2398: Error: Invalid operands
make[2]: *** [net/sctp/sm_make_chunk.o] Error 1
make[1]: *** [net/sctp] Error 2
make: *** [net] Error 2

it happens in the sctp_pack_cookie function.

- Arnaldo

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[Sridhar Samudrala]

On Fri, 12 Sep 2003, Arnaldo Carvalho de Melo wrote:

> Latest 2.6 bk tree, but I think this is hanging there for quite a while.
> 
> CC [M]  net/sctp/sm_make_chunk.o
> {standard input}: Assembler messages:
> {standard input}:2386: Error: Field not properly aligned [8] (52).
> {standard input}:2386: Error: Invalid operands
> {standard input}:2398: Error: Field not properly aligned [8] (52).
> {standard input}:2398: Error: Invalid operands
> make[2]: *** [net/sctp/sm_make_chunk.o] Error 1
> make[1]: *** [net/sctp] Error 2
> make: *** [net] Error 2
> 
> it happens in the sctp_pack_cookie function.

I don't see this problem on i386, ia64 or ppc64. Can someone familiar with parisc64
provide more details or submit a a patch to fix this problem?

Thanks
Sridhar

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[David S. Miller]

On Wed, 17 Sep 2003 16:06:18 -0700 (PDT)
Sridhar Samudrala <sri@us.ibm.com> wrote:

> I don't see this problem on i386, ia64 or ppc64. Can someone
> familiar with parisc64 provide more details or submit a a patch to
> fix this problem?

As an example, if you have an structure member of type "char":

struct foo {
	char	a;
	char	b[4];
};

And then try to do something like this:

	struct foo *p;
	unsigned int *v;

	v = (unsigned int *) (&p->b[0]);
	*v = 0;

The build is going to explode on parisc because this simply is not
allowed.  You cannot access a structure member as an object which
has larger alignment than is guarenteed for the type that member
has.

In the above example we're trying to access with 'unsigned int'
alignment a member which is only guarenteed to have the alignment
for a 'char'.

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[Sridhar Samudrala]

On Thu, 18 Sep 2003, David S. Miller wrote:

> On Wed, 17 Sep 2003 16:06:18 -0700 (PDT)
> Sridhar Samudrala <sri@us.ibm.com> wrote:
> 
> > I don't see this problem on i386, ia64 or ppc64. Can someone
> > familiar with parisc64 provide more details or submit a a patch to
> > fix this problem?
> 
> As an example, if you have an structure member of type "char":
> 
> struct foo {
> 	char	a;
> 	char	b[4];
> };
> 
> And then try to do something like this:
> 
> 	struct foo *p;
> 	unsigned int *v;
> 
> 	v = (unsigned int *) (&p->b[0]);
> 	*v = 0;
> 
> The build is going to explode on parisc because this simply is not
> allowed.  You cannot access a structure member as an object which
> has larger alignment than is guarenteed for the type that member
> has.
> 
> In the above example we're trying to access with 'unsigned int'
> alignment a member which is only guarenteed to have the alignment
> for a 'char'.
 

Thanks for explaining with an example. 

But unfortunately i am not able see this problem with a parisc64 cross compiler 
on i386. So it makes it hard to debug or fix it. Looks like this happens only
when building natively on a parisc64 machine which i don't have access to.

>From the following original note from Arnaldo
----------------------------------------- 
CC [M]  net/sctp/sm_make_chunk.o
{standard input}: Assembler messages:
{standard input}:2386: Error: Field not properly aligned [8] (52).
{standard input}:2386: Error: Invalid operands
{standard input}:2398: Error: Field not properly aligned [8] (52).
{standard input}:2398: Error: Invalid operands
make[2]: *** [net/sctp/sm_make_chunk.o] Error 1
make[1]: *** [net/sctp] Error 2
make: *** [net] Error 2

it happens in the sctp_pack_cookie function.
----------------------------------------- 

I am not able to figure out the exact code which is causing this problem as
the line numbers reported seem to correspond to the assembled file.

I am guessing that the following lines in sctp_pack_cookie() may be the
suspects.

        /* Copy the peer's init packet.  */
        memcpy(&cookie->c.peer_init[0], init_chunk->chunk_hdr,
               ntohs(init_chunk->chunk_hdr->length));

        /* Copy the raw local address list of the association. */
        memcpy((__u8 *)&cookie->c.peer_init[0] +
               ntohs(init_chunk->chunk_hdr->length), raw_addrs, addrs_len);


Am i right? But here, i don't see any accesses to a member as an object which
has larger alignment.
If so, is there an easy way to fix these assembler errors on parisc64? 

Also while reviewing the code in sctp_pack_cookie(), i noticed a structure
copy. Are structure copies portable across all the archictectures? Should we
replace it with a memcpy?

Thanks
Sridhar

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[David S. Miller]

On Fri, 19 Sep 2003 15:15:53 -0700 (PDT)
Sridhar Samudrala <sri@us.ibm.com> wrote:

> But unfortunately i am not able see this problem with a parisc64 cross compiler 
> on i386. So it makes it hard to debug or fix it. Looks like this happens only
> when building natively on a parisc64 machine which i don't have access to.

Did you build with or without SMP enabled?

Anyways, try to work with Arnaldo to figure out the precise statement
causing the problems.

> Also while reviewing the code in sctp_pack_cookie(), i noticed a structure
> copy. Are structure copies portable across all the archictectures? Should we
> replace it with a memcpy?

Yes, structure copies are portable.

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[Arnaldo Carvalho de Melo]

Em Fri, Sep 19, 2003 at 11:39:09PM -0700, David S. Miller escreveu:
> On Fri, 19 Sep 2003 15:15:53 -0700 (PDT)
> Sridhar Samudrala <sri@us.ibm.com> wrote:
> 
> > But unfortunately i am not able see this problem with a parisc64 cross compiler 
> > on i386. So it makes it hard to debug or fix it. Looks like this happens only
> > when building natively on a parisc64 machine which i don't have access to.
> 
> Did you build with or without SMP enabled?

Yes, here I'm building without SMP enabled.

> Anyways, try to work with Arnaldo to figure out the precise statement
> causing the problems.

The problem is right at:

tv_add(&asoc->cookie_life, &cookie->c.expiration);

and more specifically the problem is with cookie->c.expiration, that in
turn points to this line:

cookie = (struct sctp_signed_cookie *) retval->body;

Now retval is of this type:

/* Section 3.3.3.1 State Cookie (7) */
typedef struct sctp_cookie_param {
        sctp_paramhdr_t p;
        __u32 body[0];
} sctp_cookie_param_t __attribute__((packed));

I removed the packed attribute both from sctp_cookie_param_t and
sctp_paramhdr_t, the problem persists, ideas?

Please send any patch you come up with, I'll be happy to test it.

- Arnaldo

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[Sridhar Samudrala]

On Sat, 20 Sep 2003, Arnaldo Carvalho de Melo wrote:

> Em Fri, Sep 19, 2003 at 11:39:09PM -0700, David S. Miller escreveu:
> > On Fri, 19 Sep 2003 15:15:53 -0700 (PDT)
> > Sridhar Samudrala <sri@us.ibm.com> wrote:
> > 
> > > But unfortunately i am not able see this problem with a parisc64 cross compiler 
> > > on i386. So it makes it hard to debug or fix it. Looks like this happens only
> > > when building natively on a parisc64 machine which i don't have access to.
> > 
> > Did you build with or without SMP enabled?
> 
> Yes, here I'm building without SMP enabled.

I also started seeing this problem when i enabled CONFIG_64BIT with the
cross-compiler.

> 
> > Anyways, try to work with Arnaldo to figure out the precise statement
> > causing the problems.
> 
> The problem is right at:
> 
> tv_add(&asoc->cookie_life, &cookie->c.expiration);
> 
> Please send any patch you come up with, I'll be happy to test it.

The problem seems to be the static inline routine tv_add. When i converted it
into a regular function or a macro, the problem went away. May be parisc64
compiler has some issues with static inlines.

Arnaldo, Could you please try out this patch which converts tv_add() to a macro
TIMEVAL_ADD()

Thanks
Sridhar 

-----------------------------------------------------------------------------
diff -Nru a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
--- a/include/net/sctp/sctp.h	Mon Sep 22 14:40:46 2003
+++ b/include/net/sctp/sctp.h	Mon Sep 22 14:40:46 2003
@@ -495,22 +495,19 @@
 #define tv_lt(s, t) \
    (s.tv_sec < t.tv_sec || (s.tv_sec == t.tv_sec && s.tv_usec < t.tv_usec))
 
-/* Stolen from net/profile.h.  Using it from there is more grief than
- * it is worth.
- */
-static inline void tv_add(const struct timeval *entered, struct timeval *leaved)
-{
-	time_t usecs = leaved->tv_usec + entered->tv_usec;
-	time_t secs = leaved->tv_sec + entered->tv_sec;
-
-	if (usecs >= 1000000) {
-		usecs -= 1000000;
-		secs++;
-	}
-	leaved->tv_sec = secs;
-	leaved->tv_usec = usecs;
-}
-
+/* Add tv1 to tv2. */
+#define TIMEVAL_ADD(tv1, tv2) \
+({ \
+        suseconds_t usecs = (tv2).tv_usec + (tv1).tv_usec; \
+        time_t secs = (tv2).tv_sec + (tv1).tv_sec; \
+\
+        if (usecs >= 1000000) { \
+                usecs -= 1000000; \
+                secs++; \
+        } \
+        (tv2).tv_sec = secs; \
+        (tv2).tv_usec = usecs; \
+})
 
 /* External references. */
 
diff -Nru a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
--- a/net/sctp/sm_make_chunk.c	Mon Sep 22 14:40:46 2003
+++ b/net/sctp/sm_make_chunk.c	Mon Sep 22 14:40:46 2003
@@ -1288,7 +1288,7 @@
 
 	/* Set an expiration time for the cookie.  */
 	do_gettimeofday(&cookie->c.expiration);
-	tv_add(&asoc->cookie_life, &cookie->c.expiration);
+	TIMEVAL_ADD(asoc->cookie_life, cookie->c.expiration);
 
 	/* Copy the peer's init packet.  */
 	memcpy(&cookie->c.peer_init[0], init_chunk->chunk_hdr,
 
-----------------------------------------------------------------------------

Re: net/sctp/sm_make_chunk.c alignment problems on parisc64

[Arnaldo Carvalho de Melo]

Em Mon, Sep 22, 2003 at 03:06:41PM -0700, Sridhar Samudrala escreveu:
> On Sat, 20 Sep 2003, Arnaldo Carvalho de Melo wrote:
> > The problem is right at:
> > 
> > tv_add(&asoc->cookie_life, &cookie->c.expiration);
> > 
> > Please send any patch you come up with, I'll be happy to test it.
> 
> The problem seems to be the static inline routine tv_add. When i converted it
> into a regular function or a macro, the problem went away. May be parisc64
> compiler has some issues with static inlines.
> 
> Arnaldo, Could you please try out this patch which converts tv_add() to a macro
> TIMEVAL_ADD()

Yes, this makes the problem go away, if you have this tested please push it
DaveM's way, thanks a lot!

- Arnaldo