Re: [PING] [PATCH, PR 57748] Check for out of bounds access, Part 2
On 11/28/13 03:24, Bernd Edlinger wrote:
Hi,
On Wed, 27 Nov 2013 12:07:16, Jeff Law wrote:
On 11/27/13 05:29, Bernd Edlinger wrote:
Hi,
ping...
this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html
Note: it does, as it is, _not_ depend on the keep_aligning patch.
And it would fix some really nasty wrong code generation issues.
Is there a testcase for this problem?
Yes,
the patch contains two test cases, one for
struct S { V a; V b[0]; } P __attribute__((aligned (1)))
and another for
struct S { V b[1]; } P __attribute__((aligned (1)))
V can be anything that has a movmisalign_optab or is SLOW_UNALIGNED_ACCESS
If V::b is used as flexible array, reading p-b[1] gives garbage.
We tried hard, to fix this in stor-layout.c by not using the mode of V
for struct S, but this created ABI-fallout. So currently the only possible
way to fix it seems to be in expansion, by letting expand_real_1 know that
we need a memory reference, even if it may be unaligned.
My recommendation is to start a separate thread which focuses on this
issue and only this issue.
If there are more questions of general interest, please feel free to start
in a new thread.
Well, my point is there's this thread that deals with multiple issues;
the message with the patch itself references prior versions of the patch
and sorting out any discussion that relates strictly to the outstanding
patch is nontrivial.
Hence my request to repost the patch as a new independent thread.
Include the testcase and a reference to any current bugzilla bugs.
jeff
RE: [PING] [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Wed, 27 Nov 2013 12:07:16, Jeff Law wrote:
On 11/27/13 05:29, Bernd Edlinger wrote:
Hi,
ping...
this patch still open:
http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html
Note: it does, as it is, _not_ depend on the keep_aligning patch.
And it would fix some really nasty wrong code generation issues.
Is there a testcase for this problem?
Yes,
the patch contains two test cases, one for
struct S { V a; V b[0]; } P __attribute__((aligned (1)))
and another for
struct S { V b[1]; } P __attribute__((aligned (1)))
V can be anything that has a movmisalign_optab or is SLOW_UNALIGNED_ACCESS
If V::b is used as flexible array, reading p-b[1] gives garbage.
We tried hard, to fix this in stor-layout.c by not using the mode of V
for struct S, but this created ABI-fallout. So currently the only possible
way to fix it seems to be in expansion, by letting expand_real_1 know that
we need a memory reference, even if it may be unaligned.
My recommendation is to start a separate thread which focuses on this
issue and only this issue.
If there are more questions of general interest, please feel free to start
in a new thread.
jeff
Thanks
Bernd.
[PING] [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
ping...
this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html
Note: it does, as it is, _not_ depend on the keep_aligning patch.
And it would fix some really nasty wrong code generation issues.
Thanks
Bernd.
Date: Tue, 19 Nov 2013 15:39:39 +0100
Hello,
this is a minor update to my previous version of this patch, (using a boolean
expand_reference,
instead of adding a new expand_modifier enum value):
I forgot to pass down the expand_reference value at the second expand_expr
call inside the
case VIEW_CONVERT_EXPR. Sorry for the inconvenience.
@@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
}
if (!op0)
- op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+ op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+ NULL, expand_reference);
/* If the input and output modes are both the same, we are done. */
if (mode == GET_MODE (op0))
Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
Ok for trunk?
Thanks
Bernd.
Date: Thu, 7 Nov 2013 13:58:55 +0100
oops - this time with attachments...
Hi,
On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere
with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard
to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is
special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there
it will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we
pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
I think you are right. After some thought, I start to like that idea.
This way we have at least much more flexibility, how to handle the inner
references correctly, and if I change only the interfaces of
expand_expr_real/real_1
that will not be used at too many places, either.
Finally I think the recursion into the VIEW_CONVERT_EXPR case
is only there because of the keep_aligning flag of get_inner_reference
which should be obsolete now that we properly handle its effects
in get_object_alignment. So you wouldn't need to adjust this path
if we finally can get rid of that.
I
Re: [PING] [PATCH, PR 57748] Check for out of bounds access, Part 2
On Wed, Nov 27, 2013 at 1:29 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
ping...
this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html
Note: it does, as it is, _not_ depend on the keep_aligning patch.
And it would fix some really nasty wrong code generation issues.
I'll come back to all these patches once the late features have all
hit stage3 (hopefully next week).
We have some time left to fix this and related bugs.
Thanks,
Richard.
Thanks
Bernd.
Date: Tue, 19 Nov 2013 15:39:39 +0100
Hello,
this is a minor update to my previous version of this patch, (using a
boolean expand_reference,
instead of adding a new expand_modifier enum value):
I forgot to pass down the expand_reference value at the second expand_expr
call inside the
case VIEW_CONVERT_EXPR. Sorry for the inconvenience.
@@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
}
if (!op0)
- op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+ op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+ NULL, expand_reference);
/* If the input and output modes are both the same, we are done. */
if (mode == GET_MODE (op0))
Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
Ok for trunk?
Thanks
Bernd.
Date: Thu, 7 Nov 2013 13:58:55 +0100
oops - this time with attachments...
Hi,
On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere
with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently
hard to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is
special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
case,
because it is only a problem for STRICT_ALIGNMENT targets, and even
there it will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we
pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
I think you are right. After some thought, I start to like that idea.
This way we have at least much more flexibility, how to handle the inner
references correctly, and if I change only the interfaces of
expand_expr_real/real_1
that will not be used at too many places, either.
Finally I think the recursion into the
Re: [PING] [PATCH, PR 57748] Check for out of bounds access, Part 2
On 11/27/13 05:29, Bernd Edlinger wrote: Hi, ping... this patch still open: http://gcc.gnu.org/ml/gcc-patches/2013-11/msg02291.html Note: it does, as it is, _not_ depend on the keep_aligning patch. And it would fix some really nasty wrong code generation issues. Is there a testcase for this problem? My recommendation is to start a separate thread which focuses on this issue and only this issue. jeff
[PATCH, PR 57748] Check for out of bounds access, Part 2
Hello,
this is a minor update to my previous version of this patch, (using a boolean
expand_reference,
instead of adding a new expand_modifier enum value):
I forgot to pass down the expand_reference value at the second expand_expr call
inside the
case VIEW_CONVERT_EXPR. Sorry for the inconvenience.
@@ -10219,7 +10229,8 @@ expand_expr_real_1 (tree exp, rtx target, enum mac
}
if (!op0)
- op0 = expand_expr (treeop0, NULL_RTX, VOIDmode, modifier);
+ op0 = expand_expr_real (treeop0, NULL_RTX, VOIDmode, modifier,
+ NULL, expand_reference);
/* If the input and output modes are both the same, we are done. */
if (mode == GET_MODE (op0))
Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
Ok for trunk?
Thanks
Bernd.
Date: Thu, 7 Nov 2013 13:58:55 +0100
oops - this time with attachments...
Hi,
On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard
to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there
it will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
I think you are right. After some thought, I start to like that idea.
This way we have at least much more flexibility, how to handle the inner
references correctly, and if I change only the interfaces of
expand_expr_real/real_1
that will not be used at too many places, either.
Finally I think the recursion into the VIEW_CONVERT_EXPR case
is only there because of the keep_aligning flag of get_inner_reference
which should be obsolete now that we properly handle its effects
in get_object_alignment. So you wouldn't need to adjust this path
if we finally can get rid of that.
I proposed a patch for that, but did not hear from you:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html
nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
reference, the code there should be kept in sync with
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there it
will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
I think you are right. After some thought, I start to like that idea.
This way we have at least much more flexibility, how to handle the inner
references correctly, and if I change only the interfaces of
expand_expr_real/real_1
that will not be used at too many places, either.
Finally I think the recursion into the VIEW_CONVERT_EXPR case
is only there because of the keep_aligning flag of get_inner_reference
which should be obsolete now that we properly handle its effects
in get_object_alignment. So you wouldn't need to adjust this path
if we finally can get rid of that.
I proposed a patch for that, but did not hear from you:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html
nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
reference, the code there should be kept in sync with the normal_inner_ref,
and MEM_REF, IMHO.
Attached, my updated patch for PR57748, Part 2.
Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
Ok for trunk?
Thanks
Bernd.
What do others think?
Thanks,
Richard.
Thanks
Bernd.
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
oops - this time with attachments...
Hi,
On Fri, 25 Oct 2013 12:51:13, Richard Biener wrote:
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there
it will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
I think you are right. After some thought, I start to like that idea.
This way we have at least much more flexibility, how to handle the inner
references correctly, and if I change only the interfaces of
expand_expr_real/real_1
that will not be used at too many places, either.
Finally I think the recursion into the VIEW_CONVERT_EXPR case
is only there because of the keep_aligning flag of get_inner_reference
which should be obsolete now that we properly handle its effects
in get_object_alignment. So you wouldn't need to adjust this path
if we finally can get rid of that.
I proposed a patch for that, but did not hear from you:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg02267.html
nevertheless, even if the VIEW_CONVERT_EXPR may or may not be an inner
reference, the code there should be kept in sync with the normal_inner_ref,
and MEM_REF, IMHO.
Attached, my updated patch for PR57748, Part 2.
Boot-strapped and regression-tested on X86_64-pc-linux-gnu.
Ok for trunk?
Thanks
Bernd.
What do others think?
Thanks,
Richard.
Thanks
Bernd.2013-11-07 Bernd Edlinger bernd.edlin...@hotmail.de
PR middle-end/57748
* expr.h (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference.
(expand_expr, expand_normal): Adjust.
* expr.c (expand_expr_real, expand_expr_real_1): Add new parameter
expand_reference. Use expand_reference to expand inner references.
(store_expr): Adjust.
* cfgexpand.c (expand_call_stmt): Adjust.
testsuite:
2013-11-07 Bernd Edlinger bernd.edlin...@hotmail.de
PR middle-end/57748
*
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi Richard,
Did you just propose:
--- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200
+++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200
@@ -471,27 +471,7 @@
static enum machine_mode
mode_for_array (tree elem_type, tree size)
{
- tree elem_size;
- unsigned HOST_WIDE_INT int_size, int_elem_size;
- bool limit_p;
-
- /* One-element arrays get the component type's mode. */
- elem_size = TYPE_SIZE (elem_type);
- if (simple_cst_equal (size, elem_size))
- return TYPE_MODE (elem_type);
-
- limit_p = true;
- if (host_integerp (size, 1) host_integerp (elem_size, 1))
- {
- int_size = tree_low_cst (size, 1);
- int_elem_size = tree_low_cst (elem_size, 1);
- if (int_elem_size 0
- int_size % int_elem_size == 0
- targetm.array_mode_supported_p (TYPE_MODE (elem_type),
- int_size / int_elem_size))
- limit_p = false;
- }
- return mode_for_size_tree (size, MODE_INT, limit_p);
+ return BLKmode;
}
???
Yes. Does it work?
Richard.
No.
PASS: gcc.target/i386/pr14289-1.c (test for errors, line 10)
FAIL: gcc.target/i386/pr14289-1.c (test for excess errors)
Excess errors:
/home/ed/gnu/gcc-4.9-20131013/gcc/testsuite/gcc.target/i386/pr14289-1.c:5:14:
error: data type of 'a' isn't suitable for a register
Does that look like an ABI-Change?
the test case uses:
register int a[2] asm(ebx);
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Fri, Oct 25, 2013 at 8:29 AM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi Richard,
Did you just propose:
--- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200
+++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200
@@ -471,27 +471,7 @@
static enum machine_mode
mode_for_array (tree elem_type, tree size)
{
- tree elem_size;
- unsigned HOST_WIDE_INT int_size, int_elem_size;
- bool limit_p;
-
- /* One-element arrays get the component type's mode. */
- elem_size = TYPE_SIZE (elem_type);
- if (simple_cst_equal (size, elem_size))
- return TYPE_MODE (elem_type);
-
- limit_p = true;
- if (host_integerp (size, 1) host_integerp (elem_size, 1))
- {
- int_size = tree_low_cst (size, 1);
- int_elem_size = tree_low_cst (elem_size, 1);
- if (int_elem_size 0
- int_size % int_elem_size == 0
- targetm.array_mode_supported_p (TYPE_MODE (elem_type),
- int_size / int_elem_size))
- limit_p = false;
- }
- return mode_for_size_tree (size, MODE_INT, limit_p);
+ return BLKmode;
}
???
Yes. Does it work?
Richard.
No.
PASS: gcc.target/i386/pr14289-1.c (test for errors, line 10)
FAIL: gcc.target/i386/pr14289-1.c (test for excess errors)
Excess errors:
/home/ed/gnu/gcc-4.9-20131013/gcc/testsuite/gcc.target/i386/pr14289-1.c:5:14:
error: data type of 'a' isn't suitable for a register
Does that look like an ABI-Change?
the test case uses:
register int a[2] asm(ebx);
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Bernd.
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there it
will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
Thanks
Bernd.2013-10-25 Bernd Edlinger bernd.edlin...@hotmail.de
PR middle-end/57748
* expr.h (expand_modifier): New enum value EXPAND_REFERENCE.
* expr.c (expand_expr_real_1): Use EXAND_REFERENCE on base object.
testsuite:
2013-10-25 Bernd Edlinger bernd.edlin...@hotmail.de
PR middle-end/57748
* gcc.dg/torture/pr57748-3.c: New test.
* gcc.dg/torture/pr57748-4.c: New test.
patch-pr57748-2.diff
Description: Binary data
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there it
will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
Finally I think the recursion into the VIEW_CONVERT_EXPR case
is only there because of the keep_aligning flag of get_inner_reference
which should be obsolete now that we properly handle its effects
in get_object_alignment. So you wouldn't need to adjust this path
if we finally can get rid of that.
What do others think?
Thanks,
Richard.
Thanks
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Fri, Oct 25, 2013 at 12:51:13PM +0200, Richard Biener wrote:
On Fri, Oct 25, 2013 at 12:02 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
Eh ... even
register struct { int i; int a[0]; } asm (ebx);
works. Also with int a[1] but not with a[2]. So just handling trailing
arrays makes this case regress as well.
Now I'd call this use questionable ... but we've likely supported that
for decades and cannot change that now.
Back to fixing everything in expand.
Richard.
Ok, finally you asked for it.
Here is my previous version of that patch again.
I have now added a new value EXPAND_REFERENCE to the expand_modifier
enumeration. It is almost like EXPAND_MEMORY but it does not interfere with
constant values.
I have done the same modification to VIEW_CONVERT_EXPR too, because
this is a possible inner reference, itself. It is however inherently hard to
test around this code.
To understand this patch it is good to know what type of object the
return value tem of get_inner_reference can be.
From the program logic at get_inner_reference it is clear that the
return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF,
ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may
be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably
further restricted because exp is gimplified.
Usually the result will be a MEM_REF or a SSA_NAME of the memory where
the structure is to be found.
When you look at where EXPAND_MEMORY is handled you see it is special-cased
in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF,
ARRAY_RANGE_REF.
At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the
same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given:
If it is an unaligned memory, we just return the unaligned reference.
This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test
case,
because it is only a problem for STRICT_ALIGNMENT targets, and even there
it will
certainly be really hard to find test cases that exercise this code.
In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF
we do not have to touch the handling of the outer modifier. However we pass
EXPAND_REFERENCE to the inner object, which should not be a recursive
use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF.
TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle
EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL.
Boot-strapped and regression-tested on x86_64-linux-gnu
OK for trunk?
You point to a weak spot in expansion - that it handles creating
the base MEM to offset with handled components by recursing
into the case that handles bare MEM_REFs. This makes the
bare MEM_REF handling code somewhat awkward (it's the
one to assign mem-attrs which are later adjusted for example).
Maybe a better appropach than adding yet another expand
modifier would be to split out the base MEM expansion part
out of the bare MEM_REF handling code so we can call that
instead of recursing.
I think that we should seize every easy opportunity to break up
expand_expr* functions into smaller ones with nicer names and easier
to understand semantics. So I think is a good idea.
Thanks,
Martin
In this light - instead of a new expand modifier don't you want
an actual flag that specifies we are coming from a call that
wants to expand a base? That is, allow EXPAND_SUM
but with the recursion flag set?
Finally I think the recursion into the VIEW_CONVERT_EXPR case
is only there because of the keep_aligning flag of get_inner_reference
which should be obsolete now that we properly handle its effects
in get_object_alignment. So you wouldn't need to adjust this path
if we finally can get rid of that.
What do others think?
Thanks,
Richard.
Thanks
Bernd.
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi Martin,
On Wed, 23 Oct 2013 19:11:06, Martin Jambor wrote:
On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote:
...
? And why should the same issue not exist for
struct { V a[1]; }
indeed, it does and it's simple to trigger (on x86_64):
/* { dg-do run } */
#include stdlib.h
typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
#if 1
typedef struct S {V b[1]; } P __attribute__((aligned (1)));
struct __attribute__((packed)) T { char c; P s; };
#else
typedef struct S {V b[1]; } P;
struct T { char c; P s; };
#endif
void __attribute__((noinline, noclone))
good_value (V b)
{
if (b[0] != 3 || b[1] != 4)
__builtin_abort ();
}
void __attribute__((noinline, noclone))
check (P *p)
{
good_value (p-b[1]);
}
int
main ()
{
struct T *t = (struct T *) calloc (128, 1);
V a = { 3, 4 };
t-s.b[1] = a;
check (t-s);
free (t);
return 0;
}
While I was willing to discount the zero sized array as an
insufficiently specified oddity, this seems to be much more serious
and potentially common. It seems we really need to be able to detect
these out-of-bounds situations and tell lower levels of expander that
whatever mode you think you are expanding, it is actually BLK mode.
It's uglier than I thought.
Martin
Deja-vu...
Thanks for this test case. This definitely destroys the idea to fix this in
stor-layout.c
I think that is common practice to write array[1]; at the end of the structure,
and use it as a flexible array. The compute_record_mode has no way to
decide if that is going to be a flexible array or not.
Sorry Eric, but now I have no other choice than to go back to my previous
version
of part 2:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html
I'd just add Martin's new test case as 57748-4.c.
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
I think that is common practice to write array[1]; at the end of the structure, and use it as a flexible array. The compute_record_mode has no way to decide if that is going to be a flexible array or not. Sorry Eric, but now I have no other choice than to go back to my previous version of part 2: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html Why? Just set BLKmode in this case as well. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
I think that is common practice to write array[1]; at the end of the structure, and use it as a flexible array. The compute_record_mode has no way to decide if that is going to be a flexible array or not. Sorry Eric, but now I have no other choice than to go back to my previous version of part 2: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html Why? Just set BLKmode in this case as well. Because of the ABI-change? -- Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Because of the ABI-change? While concerns about accidental ABI changes are real, they shouldn't be overstated either. We have been saying for longer than a decade that the back-ends must not use the type mode to implement calling conventions and other external interfaces. -- Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou ebotca...@adacore.com wrote: I think that is common practice to write array[1]; at the end of the structure, and use it as a flexible array. The compute_record_mode has no way to decide if that is going to be a flexible array or not. Sorry Eric, but now I have no other choice than to go back to my previous version of part 2: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html Why? Just set BLKmode in this case as well. Works for me if it works ABI-wise (which you say it should unless the backend is buggy). Note that means mode_for_array should unconditionally return BLKmode. Richard. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote:
On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou ebotca...@adacore.com wrote:
I think that is common practice to write array[1]; at the end of the
structure, and use it as a flexible array. The compute_record_mode has no
way to decide if that is going to be a flexible array or not.
Sorry Eric, but now I have no other choice than to go back to my previous
version of part 2:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html
Why? Just set BLKmode in this case as well.
Works for me if it works ABI-wise (which you say it should unless the
backend is buggy). Note that means mode_for_array should unconditionally
return BLKmode.
Did you just propose:
--- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200
+++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200
@@ -471,27 +471,7 @@
static enum machine_mode
mode_for_array (tree elem_type, tree size)
{
- tree elem_size;
- unsigned HOST_WIDE_INT int_size, int_elem_size;
- bool limit_p;
-
- /* One-element arrays get the component type's mode. */
- elem_size = TYPE_SIZE (elem_type);
- if (simple_cst_equal (size, elem_size))
- return TYPE_MODE (elem_type);
-
- limit_p = true;
- if (host_integerp (size, 1) host_integerp (elem_size, 1))
- {
- int_size = tree_low_cst (size, 1);
- int_elem_size = tree_low_cst (elem_size, 1);
- if (int_elem_size 0
- int_size % int_elem_size == 0
- targetm.array_mode_supported_p (TYPE_MODE (elem_type),
- int_size / int_elem_size))
- limit_p = false;
- }
- return mode_for_size_tree (size, MODE_INT, limit_p);
+ return BLKmode;
}
???
Richard.
--
Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Thu, Oct 24, 2013 at 12:22 PM, Richard Biener
richard.guent...@gmail.com wrote:
On Thu, Oct 24, 2013 at 9:15 AM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi Martin,
On Wed, 23 Oct 2013 19:11:06, Martin Jambor wrote:
On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote:
...
? And why should the same issue not exist for
struct { V a[1]; }
indeed, it does and it's simple to trigger (on x86_64):
/* { dg-do run } */
#include stdlib.h
typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
#if 1
typedef struct S {V b[1]; } P __attribute__((aligned (1)));
struct __attribute__((packed)) T { char c; P s; };
#else
typedef struct S {V b[1]; } P;
struct T { char c; P s; };
#endif
void __attribute__((noinline, noclone))
good_value (V b)
{
if (b[0] != 3 || b[1] != 4)
__builtin_abort ();
}
void __attribute__((noinline, noclone))
check (P *p)
{
good_value (p-b[1]);
}
int
main ()
{
struct T *t = (struct T *) calloc (128, 1);
V a = { 3, 4 };
t-s.b[1] = a;
check (t-s);
free (t);
return 0;
}
While I was willing to discount the zero sized array as an
insufficiently specified oddity, this seems to be much more serious
and potentially common. It seems we really need to be able to detect
these out-of-bounds situations and tell lower levels of expander that
whatever mode you think you are expanding, it is actually BLK mode.
It's uglier than I thought.
Martin
Deja-vu...
Thanks for this test case. This definitely destroys the idea to fix this in
stor-layout.c
I think that is common practice to write array[1]; at the end of the
structure,
and use it as a flexible array.
Same for stuff like
struct X { char c[4]; };
that currently gets SImode. In alias handling we treat all trailing
arrays as flexible, even if they happen to be nested in sub-structs like for
struct X { int i; struct Y { char c[4]; } };
too much code out in the wild to disallow this.
The compute_record_mode has no way to
decide if that is going to be a flexible array or not.
But it could assign BLKmode to _all_ array types.
And if it is to prevent ICEs then even struct Y { char c[1]; char c2; }
which gets HImode might be used as ((struct Y *)p)-c[1] - invalid
but still shouldn't ICE in any way.
Richard.
Richard.
Sorry Eric, but now I have no other choice than to go back to my previous
version
of part 2:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html
I'd just add Martin's new test case as 57748-4.c.
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Thu, Oct 24, 2013 at 2:55 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote:
On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou ebotca...@adacore.com
wrote:
I think that is common practice to write array[1]; at the end of the
structure, and use it as a flexible array. The compute_record_mode has no
way to decide if that is going to be a flexible array or not.
Sorry Eric, but now I have no other choice than to go back to my previous
version of part 2:
http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html
Why? Just set BLKmode in this case as well.
Works for me if it works ABI-wise (which you say it should unless the
backend is buggy). Note that means mode_for_array should unconditionally
return BLKmode.
Did you just propose:
--- stor-layout.c.orig2013-10-22 10:46:49.233261818 +0200
+++ stor-layout.c2013-10-24 14:54:00.425259062 +0200
@@ -471,27 +471,7 @@
static enum machine_mode
mode_for_array (tree elem_type, tree size)
{
- tree elem_size;
- unsigned HOST_WIDE_INT int_size, int_elem_size;
- bool limit_p;
-
- /* One-element arrays get the component type's mode. */
- elem_size = TYPE_SIZE (elem_type);
- if (simple_cst_equal (size, elem_size))
-return TYPE_MODE (elem_type);
-
- limit_p = true;
- if (host_integerp (size, 1) host_integerp (elem_size, 1))
-{
- int_size = tree_low_cst (size, 1);
- int_elem_size = tree_low_cst (elem_size, 1);
- if (int_elem_size 0
- int_size % int_elem_size == 0
- targetm.array_mode_supported_p (TYPE_MODE (elem_type),
- int_size / int_elem_size))
-limit_p = false;
-}
- return mode_for_size_tree (size, MODE_INT, limit_p);
+ return BLKmode;
}
???
Yes. Does it work?
Richard.
Richard.
--
Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Thu, Oct 24, 2013 at 02:55:59PM +0200, Bernd Edlinger wrote: On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote: On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou ebotca...@adacore.com wrote: I think that is common practice to write array[1]; at the end of the structure, and use it as a flexible array. The compute_record_mode has no way to decide if that is going to be a flexible array or not. Sorry Eric, but now I have no other choice than to go back to my previous version of part 2: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html Why? Just set BLKmode in this case as well. Works for me if it works ABI-wise (which you say it should unless the backend is buggy). Note that means mode_for_array should unconditionally return BLKmode. Did you just propose: But why would we want to penalize the generated code for this? I mean, if some structure has a flexible array member or something similar to it that we allow to be used as one, if we pass it as arguments, return from functions etc., it will not have any bits beyond those and thus it should be just fine to be passed in registers etc. We can only use those extra bits if either we allocate them on the heap/stack (malloc, alloca, ...) or as a global or automatic variable with initializer that fills in the zero sized array or flexible array member (GNU extensions?), but in either of these cases the DECL_MODE or TYPE_MODE should be irrelevant. So to me this really sounds like a bug in the expansion, certainly not stor-layout. Jakub
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Did you just propose:
--- stor-layout.c.orig 2013-10-22 10:46:49.233261818 +0200
+++ stor-layout.c 2013-10-24 14:54:00.425259062 +0200
@@ -471,27 +471,7 @@
static enum machine_mode
mode_for_array (tree elem_type, tree size)
{
- tree elem_size;
- unsigned HOST_WIDE_INT int_size, int_elem_size;
- bool limit_p;
-
- /* One-element arrays get the component type's mode. */
- elem_size = TYPE_SIZE (elem_type);
- if (simple_cst_equal (size, elem_size))
- return TYPE_MODE (elem_type);
-
- limit_p = true;
- if (host_integerp (size, 1) host_integerp (elem_size, 1))
- {
- int_size = tree_low_cst (size, 1);
- int_elem_size = tree_low_cst (elem_size, 1);
- if (int_elem_size 0
- int_size % int_elem_size == 0
- targetm.array_mode_supported_p (TYPE_MODE (elem_type),
- int_size / int_elem_size))
- limit_p = false;
- }
- return mode_for_size_tree (size, MODE_INT, limit_p);
+ return BLKmode;
}
???
Yes. Does it work?
Richard.
I will give it a try.
I could also explicitly catch the case struct { a[x]; };
in compute_record_mode. That might work too.
But one way or the other that fix will be ugly.
Note:
struct Y { char c[1]; char c2; };
is invalid even if the expander would not fail,
because the code in tree-ssa-alias.c will not see the alias,
between c[2] and c2, that works only for the last array.
So that is no ICE but all generated code at -O1 and higher
will be wrong.
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Thu, Oct 24, 2013 at 3:13 PM, Jakub Jelinek ja...@redhat.com wrote: On Thu, Oct 24, 2013 at 02:55:59PM +0200, Bernd Edlinger wrote: On Thu, 24 Oct 2013 12:18:43, Richard Biener wrote: On Thu, Oct 24, 2013 at 10:48 AM, Eric Botcazou ebotca...@adacore.com wrote: I think that is common practice to write array[1]; at the end of the structure, and use it as a flexible array. The compute_record_mode has no way to decide if that is going to be a flexible array or not. Sorry Eric, but now I have no other choice than to go back to my previous version of part 2: http://gcc.gnu.org/ml/gcc-patches/2013-10/msg00222.html Why? Just set BLKmode in this case as well. Works for me if it works ABI-wise (which you say it should unless the backend is buggy). Note that means mode_for_array should unconditionally return BLKmode. Did you just propose: But why would we want to penalize the generated code for this? I mean, if some structure has a flexible array member or something similar to it that we allow to be used as one, if we pass it as arguments, return from functions etc., it will not have any bits beyond those and thus it should be just fine to be passed in registers etc. We can only use those extra bits if either we allocate them on the heap/stack (malloc, alloca, ...) or as a global or automatic variable with initializer that fills in the zero sized array or flexible array member (GNU extensions?), but in either of these cases the DECL_MODE or TYPE_MODE should be irrelevant. So to me this really sounds like a bug in the expansion, certainly not stor-layout. Sure, that was what I was saying all along ... but still, if we want to fix it in stor-layout.c then we have to fix it for all cases there, not just the zero-size array case (which I showed is not the only relevant case). Richard. Jakub
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Tue, Oct 22, 2013 at 12:25 PM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
On Tue, 8 Oct 2013 22:50:21, Eric Botcazou wrote:
I agree, that assigning a non-BLKmode to structures with zero-sized arrays
should be considered a bug.
Fine, then let's apply Martin's patch, on mainline at least.
That would definitely be a good move. Maybe someone should approve it?
But this testcase is invalid on STRICT_ALIGNMENT platforms: xx is pointer
to a
type with 4-byte alignment so its value must be a multiple of 4.
Then you probably win. But I still have some doubts.
I had to use this silly alignment/pack(4) to circumvent this statement
in compute_record_mode:
/* If structure's known alignment is less than what the scalar
mode would need, and it matters, then stick with BLKmode. */
if (TYPE_MODE (type) != BLKmode
STRICT_ALIGNMENT
! (TYPE_ALIGN (type)= BIGGEST_ALIGNMENT
|| TYPE_ALIGN (type)= GET_MODE_ALIGNMENT (TYPE_MODE (type
{
/* If this is the only reason this type is BLKmode, then
don't force containing types to be BLKmode. */
TYPE_NO_FORCE_BLK (type) = 1;
SET_TYPE_MODE (type, BLKmode);
}
But there are at least two targets where STRICT_ALIGNMENT = 0
and SLOW_UNALIGNED_ACCESS != 0: rs6000 and alpha.
This example with a byte-aligned structure will on one of these targets
likely execute this code path in expand_expr_real_1/case MEM_REF:
else if (SLOW_UNALIGNED_ACCESS (mode, align))
temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
0, TYPE_UNSIGNED (TREE_TYPE (exp)),
(modifier == EXPAND_STACK_PARM
? NULL_RTX : target),
mode, mode);
This looks wrong, but unfortunately I cannot test on these targets...
Hmm, well,
the condition that would be necessary to execute that code path
would be
STRICT_ALIGNMENT = 0
and SLOW_UNALIGNED_ACCESS != 0 for any integer mode.
The only target that is close to hit this bug is rs6000:
#define STRICT_ALIGNMENT 0
#define SLOW_UNALIGNED_ACCESS(MODE, ALIGN) \
(STRICT_ALIGNMENT \
|| (((MODE) == SFmode || (MODE) == DFmode || (MODE) == TFmode\
|| (MODE) == SDmode || (MODE) == DDmode || (MODE) == TDmode)\
(ALIGN) 32) \
|| (VECTOR_MODE_P ((MODE)) (((int)(ALIGN)) VECTOR_ALIGN (MODE
but, luckily this is 0 for all integer modes.
So I am now convinced, there won't be any valid example with unions that
executes this code path.
Therefore I updated Martin's previous patch, to meet Eric's request:
That is to only handle zero-sized arrays at the end of the structure.
That looks backwards. Why should
struct { V i; V j[0]; }
have a different mode than
struct { V j[0]; V i; }
? And why should the same issue not exist for
struct { V a[1]; }
which is also flexible enough that we support accesses beyond it
via a pointer? That struct also has V2DImode. And this is all
because
/* If this field is the whole struct, remember its mode so
that, say, we can put a double in a class into a DF
register instead of forcing it to live in the stack. */
if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
mode = DECL_MODE (field);
which is IMHO ok.
Boot-strapped and regression-tested on x86_64-linux-gnu.
Ok for trunk?
Well, I'm not so sure. And if so then I'd prefer martins original
patch, rejecting all zero-sized fields. But then only if you
consider it a real field.
Of course I blame those that added the zero-sized array extension
for all this mess ... maybe we can reduce it by rejecting
zero-sized arrays that are not at the end of a structure - which means
we can demote them to flexible arrays with a NULL TYPE_SIZE
which would completely side-step this issue in stor-layout.c.
Richard.
Regards
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Wed, Oct 23, 2013 at 06:00:43PM +0200, Richard Biener wrote:
...
? And why should the same issue not exist for
struct { V a[1]; }
indeed, it does and it's simple to trigger (on x86_64):
/* { dg-do run } */
#include stdlib.h
typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
#if 1
typedef struct S {V b[1]; } P __attribute__((aligned (1)));
struct __attribute__((packed)) T { char c; P s; };
#else
typedef struct S {V b[1]; } P;
struct T { char c; P s; };
#endif
void __attribute__((noinline, noclone))
good_value (V b)
{
if (b[0] != 3 || b[1] != 4)
__builtin_abort ();
}
void __attribute__((noinline, noclone))
check (P *p)
{
good_value (p-b[1]);
}
int
main ()
{
struct T *t = (struct T *) calloc (128, 1);
V a = { 3, 4 };
t-s.b[1] = a;
check (t-s);
free (t);
return 0;
}
While I was willing to discount the zero sized array as an
insufficiently specified oddity, this seems to be much more serious
and potentially common. It seems we really need to be able to detect
these out-of-bounds situations and tell lower levels of expander that
whatever mode you think you are expanding, it is actually BLK mode.
It's uglier than I thought.
Martin
which is also flexible enough that we support accesses beyond it
via a pointer? That struct also has V2DImode. And this is all
because
/* If this field is the whole struct, remember its mode so
that, say, we can put a double in a class into a DF
register instead of forcing it to live in the stack. */
if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
mode = DECL_MODE (field);
which is IMHO ok.
Boot-strapped and regression-tested on x86_64-linux-gnu.
Ok for trunk?
Well, I'm not so sure. And if so then I'd prefer martins original
patch, rejecting all zero-sized fields. But then only if you
consider it a real field.
Of course I blame those that added the zero-sized array extension
for all this mess ... maybe we can reduce it by rejecting
zero-sized arrays that are not at the end of a structure - which means
we can demote them to flexible arrays with a NULL TYPE_SIZE
which would completely side-step this issue in stor-layout.c.
Richard.
Regards
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
That looks backwards. Why should
struct { V i; V j[0]; }
have a different mode than
struct { V j[0]; V i; }
?
Because we support out-of-bounds access for the array in the former case and
not in the latter case.
And why should the same issue not exist for
struct { V a[1]; }
which is also flexible enough that we support accesses beyond it
via a pointer? That struct also has V2DImode. And this is all
because
/* If this field is the whole struct, remember its mode so
that, say, we can put a double in a class into a DF
register instead of forcing it to live in the stack. */
if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)))
mode = DECL_MODE (field);
which is IMHO ok.
It's OK *only* if TYPE_SIZE (type) is really the size of all the objects of
the type; if it isn't, i.e. if we allow access past TYPE_SIZE (type), then we
cannot use the field's mode. So we need to decide where to draw the line.
--
Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
While I was willing to discount the zero sized array as an insufficiently specified oddity, this seems to be much more serious and potentially common. It seems we really need to be able to detect these out-of-bounds situations and tell lower levels of expander that whatever mode you think you are expanding, it is actually BLK mode. Please let's not enter this hazardous business. We just need to draw a line somewhere and clearly state what we support in terms of out-of-bounds access. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Tue, 8 Oct 2013 22:50:21, Eric Botcazou wrote:
I agree, that assigning a non-BLKmode to structures with zero-sized arrays
should be considered a bug.
Fine, then let's apply Martin's patch, on mainline at least.
That would definitely be a good move. Maybe someone should approve it?
But this testcase is invalid on STRICT_ALIGNMENT platforms: xx is pointer to
a
type with 4-byte alignment so its value must be a multiple of 4.
Then you probably win. But I still have some doubts.
I had to use this silly alignment/pack(4) to circumvent this statement
in compute_record_mode:
/* If structure's known alignment is less than what the scalar
mode would need, and it matters, then stick with BLKmode. */
if (TYPE_MODE (type) != BLKmode
STRICT_ALIGNMENT
! (TYPE_ALIGN (type)= BIGGEST_ALIGNMENT
|| TYPE_ALIGN (type)= GET_MODE_ALIGNMENT (TYPE_MODE (type
{
/* If this is the only reason this type is BLKmode, then
don't force containing types to be BLKmode. */
TYPE_NO_FORCE_BLK (type) = 1;
SET_TYPE_MODE (type, BLKmode);
}
But there are at least two targets where STRICT_ALIGNMENT = 0
and SLOW_UNALIGNED_ACCESS != 0: rs6000 and alpha.
This example with a byte-aligned structure will on one of these targets
likely execute this code path in expand_expr_real_1/case MEM_REF:
else if (SLOW_UNALIGNED_ACCESS (mode, align))
temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
0, TYPE_UNSIGNED (TREE_TYPE (exp)),
(modifier == EXPAND_STACK_PARM
? NULL_RTX : target),
mode, mode);
This looks wrong, but unfortunately I cannot test on these targets...
Hmm, well,
the condition that would be necessary to execute that code path
would be
STRICT_ALIGNMENT = 0
and SLOW_UNALIGNED_ACCESS != 0 for any integer mode.
The only target that is close to hit this bug is rs6000:
#define STRICT_ALIGNMENT 0
#define SLOW_UNALIGNED_ACCESS(MODE, ALIGN) \
(STRICT_ALIGNMENT \
|| (((MODE) == SFmode || (MODE) == DFmode || (MODE) == TFmode \
|| (MODE) == SDmode || (MODE) == DDmode || (MODE) == TDmode) \
(ALIGN) 32) \
|| (VECTOR_MODE_P ((MODE)) (((int)(ALIGN)) VECTOR_ALIGN (MODE
but, luckily this is 0 for all integer modes.
So I am now convinced, there won't be any valid example with unions that
executes this code path.
Therefore I updated Martin's previous patch, to meet Eric's request:
That is to only handle zero-sized arrays at the end of the structure.
Boot-strapped and regression-tested on x86_64-linux-gnu.
Ok for trunk?
Regards
Bernd.2013-10-22 Martin Jambor mjam...@suse.cz
Bernd Edlinger bernd.edlin...@hotmail.de
PR middle-end/57748
* stor-layout.c (compute_record_mode): Treat trailing zero-sized array
fields like incomplete types.
testsuite:
2013-10-22 Bernd Edlinger bernd.edlin...@hotmail.de
PR middle-end/57748
* gcc.dg/torture/pr57748-3.c: New test.
patch-pr57748-2.diff
Description: Binary data
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Would you agree that this error: flexible array member should also be emitted for a zero-sized array member, maybe as error: zero-sized array member not at end of struct? I would have answered yes when zero-sized arrays where introduced, but it's far less clear a couple of decades later IMO. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi Eric,
Would you agree that this error: flexible array member
should also be emitted for a zero-sized array member,
maybe as error: zero-sized array member not at end of struct?
I would have answered yes when zero-sized arrays where introduced, but it's
far less clear a couple of decades later IMO.
But if zero-sized arrays everywhere in a structure is valid C,
then the attached test case is a valid test case.
And it will break your patch for PR58570 at -O1 and above,
because you can no longer assume that different array members
are not an alias.
Regards
Bernd.int printf (const char *, ...);
struct S
{
char a[0];
short b[1];
};
int e = 1, i;
static struct S d;
int
main ()
{
if (e)
{
d.b[i]=!d.b[i]; //0-1
d.a[i]=!d.a[i]; //1-0
d.b[i]=!d.b[i]; //0-1
}
printf (%d\n, d.b[0]);
return 0;
}
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
But if zero-sized arrays everywhere in a structure is valid C, then the attached test case is a valid test case. Not necessarily, you can write the declaration but you cannot index the array, i.e. this is undefined behavior. And there is nothing new, distinct fields have been disambiguated in alias.c for ages. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
That would definitely be a good move. Maybe someone should approve it?
Yes, but I agree that we ought to restrict it to trailing zero-sized arrays.
That would help to allay Jakub's concerns about possible ABI fallouts.
--
Eric Botcazou
Other uses of zero-sized arrays in structures and unions are
at least questionable, and should be rejected.
While this construct produces an error:
struct s
{
int a[];
float b;
};
error: flexible array member not at end of struct
This does not even produce a waning:
struct s
{
int a[0];
float b;
};
Would you agree that this error: flexible array member
should also be emitted for a zero-sized array member,
maybe as error: zero-sized array member not at end of struct?
Regards
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
That would definitely be a good move. Maybe someone should approve it? Yes, but I agree that we ought to restrict it to trailing zero-sized arrays. That would help to allay Jakub's concerns about possible ABI fallouts. -- Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
OK, what do you think of it now? My take on this is that the Proper Fix(tm) has been posted by Martin: http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00082.html IMO it's a no-brainer, modulo the ABI concern. Everything else is more or less clever stuff to paper over this original compute_record_mode bug. We are lying to the expander by pretending that the object has V2DImode since it's larger and thus cannot be manipulated in this mode; everything is clearly downhill from there. If we don't want to properly fix the bug then let's put a hack in the expander, possibly using EXPAND_MEMORY, but it should trigger _only_ for structures with zero-sized arrays and non-BLKmode and be preceded by a big ??? comment explaining why it is deemed necessary. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Tue, 8 Oct 2013 10:01:37, Eric Botcazou wrote:
OK, what do you think of it now?
My take on this is that the Proper Fix(tm) has been posted by Martin:
http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00082.html
IMO it's a no-brainer, modulo the ABI concern. Everything else is more or
less clever stuff to paper over this original compute_record_mode bug.
We are lying to the expander by pretending that the object has V2DImode since
it's larger and thus cannot be manipulated in this mode; everything is clearly
downhill from there. If we don't want to properly fix the bug then let's put
a hack in the expander, possibly using EXPAND_MEMORY, but it should trigger
_only_ for structures with zero-sized arrays and non-BLKmode and be preceded
by a big ??? comment explaining why it is deemed necessary.
--
Eric Botcazou
I agree, that assigning a non-BLKmode to structures with zero-sized arrays
should be considered a bug.
And it should be checked that there is only ONE zero-sized array.
And it should be checked that it is only allowed at the end of the structure.
Otherwise we have something like a union instead of a structure,
which will break the code in tree-ssa-alias.c !
And again, this is not only a problem of structures with zero-sized
arrays at the end. Remember my previous example code:
On ARM (or anything with STRICT_ALIGNMENT) this union has the
same problems:
/* PR middle-end/57748 */
/* arm-eabi-gcc -mcpu=cortex-a9 -O3 */
#include stdlib.h
union x
{
short a[2];
char x[4];
} __attribute__((packed, aligned(4))) ;
typedef volatile union x *s;
void __attribute__((noinline, noclone))
check (void)
{
s xx=(s)(0x8002);
/* although volatile xx-x[3] reads 4 bytes here */
if (xx-x[3] != 3)
abort ();
}
void __attribute__((noinline, noclone))
foo (void)
{
s xx=(s)(0x8002);
xx-x[3] = 3;
}
int
main ()
{
foo ();
check ();
return 0;
}
This union has a UINT32 mode, look at compute_record_mode!
Because of this example I still think that the expander should know what
we intend to do with the base object.
Regards
Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
I agree, that assigning a non-BLKmode to structures with zero-sized arrays
should be considered a bug.
Fine, then let's apply Martin's patch, on mainline at least.
And again, this is not only a problem of structures with zero-sized
arrays at the end. Remember my previous example code:
On ARM (or anything with STRICT_ALIGNMENT) this union has the
same problems:
/* PR middle-end/57748 */
/* arm-eabi-gcc -mcpu=cortex-a9 -O3 */
#include stdlib.h
union x
{
short a[2];
char x[4];
} __attribute__((packed, aligned(4))) ;
typedef volatile union x *s;
void __attribute__((noinline, noclone))
check (void)
{
s xx=(s)(0x8002);
/* although volatile xx-x[3] reads 4 bytes here */
if (xx-x[3] != 3)
abort ();
}
void __attribute__((noinline, noclone))
foo (void)
{
s xx=(s)(0x8002);
xx-x[3] = 3;
}
int
main ()
{
foo ();
check ();
return 0;
}
But this testcase is invalid on STRICT_ALIGNMENT platforms: xx is pointer to a
type with 4-byte alignment so its value must be a multiple of 4.
--
Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Tue, 8 Oct 2013 22:50:21, Eric Botcazou wrote:
I agree, that assigning a non-BLKmode to structures with zero-sized arrays
should be considered a bug.
Fine, then let's apply Martin's patch, on mainline at least.
That would definitely be a good move. Maybe someone should approve it?
But this testcase is invalid on STRICT_ALIGNMENT platforms: xx is pointer to a
type with 4-byte alignment so its value must be a multiple of 4.
Then you probably win. But I still have some doubts.
I had to use this silly alignment/pack(4) to circumvent this statement
in compute_record_mode:
/* If structure's known alignment is less than what the scalar
mode would need, and it matters, then stick with BLKmode. */
if (TYPE_MODE (type) != BLKmode
STRICT_ALIGNMENT
! (TYPE_ALIGN (type)= BIGGEST_ALIGNMENT
|| TYPE_ALIGN (type)= GET_MODE_ALIGNMENT (TYPE_MODE (type
{
/* If this is the only reason this type is BLKmode, then
don't force containing types to be BLKmode. */
TYPE_NO_FORCE_BLK (type) = 1;
SET_TYPE_MODE (type, BLKmode);
}
But there are at least two targets where STRICT_ALIGNMENT = 0
and SLOW_UNALIGNED_ACCESS != 0: rs6000 and alpha.
This example with a byte-aligned structure will on one of these targets
likely execute this code path in expand_expr_real_1/case MEM_REF:
else if (SLOW_UNALIGNED_ACCESS (mode, align))
temp = extract_bit_field (temp, GET_MODE_BITSIZE (mode),
0, TYPE_UNSIGNED (TREE_TYPE (exp)),
(modifier == EXPAND_STACK_PARM
? NULL_RTX : target),
mode, mode);
This looks wrong, but unfortunately I cannot test on these targets...
Regards
Bernd.
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi Eric, On Fri, 27 Sep 2013 10:36:57, Eric Botcazou wrote: Sure, but the modifier is not meant to force something into memory, especially when it is already in an register. Remember, we are only talking of structures here, and we only want to access one member. It is more the other way round: It says: You do not have to load the value in a register, if it is already in memory I'm happy EXPAND_MEMORY means we are interested in a memory result, even if the memory is constant and we could have propagated a constant value. */ We definitely want to propagate constant values here, look at the code below. And it already lists explicit cases where we really need to splill to memory. -- Eric Botcazou I'd like to continue this discussion by proposing this updated patch. I have now added a new value EXPAND_REFERENCE to the expand_modifier enumeration. It is almost like EXPAND_MEMORY but it does not interfere with constant values. I have done the same modification to VIEW_CONVERT_EXPR too, because this is a possible inner reference, itself. It is however inherently hard to test around this code. To understand this patch it is good to know what type of object the return value tem of get_inner_reference can be. From the program logic at get_inner_reference it is clear that the return value may *not* be BIT_FIELD_REF, COMPONENT_REF, ARRAY_REF, ARRAY_RANGE_REF, REALPART_EXPR, IMAGPART_EXPR. The result may be VIEW_CONVERT_EXPR only on a STRICT_ALIGNMENT target. This is probably further restricted because exp is gimplified. Usually the result will be a MEM_REF or a SSA_NAME of the memory where the structure is to be found. When you look at where EXPAND_MEMORY is handled you see it is special-cased in TARGET_MEM_REF, MEM_REF, ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. At TARGET_MEM_REF, MEM_REF, VIEW_CONVERT_EXPR, it should be the same if EXPAND_MEMORY, EXPAND_WRITE or EXPAND_REFERENCE is given: If it is an unaligned memory, we just return the unaligned reference. This was missing for VIEW_CONVERT_EXPR, and unfortunately I have no test case, because it is only a problem for STRICT_ALIGNMENT targets, and even there it will certainly be really hard to find test cases that exercise this code. In ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF we do not have to touch the handling of the outer modifier. However we pass EXPAND_REFERENCE to the inner object, which should not be a recursive use of any ARRAY_REF, COMPONENT_REF, BIT_FIELD_REF, ARRAY_RANGE_REF. TARGET_MEM_REF, MEM_REF and VIEW_CONVERT_EXPR know how to handle EXPAND_REFERENCE, anything else handles it like EXPAND_NORMAL. OK, what do you think of it now? Thanks Bernd.2013-10-03 Bernd Edlinger bernd.edlin...@hotmail.de PR middle-end/57748 * expr.h (expand_modifier): New enum value EXPAND_REFERENCE. * expr.c (expand_expr_real_1): Use EXAND_REFERENCE on base object. testsuite/ PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test. patch-pr57748-2.diff Description: Binary data
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Sure, but the modifier is not meant to force something into memory, especially when it is already in an register. Remember, we are only talking of structures here, and we only want to access one member. It is more the other way round: It says: You do not have to load the value in a register, if it is already in memory I'm happy EXPAND_MEMORY means we are interested in a memory result, even if the memory is constant and we could have propagated a constant value. */ We definitely want to propagate constant values here, look at the code below. And it already lists explicit cases where we really need to splill to memory. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Wed, 25 Sep 2013 16:01:02, Martin Jambor wrote:
Hi,
On Wed, Sep 25, 2013 at 11:05:21AM +0200, Richard Biener wrote:
On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor mjam...@suse.cz wrote:
Hi,
On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote:
On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
with the attached patch the read-side of the out of bounds accesses are
fixed.
There is a single new test case pr57748-3.c that is derived from Martin's
test case.
The test case does only test the read access and does not depend on part
1 of the
patch.
This patch was boot-strapped and regression tested on
x86_64-unknown-linux-gnu.
Additionally I generated eCos and an eCos-application (on ARMv5 using
packed
structures) with an arm-eabi cross compiler, and looked for differences
in the
disassembled code with and without this patch, but there were none.
OK for trunk?
Index: gcc/expr.c
===
--- gcc/expr.c (revision 202778)
+++ gcc/expr.c (working copy)
@@ -9878,7 +9878,7 @@
modifier != EXPAND_STACK_PARM
? target : NULL_RTX),
VOIDmode,
- modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+ EXPAND_MEMORY);
/* If the bitfield is volatile, we want to access it in the
field's mode, not the computed mode.
context suggests that we may arrive with EXPAND_STACK_PARM here
which is a correctness modifier (see its docs). But I'm not too familiar
with the details of the various expand modifiers, Eric may be though.
That said, I still believe that fixing the misalign path in
expand_assignment
would be better than trying to avoid it. For this testcase the issue is
again that expand_assignment passes the wrong mode/target to the
movmisalign optab.
Perhaps I'm stating the obvious, but this is supposed to address a
separate bug in the expansion of the RHS (as opposed to the first bug
which was in the expansion of the LHS), and so we would have to make
expand_assignment to also examine potential misalignments in the RHS,
which it so far does not do, despite its complexity.
Having said that, I also dislike the patch, but I am quite convinced
that if we allow non-BLK structures with zero sized arrays, the fix
will be ugly - but I'll be glad to be shown I've been wrong.
I don't think the issues have anything to do with zero sized arrays
or non-BLK structures. The issue is just we are feeding movmisaling
with the wrong mode (the mode of the base object) if we are expanding
a base object which is unaligned and has non-BLK mode.
I disagree. Consider a slightly modified testcase:
#include stdlib.h
typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
#if 1
typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
struct __attribute__((packed)) T { char c; P s; };
#else
typedef struct S { V a; V b[0]; } P;
struct T { char c; P s; };
#endif
void __attribute__((noinline, noclone))
good_value (V b)
{
if (b[0] != 3 || b[1] != 4)
__builtin_abort ();
}
void __attribute__((noinline, noclone))
check (P *p)
{
good_value (p-b[0]);
}
int
main ()
{
struct T *t = (struct T *) calloc (128, 1);
V a = { 3, 4 };
t-s.b[0] = a;
check (t-s);
free (t);
return 0;
}
The problem is the expansion of the memory load in function check.
All involved modes, the one of the base structure and of the loaded
component, are V2DI so even if we somehow mixed them up, in this
example it should not matter, yet the unaligned load gives wrong
results.
I'm still convinced that the problem is that we have a V2DImode
structure which is larger that the mode tells and we want to load the
data from outside of its mode. That is only happening because zero
sized arrays.
Yes, this is another good example, and it passes vector values on the
stack to a function. Again my patch produces working code, while
the current trunk produces just completely _insane_ code.
And again, this is not only a problem of structures with zero-sized
arrays at the end. Remember my previous example code:
On ARM (or anything with STRICT_ALIGNMENT) this union has the
same problems:
/* PR middle-end/57748 */
/* arm-eabi-gcc -mcpu=cortex-a9 -O3 */
#include stdlib.h
union x
{
short a[2];
char x[4];
} __attribute__((packed, aligned(4))) ;
typedef volatile union x *s;
void __attribute__((noinline, noclone))
check (void)
{
s xx=(s)(0x8002);
/* although volatile xx-x[3] reads 4 bytes here */
if (xx-x[3] != 3)
abort ();
}
void __attribute__((noinline, noclone))
foo (void)
{
s xx=(s)(0x8002);
xx-x[3] = 3;
}
int
main ()
{
foo ();
check ();
return 0;
}
So what we maybe need is another expand modifier that tells us
to not use movmisalign when expanding the base object.
In that case we can just as well use EXPAND_MEMORY. If so, I'd do
that only when there is a zero sized array involved in order not to
avoid
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
So I still think my patch does the right thing. The rationale is: = expand_expr (tem, (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE COMPLETE_TYPE_P (TREE_TYPE (tem)) (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem))) != INTEGER_CST) modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, EXPAND_MEMORY); returns the address of the structure in question, we can add offset, bitoffset, and access the memory in the right mode and alignment information is passed to the backend via MEM_ALIGN (op0). But there are conceptually no reasons to require a MEM here. Look at all the code just below the block. Given how hard it is to eliminate spills to memory in RTL once they are generated, this shouldn't be taken lightly. -- Eric Botcazou
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi, On Thu, 26 Sep 2013 11:34:02, Eric Botcazou wrote: So I still think my patch does the right thing. The rationale is: = expand_expr (tem, (TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE COMPLETE_TYPE_P (TREE_TYPE (tem)) (TREE_CODE (TYPE_SIZE (TREE_TYPE (tem))) != INTEGER_CST) modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, EXPAND_MEMORY); returns the address of the structure in question, we can add offset, bitoffset, and access the memory in the right mode and alignment information is passed to the backend via MEM_ALIGN (op0). But there are conceptually no reasons to require a MEM here. Look at all the code just below the block. Given how hard it is to eliminate spills to memory in RTL once they are generated, this shouldn't be taken lightly. -- Eric Botcazou Sure, but the modifier is not meant to force something into memory, especially when it is already in an register. Remember, we are only talking of structures here, and we only want to access one member. It is more the other way round: It says: You do not have to load the value in a register, if it is already in memory I'm happy At least in my understanding. Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor mjam...@suse.cz wrote: Hi, On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger bernd.edlin...@hotmail.de wrote: Hi, with the attached patch the read-side of the out of bounds accesses are fixed. There is a single new test case pr57748-3.c that is derived from Martin's test case. The test case does only test the read access and does not depend on part 1 of the patch. This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. Additionally I generated eCos and an eCos-application (on ARMv5 using packed structures) with an arm-eabi cross compiler, and looked for differences in the disassembled code with and without this patch, but there were none. OK for trunk? Index: gcc/expr.c === --- gcc/expr.c (revision 202778) +++ gcc/expr.c (working copy) @@ -9878,7 +9878,7 @@ modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, -modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); +EXPAND_MEMORY); /* If the bitfield is volatile, we want to access it in the field's mode, not the computed mode. context suggests that we may arrive with EXPAND_STACK_PARM here which is a correctness modifier (see its docs). But I'm not too familiar with the details of the various expand modifiers, Eric may be though. That said, I still believe that fixing the misalign path in expand_assignment would be better than trying to avoid it. For this testcase the issue is again that expand_assignment passes the wrong mode/target to the movmisalign optab. Perhaps I'm stating the obvious, but this is supposed to address a separate bug in the expansion of the RHS (as opposed to the first bug which was in the expansion of the LHS), and so we would have to make expand_assignment to also examine potential misalignments in the RHS, which it so far does not do, despite its complexity. Having said that, I also dislike the patch, but I am quite convinced that if we allow non-BLK structures with zero sized arrays, the fix will be ugly - but I'll be glad to be shown I've been wrong. I don't think the issues have anything to do with zero sized arrays or non-BLK structures. The issue is just we are feeding movmisaling with the wrong mode (the mode of the base object) if we are expanding a base object which is unaligned and has non-BLK mode. So what we maybe need is another expand modifier that tells us to not use movmisalign when expanding the base object. Or we need to stop expanding the base object with VOIDmode and instead pass down the mode of the access (TYPE_MODE (TREE_TYPE (exp)) which will probably confuse other code. So eventually not handling the misaligned case by expansion of the base but inlined similar to how it was in expand_assignment would be needed here. Richard. Martin
RE: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hmm.,
On Tue, 24 Sep 2013 20:06:51, Martin Jambor wrote:
Hi,
On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote:
On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
with the attached patch the read-side of the out of bounds accesses are
fixed.
There is a single new test case pr57748-3.c that is derived from Martin's
test case.
The test case does only test the read access and does not depend on part 1
of the
patch.
This patch was boot-strapped and regression tested on
x86_64-unknown-linux-gnu.
Additionally I generated eCos and an eCos-application (on ARMv5 using packed
structures) with an arm-eabi cross compiler, and looked for differences in
the
disassembled code with and without this patch, but there were none.
OK for trunk?
Index: gcc/expr.c
===
--- gcc/expr.c (revision 202778)
+++ gcc/expr.c (working copy)
@@ -9878,7 +9878,7 @@
modifier != EXPAND_STACK_PARM
? target : NULL_RTX),
VOIDmode,
- modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
+ EXPAND_MEMORY);
/* If the bitfield is volatile, we want to access it in the
field's mode, not the computed mode.
context suggests that we may arrive with EXPAND_STACK_PARM here
which is a correctness modifier (see its docs). But I'm not too familiar
with the details of the various expand modifiers, Eric may be though.
EXPAND_STACK_PARM is used when expanding to a TARGET on the stack for
a call parameter. Such targets require special care as we haven't yet
marked TARGET so that it's safe from being trashed by libcalls. We
don't want to use TARGET for anything but the final result;
Intermediate values must go elsewhere. Additionally, calls to
emit_block_move will be flagged with BLOCK_OP_CALL_PARM.
This should not be a problem: If modifier == EXPAND_STACK_PARM
the target is NOT passed down, and thus not giving the modifier either
should not cause incorrect code. But I might be wrong...
On the other hand, in the case where tem is a unaligned MEM_REF and
exp is not exactly the same object as tem,
EXPAND_STACK_PARM may exactly do the wrong thing.
What I can tell is, that this patch does not change a single bit in the complete
GCC boot strap process, stage 2 and stage 3 are binary identical with or
without this patch, and that all test cases pass, even the ADA test cases...
That said, I still believe that fixing the misalign path in expand_assignment
would be better than trying to avoid it. For this testcase the issue is
again that expand_assignment passes the wrong mode/target to the
movmisalign optab.
Perhaps I'm stating the obvious, but this is supposed to address a
separate bug in the expansion of the RHS (as opposed to the first bug
which was in the expansion of the LHS), and so we would have to make
expand_assignment to also examine potential misalignments in the RHS,
which it so far does not do, despite its complexity.
Thanks for pointing that out. This is true.
Having said that, I also dislike the patch, but I am quite convinced
that if we allow non-BLK structures with zero sized arrays, the fix
will be ugly - but I'll be glad to be shown I've been wrong.
Martin
I could of course look at the type of tem, and only mess with the
expand modifier in case of a MEM_REF. But I am not sure what
to do about the VIEW_CONVERT_EXPR, if the code at normal_inner_ref
is wrong, the code at VIEW_CONVERT_EXPR can't possibly be any better,
although I have not yet any idea how to write a test case for this:
/* ??? We should work harder and deal with non-zero offsets. */
if (!offset
(bitpos % BITS_PER_UNIT) == 0
bitsize= 0
compare_tree_int (TYPE_SIZE (type), bitsize) == 0)
{
/* See the normal_inner_ref case for the rationale. */
orig_op0
= expand_expr (tem,
(TREE_CODE (TREE_TYPE (tem)) == UNION_TYPE
(TREE_CODE (TYPE_SIZE (TREE_TYPE (tem)))
!= INTEGER_CST)
modifier != EXPAND_STACK_PARM
? target : NULL_RTX),
VOIDmode,
modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier);
Especially for EXPAND_STACK_PARM? If this does really allocate
something on a register, then the result will be thrown away, because
it is not MEM_P?
Right?
Note the RHS-Bug can also affect ordinary unions on ARM and all
STRICT_ALIGNMENT targets.
Please check the attached test case. It may not be obvious at
first sight, but the assembler code for check is definitely wrong,
because it reads the whole structure, and uses only one byte of it.
And that is, not OK because x was volatile...
check:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 0, uses_anonymous_args = 0
stmfd sp!,
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi,
On Wed, Sep 25, 2013 at 11:05:21AM +0200, Richard Biener wrote:
On Tue, Sep 24, 2013 at 8:06 PM, Martin Jambor mjam...@suse.cz wrote:
Hi,
On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote:
On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger
bernd.edlin...@hotmail.de wrote:
Hi,
with the attached patch the read-side of the out of bounds accesses are
fixed.
There is a single new test case pr57748-3.c that is derived from
Martin's test case.
The test case does only test the read access and does not depend on part
1 of the
patch.
This patch was boot-strapped and regression tested on
x86_64-unknown-linux-gnu.
Additionally I generated eCos and an eCos-application (on ARMv5 using
packed
structures) with an arm-eabi cross compiler, and looked for differences
in the
disassembled code with and without this patch, but there were none.
OK for trunk?
Index: gcc/expr.c
===
--- gcc/expr.c (revision 202778)
+++ gcc/expr.c (working copy)
@@ -9878,7 +9878,7 @@
modifier != EXPAND_STACK_PARM
? target : NULL_RTX),
VOIDmode,
-modifier == EXPAND_SUM ? EXPAND_NORMAL :
modifier);
+EXPAND_MEMORY);
/* If the bitfield is volatile, we want to access it in the
field's mode, not the computed mode.
context suggests that we may arrive with EXPAND_STACK_PARM here
which is a correctness modifier (see its docs). But I'm not too familiar
with the details of the various expand modifiers, Eric may be though.
That said, I still believe that fixing the misalign path in
expand_assignment
would be better than trying to avoid it. For this testcase the issue is
again that expand_assignment passes the wrong mode/target to the
movmisalign optab.
Perhaps I'm stating the obvious, but this is supposed to address a
separate bug in the expansion of the RHS (as opposed to the first bug
which was in the expansion of the LHS), and so we would have to make
expand_assignment to also examine potential misalignments in the RHS,
which it so far does not do, despite its complexity.
Having said that, I also dislike the patch, but I am quite convinced
that if we allow non-BLK structures with zero sized arrays, the fix
will be ugly - but I'll be glad to be shown I've been wrong.
I don't think the issues have anything to do with zero sized arrays
or non-BLK structures. The issue is just we are feeding movmisaling
with the wrong mode (the mode of the base object) if we are expanding
a base object which is unaligned and has non-BLK mode.
I disagree. Consider a slightly modified testcase:
#include stdlib.h
typedef long long V
__attribute__ ((vector_size (2 * sizeof (long long)), may_alias));
#if 1
typedef struct S { V a; V b[0]; } P __attribute__((aligned (1)));
struct __attribute__((packed)) T { char c; P s; };
#else
typedef struct S { V a; V b[0]; } P;
struct T { char c; P s; };
#endif
void __attribute__((noinline, noclone))
good_value (V b)
{
if (b[0] != 3 || b[1] != 4)
__builtin_abort ();
}
void __attribute__((noinline, noclone))
check (P *p)
{
good_value (p-b[0]);
}
int
main ()
{
struct T *t = (struct T *) calloc (128, 1);
V a = { 3, 4 };
t-s.b[0] = a;
check (t-s);
free (t);
return 0;
}
The problem is the expansion of the memory load in function check.
All involved modes, the one of the base structure and of the loaded
component, are V2DI so even if we somehow mixed them up, in this
example it should not matter, yet the unaligned load gives wrong
results.
I'm still convinced that the problem is that we have a V2DImode
structure which is larger that the mode tells and we want to load the
data from outside of its mode. That is only happening because zero
sized arrays.
So what we maybe need is another expand modifier that tells us
to not use movmisalign when expanding the base object.
In that case we can just as well use EXPAND_MEMORY. If so, I'd do
that only when there is a zero sized array involved in order not to
avoid using movmisalign when we can.
Or we need to stop expanding the base object with VOIDmode and
instead pass down the mode of the access (TYPE_MODE (TREE_TYPE
(exp)) which will probably confuse other code.
Well, because I think the problem is not (just) mixing up modes, I
don't think this will help either.
So eventually not
handling the misaligned case by expansion of the base but inlined
similar to how it was in expand_assignment would be needed here.
At least now I fail to see how this would be different from copying a
large portion of expand_expr_real_1 (all of the MEM_REF part without
the movmisalign bit) and calling that when we detect this case
whatever we eventually agree it is.
Martin
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger bernd.edlin...@hotmail.de wrote: Hi, with the attached patch the read-side of the out of bounds accesses are fixed. There is a single new test case pr57748-3.c that is derived from Martin's test case. The test case does only test the read access and does not depend on part 1 of the patch. This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. Additionally I generated eCos and an eCos-application (on ARMv5 using packed structures) with an arm-eabi cross compiler, and looked for differences in the disassembled code with and without this patch, but there were none. OK for trunk? Index: gcc/expr.c === --- gcc/expr.c (revision 202778) +++ gcc/expr.c (working copy) @@ -9878,7 +9878,7 @@ modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, -modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); +EXPAND_MEMORY); /* If the bitfield is volatile, we want to access it in the field's mode, not the computed mode. context suggests that we may arrive with EXPAND_STACK_PARM here which is a correctness modifier (see its docs). But I'm not too familiar with the details of the various expand modifiers, Eric may be though. That said, I still believe that fixing the misalign path in expand_assignment would be better than trying to avoid it. For this testcase the issue is again that expand_assignment passes the wrong mode/target to the movmisalign optab. Richard. Regards Bernd.
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Index: gcc/expr.c === --- gcc/expr.c (revision 202778) +++ gcc/expr.c (working copy) @@ -9878,7 +9878,7 @@ modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, -modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); +EXPAND_MEMORY); /* If the bitfield is volatile, we want to access it in the field's mode, not the computed mode. context suggests that we may arrive with EXPAND_STACK_PARM here which is a correctness modifier (see its docs). But I'm not too familiar with the details of the various expand modifiers, Eric may be though. Yes, this change looks far too bold and is presumably papering over the underlying issue... That said, I still believe that fixing the misalign path in expand_assignment would be better than trying to avoid it. For this testcase the issue is again that expand_assignment passes the wrong mode/target to the movmisalign optab. ...then let's just fix the movmisalign stuff. -- Eric Botcazou
Re: [PATCH, PR 57748] Check for out of bounds access, Part 2
Hi, On Tue, Sep 24, 2013 at 12:02:17PM +0200, Richard Biener wrote: On Tue, Sep 24, 2013 at 4:52 AM, Bernd Edlinger bernd.edlin...@hotmail.de wrote: Hi, with the attached patch the read-side of the out of bounds accesses are fixed. There is a single new test case pr57748-3.c that is derived from Martin's test case. The test case does only test the read access and does not depend on part 1 of the patch. This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. Additionally I generated eCos and an eCos-application (on ARMv5 using packed structures) with an arm-eabi cross compiler, and looked for differences in the disassembled code with and without this patch, but there were none. OK for trunk? Index: gcc/expr.c === --- gcc/expr.c (revision 202778) +++ gcc/expr.c (working copy) @@ -9878,7 +9878,7 @@ modifier != EXPAND_STACK_PARM ? target : NULL_RTX), VOIDmode, -modifier == EXPAND_SUM ? EXPAND_NORMAL : modifier); +EXPAND_MEMORY); /* If the bitfield is volatile, we want to access it in the field's mode, not the computed mode. context suggests that we may arrive with EXPAND_STACK_PARM here which is a correctness modifier (see its docs). But I'm not too familiar with the details of the various expand modifiers, Eric may be though. That said, I still believe that fixing the misalign path in expand_assignment would be better than trying to avoid it. For this testcase the issue is again that expand_assignment passes the wrong mode/target to the movmisalign optab. Perhaps I'm stating the obvious, but this is supposed to address a separate bug in the expansion of the RHS (as opposed to the first bug which was in the expansion of the LHS), and so we would have to make expand_assignment to also examine potential misalignments in the RHS, which it so far does not do, despite its complexity. Having said that, I also dislike the patch, but I am quite convinced that if we allow non-BLK structures with zero sized arrays, the fix will be ugly - but I'll be glad to be shown I've been wrong. Martin
[PATCH, PR 57748] Check for out of bounds access, Part 2
Hi, with the attached patch the read-side of the out of bounds accesses are fixed. There is a single new test case pr57748-3.c that is derived from Martin's test case. The test case does only test the read access and does not depend on part 1 of the patch. This patch was boot-strapped and regression tested on x86_64-unknown-linux-gnu. Additionally I generated eCos and an eCos-application (on ARMv5 using packed structures) with an arm-eabi cross compiler, and looked for differences in the disassembled code with and without this patch, but there were none. OK for trunk? Regards Bernd.2013-09-24 Bernd Edlinger bernd.edlin...@hotmail.de PR middle-end/57748 * expr.c (expand_expr_real_1): Use EXAND_MEMORY on base object. testsuite/ PR middle-end/57748 * gcc.dg/torture/pr57748-3.c: New test. patch-pr57748-2.diff Description: Binary data
