Re: CVS commit: src/external/bsd/file
On Fri, Jan 17, 2014 at 11:32:29PM +, David Laight wrote: If the problem actually caused by gcc failing to pair all the conditionals? Compiling with 'clazz' a compile-time constant might show things. I don't think so, and I'll leave a final fix to Christos as maintainer. Or, if memcpy() is defined in a header file and uses casts to optimise inlined copies of fixed sizes it might be that the pointer-aliasing rules mean that the actual structure might not have been written. The copy is of the full size of the relevant structure, but through a (void*) pointer - sounds more like a gcc bug to me. A possible solution to that is an asm statement with a memory constraint for the buffer areas either side of the actual copy. Since the warning is a false positive, I think we should not tamper with the source too much, besides shutting up the warning via a pragma or whatever (should work with push/pop, but even that is ugly). Christos? Martin
Re: CVS commit: src/external/bsd/file
On Sat, Jan 18, 2014 at 10:33 AM, Martin Husemann mar...@duskware.de wrote: On Fri, Jan 17, 2014 at 11:32:29PM +, David Laight wrote: If the problem actually caused by gcc failing to pair all the conditionals? Compiling with 'clazz' a compile-time constant might show things. I don't think so, and I'll leave a final fix to Christos as maintainer. Or, if memcpy() is defined in a header file and uses casts to optimise inlined copies of fixed sizes it might be that the pointer-aliasing rules mean that the actual structure might not have been written. The copy is of the full size of the relevant structure, but through a (void*) pointer - sounds more like a gcc bug to me. I believe gcc is correct here, you are only allowed to alias via a char pointer or the original type. Because you put a void pointer in instead gcc is right to complain. Justin
Re: CVS commit: src/external/bsd/file
On Sat, Jan 18, 2014 at 10:46:50AM +, Justin Cormack wrote: I believe gcc is correct here, you are only allowed to alias via a char pointer or the original type. Because you put a void pointer in instead gcc is right to complain. Right, but actually the complaint does not go away if we make the ? operator into an if () with two separate memcpy() calls without any casts. Martin
Re: CVS commit: src/external/bsd/file
On Jan 18, 11:33am, mar...@duskware.de (Martin Husemann) wrote: -- Subject: Re: CVS commit: src/external/bsd/file | On Fri, Jan 17, 2014 at 11:32:29PM +, David Laight wrote: | If the problem actually caused by gcc failing to pair all the conditionals? | Compiling with 'clazz' a compile-time constant might show things. | | I don't think so, and I'll leave a final fix to Christos as maintainer. | | Or, if memcpy() is defined in a header file and uses casts to optimise | inlined copies of fixed sizes it might be that the pointer-aliasing | rules mean that the actual structure might not have been written. | | The copy is of the full size of the relevant structure, but through a | (void*) pointer - sounds more like a gcc bug to me. | | A possible solution to that is an asm statement with a memory | constraint for the buffer areas either side of the actual copy. | | Since the warning is a false positive, I think we should not tamper with | the source too much, besides shutting up the warning via a pragma or whatever | (should work with push/pop, but even that is ugly). | | Christos? I don't know which memcpy is causing it in readelf.c... I'll try it. christos
Re: CVS commit: src/external/bsd/file
On Sat, Jan 18, 2014 at 11:53:09AM +0100, Martin Husemann wrote:
On Sat, Jan 18, 2014 at 10:46:50AM +, Justin Cormack wrote:
I believe gcc is correct here, you are only allowed to alias via a
char pointer or the original type. Because you put a void pointer in
instead gcc is right to complain.
You can also copy to 'void *' and then back to the original type.
In fact that is the only way you are allowed to use 'void *'.
Right, but actually the complaint does not go away if we make the ? operator
into an if () with two separate memcpy() calls without any casts.
I wouldn't expect it to.
There are two possible things gcc is complaining about:
1) It thinks you are accessing nh32 when only nh64 has been initialised
(or v.v.).
2) It doesn't think the memcpy() calls actually writes the structures.
The first can be eliminated by unconditionally compiling both memcpy() calls.
If gcc doesn't think the memcpy() is writing to the structures then
it might completely mis-optimise the code.
For instance: if memcpy() is (modulo typing bugs):
#define memcpy(d, s, len) \
if (__builtin_constant(len) (len) == 8) \
*(uint64_t *)(d) = *(uint64_t *)(s); \
else \
__memcpy(d, s, len) \
And you have:
struct { int32_t a; int32_t b } x, y = {1, 2};
then:
memcpy(x, y, 8);
printf(%u, x.a + x.b);
gcc can optimise away 'y' and the memcpy() completely, leaving 'x'
undefined.
While this might not affect the code in file, it might cause obscure
effects elsewhere.
The builtin memcpy() should have appropriate constraints to indicate
that it reads and writes the memory.
I think it should be possible the use asm statements that generate
comments but have m constraints (to a correctly sized char [])
top tail in the memcpy() expansion so that the old buffer is read
before the copy and the new one afterwards.
Maybe:
#define reference_memory(ptr, len) \
asm volatile ( :: m ( *(struct {char x[len]; } *)(ptr)) )
#define memcpy(d, s, len) \
if (__builtin_constant(len) (len) == 8) { \
reference_memory(s, 8); \
*(uint64_t *)(d) = *(uint64_t *)(s); \
reference_memory(d, 8); \
} else \
__memcpy(d, s, len) \
But my gnu asm is a bit weak!
David
--
David Laight: da...@l8s.co.uk
