Hi Lianbo, sorry for the late response. On Sun, 3 Oct 2021 10:25:26 +0800 lijiang <lijiang@xxxxxxxxxx> wrote: > On Sat, Oct 2, 2021 at 12:10 AM Philipp Rudo <prudo@xxxxxxxxxx> wrote: > > > > Hi Tao, > > Hi Lianbo, > > > > sorry for the late response. > > > > On Wed, 29 Sep 2021 15:46:19 +0800 > > lijiang <lijiang@xxxxxxxxxx> wrote: > > > > > > > > Currently, the macro is used twice in the symbols.c. This change seems > > > > > > not complicated. Any thoughts? > > > > > > > > > > do I understand your suggestion correct, you propose to replace the > > > > > > > > > > #define SYMNAME_HASH_INDEX(name) ... > > > > > > > > > > in defs.h by something like > > > > > > > > > > static unsigned long long SYMNAME_HASH_INDEX(const unsigned char * const name) { > > > > > return (name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2]) % SYMNAME_HASH); > > > > > } > > > > > > > > > > in symbols.c? If so, I think that should be fine. > > > > > > > > Yes, you are right, Philipp. > > > > > > > > > > > Please correct me if I'm wrong. I don't think the function can work. > > > > Let's say name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2]) == > > > > -1, then we will have: > > > > > > > > static unsigned long long SYMNAME_HASH_INDEX(const unsigned char * const name) { > > > > return (-1) % 512; > > > > } > > > > > > > > The returned value is a very large number, and will overflow the array. > > > > @Tao: I don't think that should be possible here. In the function I > > have defined name as _unsigned_ char*. So the calculation should never > > become negative. But to be honest I haven't tested it. > > > > > No, this is a modulo operation, and its result will never exceed '512' anyway. > > > (unsigned long long)(-1) % 512 = 511 > > > > @Lianbo: I don't think that's true. When the modulo is used with an > > negative dividend the result becomes negative. When this then gets > > casted to an unsigend integer type the result will become very large. > > That's also what a quick test showed me > > > > printf("%lli\n", ((-1LL) % 512)); --> -1 > > printf("%llu\n", (unsigned long long) ((-1LL) % 512)); --> 18446744073709551615 > > > > Thank you for the reply, Tao and Philipp. Let's go back to the problem > itself and continue our discussion. > Does the following change work for you? > > --- > defs.h | 2 -- > symbols.c | 18 +++++++++++++++--- > 2 files changed, 15 insertions(+), 5 deletions(-) > > diff --git a/defs.h b/defs.h > index cbd45e52f9da..d7b9bcc89878 100644 > --- a/defs.h > +++ b/defs.h > @@ -2728,8 +2728,6 @@ struct downsized { > (((vaddr) >> machdep->pageshift) % SYMVAL_HASH) > > #define SYMNAME_HASH (512) > -#define SYMNAME_HASH_INDEX(name) \ > - ((name[0] ^ (name[strlen(name)-1] * name[strlen(name)/2])) % SYMNAME_HASH) > > #define PATCH_KERNEL_SYMBOLS_START ((char *)(1)) > #define PATCH_KERNEL_SYMBOLS_STOP ((char *)(2)) > diff --git a/symbols.c b/symbols.c > index 69dccdb09d5f..8a2230df7712 100644 > --- a/symbols.c > +++ b/symbols.c > @@ -1127,6 +1127,18 @@ symname_hash_init(void) > st->__per_cpu_end = sp->value; > } > > +unsigned int symname_hash_index(unsigned char *name) > +{ > + unsigned int len, value; > + > + len = strlen(name); > + if (!len) > + error(FATAL, "The length of the symbol name is zero!\n"); > + > + value = name[len-1] * name[len/2]; > + > + return (name[0] ^ value) % (unsigned int)SYMNAME_HASH; > +} > /* > * Install a single static kernel symbol into the symname_hash. > */ > @@ -1134,9 +1146,9 @@ static void > symname_hash_install(struct syment *spn) > { > struct syment *sp; > - int index; > + unsigned int index; > > - index = SYMNAME_HASH_INDEX(spn->name); > + index = symname_hash_index(spn->name); > spn->cnt = 1; > > if ((sp = st->symname_hash[index]) == NULL) > @@ -1165,7 +1177,7 @@ symname_hash_search(char *name) > { > struct syment *sp; > > - sp = st->symname_hash[SYMNAME_HASH_INDEX(name)]; > + sp = st->symname_hash[symname_hash_index(name)]; > > while (sp) { > if (STREQ(sp->name, name)) the patch looks good to me. Personally I would only update the definition of SYMNAME_HASH to be unsigned rather than doing the cast in symname_hash_index. But that's only a personal preference. Thanks Philipp -- Crash-utility mailing list Crash-utility@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/crash-utility