[Mesa-dev,3/3] glsl/cache/tests: Cast cache_get result to avoid compiler warning

Submitted by Aaron Watry on Nov. 22, 2016, 6:46 p.m.

Details

Message ID 1479840406-30300-3-git-send-email-awatry@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Aaron Watry Nov. 22, 2016, 6:46 p.m.
disk_cache_get returns void*, but we were storing/comparing a char*.

Signed-off-by: Aaron Watry <awatry@gmail.com>
---
Note that this did, and still, segfaults for me when I actually run it...

But at least the compiler is no longer complaining about the type mismatch.
 src/compiler/glsl/tests/cache_test.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/compiler/glsl/tests/cache_test.c b/src/compiler/glsl/tests/cache_test.c
index 0ef05aa..c663e54 100644
--- a/src/compiler/glsl/tests/cache_test.c
+++ b/src/compiler/glsl/tests/cache_test.c
@@ -208,14 +208,14 @@  test_put_and_get(void)
    _mesa_sha1_compute(blob, sizeof(blob), blob_key);
 
    /* Ensure that disk_cache_get returns nothing before anything is added. */
-   result = disk_cache_get(cache, blob_key, &size);
+   result = (char*) disk_cache_get(cache, blob_key, &size);
    expect_null(result, "disk_cache_get with non-existent item (pointer)");
    expect_equal(size, 0, "disk_cache_get with non-existent item (size)");
 
    /* Simple test of put and get. */
    disk_cache_put(cache, blob_key, blob, sizeof(blob));
 
-   result = disk_cache_get(cache, blob_key, &size);
+   result = (char*) disk_cache_get(cache, blob_key, &size);
    expect_equal_str(blob, result, "disk_cache_get of existing item (pointer)");
    expect_equal(size, sizeof(blob), "disk_cache_get of existing item (size)");
 
@@ -225,7 +225,7 @@  test_put_and_get(void)
    _mesa_sha1_compute(string, sizeof(string), string_key);
    disk_cache_put(cache, string_key, string, sizeof(string));
 
-   result = disk_cache_get(cache, string_key, &size);
+   result = (char*) disk_cache_get(cache, string_key, &size);
    expect_equal_str(result, string, "2nd disk_cache_get of existing item (pointer)");
    expect_equal(size, sizeof(string), "2nd disk_cache_get of existing item (size)");
 

Comments

On 11/22, Aaron Watry wrote:
>disk_cache_get returns void*, but we were storing/comparing a char*.
>
>Signed-off-by: Aaron Watry <awatry@gmail.com>
>---
>Note that this did, and still, segfaults for me when I actually run it...

Strange. It passes for me.

>
>But at least the compiler is no longer complaining about the type mismatch.
> src/compiler/glsl/tests/cache_test.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/src/compiler/glsl/tests/cache_test.c b/src/compiler/glsl/tests/cache_test.c
>index 0ef05aa..c663e54 100644
>--- a/src/compiler/glsl/tests/cache_test.c
>+++ b/src/compiler/glsl/tests/cache_test.c
>@@ -208,14 +208,14 @@ test_put_and_get(void)
>    _mesa_sha1_compute(blob, sizeof(blob), blob_key);
>
>    /* Ensure that disk_cache_get returns nothing before anything is added. */
>-   result = disk_cache_get(cache, blob_key, &size);
>+   result = (char*) disk_cache_get(cache, blob_key, &size);

void * is implicitly convertable to char *, so I think the code is fine
as is. Maybe if I saw the compiler warning.

"result" could be declared void * to begin with, and that would be fine.
I just tested it, and it seems to work.

Also, please use (char *) for casts and not (char*) in Mesa.
On Tue, Nov 22, 2016 at 2:48 PM, Matt Turner <mattst88@gmail.com> wrote:

