[clang-analyzer] fix warnings emitted on compiler-rt code base

[apelete]

Summary

The following warnings were reported while running clang analyzer on Compiler-RT:

Unix API: allocator sizeof operand mismatch, on file:

• lib/builtins/emutls.c.

This patch does not suppress the warning in fact, since sizeof operand
still mismatch, but shouldn't the alloc return value be casted for
correctness ?

Fixes T123 (please note that first revision was sent only to
llvm-commits mailing list, not reviewed yet).

Signed-off-by: Apelete Seketeli <[e-mail]>

[george.burgess.iv]

Hi, and thanks for the patch!

It looks like the static analyzer might think we're allocating an array of emutls_address_array here. If we want to make it be quiet, we can probably just swap to using sizeof(emutls_address_array), but I'm not sure if that's better than sizeof(void*), given that the former makes it look like we were actually trying to allocate an array of arrays (IMO).

but shouldn't the alloc return value be casted for correctness ?

C allows implicit conversions from void * to other pointer types, so I don't think an explicit cast here would cause the code to be more/less correct.

Either way, if this patch lands (I don't have an opinion on whether or not it should), please also add a cast to the malloc at line 53, so we have a consistent style within emutls.c. :)

[apelete]

In D19085#400579, @george.burgess.iv wrote:

It looks like the static analyzer might think we're allocating an array of emutls_address_array here. If we want to make it be quiet, we can probably just swap to using sizeof(emutls_address_array), but I'm not sure if that's better than sizeof(void*), given that the former makes it look like we were actually trying to allocate an array of arrays (IMO).

I refrained from swapping to use 'sizeof(emutls_address_array)' because the result would have been not correct to me.
Let's take the following lines:

164. emutls_address_array* array = pthread_getspecific(emutls_pthread_key);
165. if (array == NULL) {
166. uintptr_t new_size = emutls_new_data_array_size(index);
167. array = (emutls_address_array *) calloc(new_size + 1, sizeof(void*));
168. emutls_cmheck_array_set_size(array, new_size);
169. }

My understanding is that the code is currently allocating the 'void* data' array within emutls_address_array object by also taking into account the size of the extra member 'uintptr_t size' within emutls_address_array.
Which means we are allocating at once the needed space to store the value of 'array' since variable 'new_size' is assigned the total size of emutls_address_array.

Swapping to use 'sizeof(emutls_address_array)' would indeed allocate an array of arrays (ie. an array of emutls_address_array objects), which does not look like what the code is trying to do.

I will gladly correct if I'm wrong here, just let me know.

but shouldn't the alloc return value be casted for correctness ?

C allows implicit conversions from void * to other pointer types, so I don't think an explicit cast here would cause the code to be more/less correct.

Here I was thinking that implicit conversions were only allowed in C (but not in C++), and that this part of the code would be processed as C++ code.
I realise now that there is actually no C++ construct or feature in the whole file.

Just for my own information, since I'm new to compiler technology and LLVM, is this file processed as C or C++ code ?

Either way, if this patch lands (I don't have an opinion on whether or not it should), please also add a cast to the malloc at line 53, so we have a consistent style within emutls.c. :)

Ok, will do in next revision.

Thanks for your review.

[george.burgess.iv]

You're right that the code isn't trying to allocate an array of arrays, and I agree that sizeof(emutls_address_array) looks misleading. However, sizeof(emutls_address_array) should be equal to sizeof(void *), so the generated code should be identical.

That said, maybe we can placate the static analyzer if we make the new sizes new_size * sizeof(void*) + sizeof(emutls_address_array)? If that works, it's probably the cleanest solution.

Just for my own information, since I'm new to compiler technology and LLVM, is this file processed as C or C++ code ?

Given that it has a .c extension, I'd say it's C. If you'd like to check this in the future, just throw namespace {} in the file and recompile. If it compiles, the compiler thinks it's C++. :)

[apelete]

In D19085#401547, @george.burgess.iv wrote:

You're right that the code isn't trying to allocate an array of arrays, and I agree that sizeof(emutls_address_array) looks misleading. However, sizeof(emutls_address_array) should be equal to sizeof(void *), so the generated code should be identical.

I forgot 'void *' can indeed hold any data pointer. Generated code should indeed be identical, thanks for your patience :-).

That said, maybe we can placate the static analyzer if we make the new sizes new_size * sizeof(void*) + sizeof(emutls_address_array)? If that works, it's probably the cleanest solution.

It works, the static analyzer goes silent with this change.
New revision incoming.

Just for my own information, since I'm new to compiler technology and LLVM, is this file processed as C or C++ code ?

Given that it has a .c extension, I'd say it's C. If you'd like to check this in the future, just throw namespace {} in the file and recompile. If it compiles, the compiler thinks it's C++. :)

Thanks for the tip.

[apelete]

[clang-analyzer] fix warnings emitted on compiler-rt code base

Following changes were done in this revision:

• lib/builtins/emutls.c: expand calculation of new size of allocation to make use of actual type apparent.

[george.burgess.iv]

I forgot 'void *' can indeed hold any data pointer. Generated code should indeed be identical, thanks for your patience :-).

Not a problem!

It works, the static analyzer goes silent with this change.

Yay :)

Looks good to me after a few tiny changes. After it's updated, will you need me to check this in for you?

lib/builtins/emutls.c

167 ↗

(On Diff #53794)

Change sizeof(emutls_address_array *) to sizeof(emutls_address_array), please.

174 ↗

(On Diff #53794)

Same as above.

[apelete]

In D19085#401845, @george.burgess.iv wrote:

Looks good to me after a few tiny changes. After it's updated, will you need me to check this in for you?

Yes, please do so.

[apelete]

[clang-analyzer] fix warnings emitted on compiler-rt code base

Following changes were done in this revision:

• lib/builtins/emutls.c: fix computation to allocate extra slot to store data array size.

compiler-rt/trunk/lib/builtins/emutls.c

 /* Returns the thread local emutls_address_array.
  * Extends its size if necessary to hold address at index.
  */
 static __inline emutls_address_array *
 emutls_get_address_array(uintptr_t index) {
     emutls_address_array* array = pthread_getspecific(emutls_pthread_key);
     if (array == NULL) {
         uintptr_t new_size = emutls_new_data_array_size(index);
-        array = calloc(new_size + 1, sizeof(void*));
+        array = malloc(new_size * sizeof(void *) + sizeof(emutls_address_array));
+        if (array)
+            memset(array->data, 0, new_size * sizeof(void*));
         emutls_check_array_set_size(array, new_size);
     } else if (index > array->size) {
         uintptr_t orig_size = array->size;
         uintptr_t new_size = emutls_new_data_array_size(index);
-        array = realloc(array, (new_size + 1) * sizeof(void*));
+        array = realloc(array, new_size * sizeof(void *) + sizeof(emutls_address_array));
         if (array)
             memset(array->data + orig_size, 0,
                    (new_size - orig_size) * sizeof(void*));
         emutls_check_array_set_size(array, new_size);
     }
     return array;
 }
 
 void* __emutls_get_address(__emutls_control* control) {
     uintptr_t index = emutls_get_index(control);
     emutls_address_array* array = emutls_get_address_array(index);
     if (array->data[index - 1] == NULL)
         array->data[index - 1] = emutls_allocate_object(control);
     return array->data[index - 1];
 }