Hi,
Here's an updated patch as you requested.
Note that it implicitly fixes a small bug in the hash function that
was introduced when modifying the eina code: the line "hash ^= key[2]
<< 18;" is supposed to be accessing the 3rd byte of the remainder of
the input, but when 'it' was introduced, 'key' ('data' in eina code)
was no longer incremented, so this ended up accessing the 3rd byte of
the input from the beginning. This is fixed by iterating over 'key',
like the eina code does.
As far as implementation of get16bits() is concerned, the "attribute
packed" approach is the best; it avoids having to hardcode specific
platforms, letting the compiler deal with the details of memory
access. (and it works in every compiler this code is ever going to be
compiled with; e.g. gcc and clang both support this)
On Fri, Feb 3, 2012 at 5:33 PM, Lucas De Marchi
<lucas.demarchi@xxxxxxxxxxxxxx> wrote:
> Hi Ambroz,
>
> * Ambroz Bizjak <ambrop7@xxxxxxxxx> [2012-02-03 02:52:23 +0100]:
>
>> Hello.
>>
>> [ see bug report https://github.com/archlinuxarm/PKGBUILDs/issues/127 ]
>>
>> The depmod program from kmod fails to properly compute kernel module
>> dependencies on ARMv5 systems:
>>
>> [root@alarm ~]# modinfo bridge
>> filename: /lib/modules/2.6.39.4/kernel/net/bridge/bridge.ko
>> version: 2.3
>> license: GPL
>> srcversion: 6B583530AE2B39C7E2317BF
>> depends: stp,llc
>> vermagic: 2.6.39.4 preempt mod_unload ARMv5
>> [root@alarm ~]# depmod
>> [root@alarm ~]# cat /lib/modules/2.6.39.4/modules.dep |grep bridge
>> kernel/net/bridge/bridge.ko:
>> [root@alarm ~]#
>>
>> See how modinfo properly lists the dependencies, but modules.dep which
>> depmod generates does not contain them. As a result, most kernel
>> modules fail to load because their dependencies are not loaded by
>> modprobe.
>>
>> The cause is unaligned memory access in a hash function. Patch is
>> attacked, and linked to in the above link.
>>
>> Best regards,
>> Ambroz Bizjak
>
> Thanks for very descriptive bug report _and_ patch :-).
>
> This code came from Eina[1] so I was very surprised with your bug
> report since Eina works fine in ARM. However when Gustavo imported
> it, his changes broke unaligned accesses like you noticed.
>
> I'd prefer a solution more in line with eina's implementation, that
> is... creating a get16bits() or get_unaligned_short() macro that do the
> right thing and use it instead of assigning the value of the iterator.
>
> Could you prepare this patch?
>
> Thanks
> Lucas De Marchi
>
> [1] http://trac.enlightenment.org/e/browser/trunk/eina/src/lib/eina_hash.c
diff --git a/libkmod/libkmod-hash.c b/libkmod/libkmod-hash.c
index f58e9db..3a6c8bf 100644
--- a/libkmod/libkmod-hash.c
+++ b/libkmod/libkmod-hash.c
@@ -83,6 +83,15 @@ void hash_free(struct hash *hash)
free(hash);
}
+struct unaligned_short {
+ unsigned short v;
+} __attribute__((packed));
+
+static inline unsigned short get16bits(const char *ptr)
+{
+ return ((struct unaligned_short *)ptr)->v;
+}
+
static inline unsigned int hash_superfast(const char *key, unsigned int len)
{
/* Paul Hsieh (http://www.azillionmonkeys.com/qed/hash.html)
@@ -90,36 +99,35 @@ static inline unsigned int hash_superfast(const char *key, unsigned int len)
* EFL's eina and possible others.
*/
unsigned int tmp, hash = len, rem = len & 3;
- const unsigned short *itr = (const unsigned short *)key;
len /= 4;
/* Main loop */
for (; len > 0; len--) {
- hash += itr[0];
- tmp = (itr[1] << 11) ^ hash;
+ hash += get16bits(key);
+ tmp = (get16bits(key + 2) << 11) ^ hash;
hash = (hash << 16) ^ tmp;
- itr += 2;
+ key += 4;
hash += hash >> 11;
}
/* Handle end cases */
switch (rem) {
case 3:
- hash += *itr;
+ hash += get16bits(key);
hash ^= hash << 16;
hash ^= key[2] << 18;
hash += hash >> 11;
break;
case 2:
- hash += *itr;
+ hash += get16bits(key);
hash ^= hash << 11;
hash += hash >> 17;
break;
case 1:
- hash += *(const char *)itr;
+ hash += *key;
hash ^= hash << 10;
hash += hash >> 1;
}