> On 11/22, Aaron Watry wrote:
>
>> disk_cache_get returns void*, but we were storing/comparing a char*.
>>
>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>> ---
>> Note that this did, and still, segfaults for me when I actually run it...
>>
>
> Strange. It passes for me.
>

I guess that I should have clarified, this is also in the "building a
32-bit mesa on 64-bit host" scenario of my other two patches.

If I enable the shader cache on a 64-bit build, it works fine.

I'm not sure which scenario you're building for...

What I'm getting in my cross-compile scenario is:
/home/me/src/mesa/bin/test-driver: line 107: 10596 Segmentation fault
(core dumped) "$@" > $log_file 2>&1
FAIL: glsl/tests/cache-test

If I try to run the test directly, I get:
Error: Test 'disk_cache_create with no environment variables' failed:
Result=NULL, but expected something else.
Segmentation fault (core dumped)

Valgrind says:
==26330== Invalid read of size 4
==26330==    at 0x804A366: disk_cache_destroy (disk_cache.c:318)
==26330==    by 0x8048FA3: test_disk_cache_create (cache_test.c:143)
==26330==    by 0x8048FA3: main (cache_test.c:391)
==26330==  Address 0x8 is not stack'd, malloc'd or (recently) free'd

WIth that being said, I'm not seeing the warning that I thought I saw
before... I'm going to go back to the drawing board with this one and see
if I can address the segfault (looks like a failure to determine the user's
home directory for my environment, and I'm not sure why that's happening
for the 32-bit case).

The warnings that I thought I was getting before are no longer present, and
as you noted below should be allowed. It's possible I saw it when building
with clang instead of gcc, but I'm not sure anymore (I've been carrying
this patch locally for a while).

--Aaron




>
>
>> But at least the compiler is no longer complaining about the type
>> mismatch.
>> src/compiler/glsl/tests/cache_test.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/compiler/glsl/tests/cache_test.c
>> b/src/compiler/glsl/tests/cache_test.c
>> index 0ef05aa..c663e54 100644
>> --- a/src/compiler/glsl/tests/cache_test.c
>> +++ b/src/compiler/glsl/tests/cache_test.c
>> @@ -208,14 +208,14 @@ test_put_and_get(void)
>>    _mesa_sha1_compute(blob, sizeof(blob), blob_key);
>>
>>    /* Ensure that disk_cache_get returns nothing before anything is
>> added. */
>> -   result = disk_cache_get(cache, blob_key, &size);
>> +   result = (char*) disk_cache_get(cache, blob_key, &size);
>>
>
> void * is implicitly convertable to char *, so I think the code is fine
> as is. Maybe if I saw the compiler warning.
>
> "result" could be declared void * to begin with, and that would be fine.
> I just tested it, and it seems to work.
>
> Also, please use (char *) for casts and not (char*) in Mesa.
>

But, what I've also been seeing is sporadic warnings in the nir control
flow tests regarding signed/unsigned comparisons.
On Tue, Nov 22, 2016 at 8:27 PM, Aaron Watry <awatry@gmail.com> wrote:

>
>
> On Tue, Nov 22, 2016 at 2:48 PM, Matt Turner <mattst88@gmail.com> wrote:
>
>> On 11/22, Aaron Watry wrote:
>>
>>> disk_cache_get returns void*, but we were storing/comparing a char*.
>>>
>>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>>> ---
>>> Note that this did, and still, segfaults for me when I actually run it...
>>>
>>
>> Strange. It passes for me.
>>
>
A post-mortem in case someone else runs into it.  This ended up being a
local environment issue.

In stepping through the disk_cache.c code, I found that getpwuid_r is
failing to find my passwd entry. My current login account is coming from an
active directory server (and therefore doesn't actually have an entry in
/etc/passwd).

It worked fine on 64-bit, but failed on 32-bit.  A little searching led me
to discover that libnss-sss:i386 was missing. I installed that, and this
test is passing now.

Sorry for the noise, and the review on the other two patches.

--Aaron


>
>
>