[PATCH 23/28] elf: Unify old and new format cache handling code in ld.so

[Adhemerval Zanella]

On 30/10/2020 10:22, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>>> +/* Compute the address of the element INDEX of the array at LIBS.
>>> +   Conceptually, this is &LIBS[INDEX], but use ENTRY_SIZE for the size
>>> +   of *LIBS.  */
>>> +static inline const struct file_entry *
>>> +_dl_cache_file_entry (const struct file_entry *libs, size_t entry_size,
>>> +		      size_t index)
>>> +{
>>> +  return (const void *) libs + index * entry_size;
>>> +}
>>> +
>>
>> Although pointer arithmetic with void *is valid for gcc, I think we should 
>> use uintptr_t here.
> 
> Do you mean char *?  It will need another cast for the result.  I don't
> think it clarifies matters.

I don't have a strong opinion in fact, it just is might not be straightforward
for a reader not used with GNU C extensions.

> 
>>> +{
>>> +  /* Used by the HWCAP check in the struct file_entry_new case.  */
>>> +  uint64_t platform = _dl_string_platform (GLRO (dl_platform));
>>> +  if (platform != (uint64_t) -1)
>>> +    platform = 1ULL << platform;
>>
>> Maybe UINT64_C(1) << platform?
> 
> 1ULL needs to be at least 64 bits, so this should be fine.
> 
>>> +
>>> +		      best = string_table + lib->value;
>>> +
>>> +		      if (flags == GLRO (dl_correct_cache_id))
>>> +			/* We've found an exact match for the shared
>>> +			   object and no general `ELF' release.  Stop
>>
>> Should we replace the grave accent with apostrophe in this case?
> 
> I want to minimize the changes in this refactoring.

But git shows this as new lines anyway, so I see no good reason to
not replace it with an apostrophe.

> 
>>> diff --git a/sysdeps/generic/dl-cache.h b/sysdeps/generic/dl-cache.h
>>> index b154740da9..fec209509d 100644
>>> --- a/sysdeps/generic/dl-cache.h
>>> +++ b/sysdeps/generic/dl-cache.h
>>> @@ -66,8 +66,8 @@
>>>  */
>>>  struct file_entry
>>>  {
>>> -  int flags;		/* This is 1 for an ELF library.  */
>>> -  unsigned int key, value; /* String table indices.  */
>>> +  int32_t flags;		/* This is 1 for an ELF library.  */
>>> +  uint32_t key, value;		/* String table indices.  */
>>>  };
>>>  
>>
>> Ok, although not sure if our code guidelines allows multiple fields in the
>> same line.
> 
> Likewise.
> 
> I'll assume your Reviewed-by: stands and push this separately.

LGTM.