Unify module.*

Unify module.*

[Sam Ravnborg]

Following patchset unifies module.h and module.c.

The unification is done in a few smaller steps to make
it obvious what is happeing.

The first two patches unify module.h and prepare for
unification of module.c.

The third and fourth patch do all the preparations of module_64.c
without introducing any sparc32 specific code but all
sparc64 specific code is ifdef'ed.


I really did not like this part:
#ifdef CONFIG_SPARC64
                BUG_ON(((u64)location >> (u64)32) != (u64)0);
#endif /* CONFIG_SPARC64 */

I think it should be safe to do:

		BUG_ON(((unsigned long)location >> (unsigned long)32) !		       (unsigned long)0);

As to my best understanding unsigned long
is 32 bit on sparc32 and 64 bit on sparc64.
So we should with the above code not get any warnings.

But I did not really see the point of the BUG_ON in the first place,
and my limited digging did not turn up when it was added.

So to be safe I kept the ugly ifdef for now.
If is easy to spot/remove.

The final patch that unifies module.c adds one specific entry
to the case that is sparc32 specific:

                case R_SPARC_32:
+               case R_SPARC_UA32:

But this should not cause any problems.

The patch serie is build tested on a defconfig for
sparc and sparc64.

	Sam

Sam Ravnborg (5):
      sparc: introduce CONFIG_BITS
      sparc: unify module.h
      sparc64: use bit neutral Elf symbols
      sparc64: prepare module_64.c for unification
      sparc: unify module.c

 arch/sparc/Kconfig                          |    6 +-
 arch/sparc/include/asm/module.h             |   32 ++++--
 arch/sparc/include/asm/module_32.h          |    7 -
 arch/sparc/include/asm/module_64.h          |    7 -
 arch/sparc/kernel/Makefile                  |    2 +-
 arch/sparc/kernel/{module_64.c => module.c} |  112 +++++++++++++------
 arch/sparc/kernel/module_32.c               |  163 ---------------------------
 7 files changed, 107 insertions(+), 222 deletions(-)

Re: Unify module.*

[Sam Ravnborg]

> 
> I think it should be safe to do:
> 
> 		BUG_ON(((unsigned long)location >> (unsigned long)32) !> 		       (unsigned long)0);
> 
> As to my best understanding unsigned long
> is 32 bit on sparc32 and 64 bit on sparc64.
> So we should with the above code not get any warnings.
> 
> But I did not really see the point of the BUG_ON in the first place,
> and my limited digging did not turn up when it was added.
> 
> So to be safe I kept the ugly ifdef for now.
> If is easy to spot/remove.

And just tell me and I will respin the patch serie
with this or any other modifications requested.

Just to make sure this was explicitly stated.

	Sam

Re: Unify module.*

[Chris Torek]

>I really did not like this part:
>#ifdef CONFIG_SPARC64
>                BUG_ON(((u64)location >> (u64)32) != (u64)0);
>#endif /* CONFIG_SPARC64 */
>
>I think it should be safe to do:
>
>		BUG_ON(((unsigned long)location >> (unsigned long)32) !>		       (unsigned long)0);

No, you can't safely shift a 32-bit value by 32 bits (the compiler
is allowed to turn this into a shift of 0 bits for instance, by
using a 5-bit mask for the shift distance).  However it should be
safe to do this, which is exactly the same in practice:

		BUG_ON((((unsigned long)location >> 31) >> 1) !		       (unsigned long)0);

>But I did not really see the point of the BUG_ON in the first place,
>and my limited digging did not turn up when it was added.

Not sure who uses it and why it was added, but it's equivalent to:

	BUG_ON((unsigned long)location > 0xffffffff);

which I personally prefer to the shift version -- I think it makes
the intent more obvious -- but this might still draw a warning about
being always-false on 32-bit.  Of course, if gcc gets any smarter,
so might the shift version.

Chris