| Message ID | 20140611143414.GJ17894@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 11, 2014 at 6:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > Hi, > > This patch fixes problem with size emitted for static structures with flexible array. I found a couple of trackers in guzilla for this problem but all of them are marked as fixed and problem still exists. > > For a simple testcase > > struct S { int a; int b[0]; } s = { 1, { 0, 0} }; > > current trunk produces (no flags): > > .globl s > .data > .align 4 > .type s, @object > .size s, 4 > s: > .long 1 > .long 0 > .long 0 > > which has wrong size for object s. > > This problem is important for checker because wrong size leads to wrong bounds and false bounds violations. Following patch uses DECL_SIZE_UNIT instead of type size and works well for me. Does it look OK? There is a bug about this in bugzilla somewhere. It looks ok to me - did you test with all languages? In particular did you test Ada? Thanks, Richard. > Bootstrapped and tested on linux-x86_64. > > Thanks, > Ilya > -- > gcc/ > > 2014-06-11 Ilya Enkovich <ilya.enkovich@intel.com> > > * config/elfos.h (ASM_DECLARE_OBJECT_NAME): Use decl size > instead of type size. > (ASM_FINISH_DECLARE_OBJECT): Likewise. > > > diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h > index c1d5553..7929708 100644 > --- a/gcc/config/elfos.h > +++ b/gcc/config/elfos.h > @@ -313,7 +313,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see > && (DECL) && DECL_SIZE (DECL)) \ > { \ > size_directive_output = 1; \ > - size = int_size_in_bytes (TREE_TYPE (DECL)); \ > + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ > ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, size); \ > } \ > \ > @@ -341,7 +341,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see > && !size_directive_output) \ > { \ > size_directive_output = 1; \ > - size = int_size_in_bytes (TREE_TYPE (DECL)); \ > + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ > ASM_OUTPUT_SIZE_DIRECTIVE (FILE, name, size); \ > } \ > } \
2014-06-12 11:55 GMT+04:00 Richard Biener <richard.guenther@gmail.com>: > On Wed, Jun 11, 2014 at 6:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >> Hi, >> >> This patch fixes problem with size emitted for static structures with flexible array. I found a couple of trackers in guzilla for this problem but all of them are marked as fixed and problem still exists. >> >> For a simple testcase >> >> struct S { int a; int b[0]; } s = { 1, { 0, 0} }; >> >> current trunk produces (no flags): >> >> .globl s >> .data >> .align 4 >> .type s, @object >> .size s, 4 >> s: >> .long 1 >> .long 0 >> .long 0 >> >> which has wrong size for object s. >> >> This problem is important for checker because wrong size leads to wrong bounds and false bounds violations. Following patch uses DECL_SIZE_UNIT instead of type size and works well for me. Does it look OK? > > There is a bug about this in bugzilla somewhere. I looked through bugzilla and found two trackers with similar problem. The first one is 57180 which is still open but with comment that problem is fixed on the trunk. I checked it and it really passes for the trunk (should tracker be closed then?). Another one is 28865 (and a set of its duplicates). Interesting thing here is that original testcase uses array of integers but testcases which were added with commit use arrays of chars. Original test is still compiled wrongly. I also see that a patch very similar to what I posted was proposed as a solution but it was reported to cause a problem with glibc/nss/nss_files/files-init.c. There is a corresponding testcase in the tracker which results wrong padding when patch is applied but it seems to be another problem because I do not see any problem when use mpx compiler branch for this testcase. > > It looks ok to me - did you test with all languages? In particular did > you test Ada? I configure compiler with no language disabling and then run bootstrap and make check. Does it mean all languages are covered? Will make more testing if required. Thanks, Ilya > > Thanks, > Richard. > >> Bootstrapped and tested on linux-x86_64. >> >> Thanks, >> Ilya >> -- >> gcc/ >> >> 2014-06-11 Ilya Enkovich <ilya.enkovich@intel.com> >> >> * config/elfos.h (ASM_DECLARE_OBJECT_NAME): Use decl size >> instead of type size. >> (ASM_FINISH_DECLARE_OBJECT): Likewise. >> >> >> diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h >> index c1d5553..7929708 100644 >> --- a/gcc/config/elfos.h >> +++ b/gcc/config/elfos.h >> @@ -313,7 +313,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see >> && (DECL) && DECL_SIZE (DECL)) \ >> { \ >> size_directive_output = 1; \ >> - size = int_size_in_bytes (TREE_TYPE (DECL)); \ >> + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ >> ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, size); \ >> } \ >> \ >> @@ -341,7 +341,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see >> && !size_directive_output) \ >> { \ >> size_directive_output = 1; \ >> - size = int_size_in_bytes (TREE_TYPE (DECL)); \ >> + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ >> ASM_OUTPUT_SIZE_DIRECTIVE (FILE, name, size); \ >> } \ >> } \
On Fri, Jun 13, 2014 at 1:38 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: > 2014-06-12 11:55 GMT+04:00 Richard Biener <richard.guenther@gmail.com>: >> On Wed, Jun 11, 2014 at 6:08 PM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote: >>> Hi, >>> >>> This patch fixes problem with size emitted for static structures with flexible array. I found a couple of trackers in guzilla for this problem but all of them are marked as fixed and problem still exists. >>> >>> For a simple testcase >>> >>> struct S { int a; int b[0]; } s = { 1, { 0, 0} }; >>> >>> current trunk produces (no flags): >>> >>> .globl s >>> .data >>> .align 4 >>> .type s, @object >>> .size s, 4 >>> s: >>> .long 1 >>> .long 0 >>> .long 0 >>> >>> which has wrong size for object s. >>> >>> This problem is important for checker because wrong size leads to wrong bounds and false bounds violations. Following patch uses DECL_SIZE_UNIT instead of type size and works well for me. Does it look OK? >> >> There is a bug about this in bugzilla somewhere. > > I looked through bugzilla and found two trackers with similar problem. > > The first one is 57180 which is still open but with comment that > problem is fixed on the trunk. I checked it and it really passes for > the trunk (should tracker be closed then?). > > Another one is 28865 (and a set of its duplicates). Interesting thing > here is that original testcase uses array of integers but testcases > which were added with commit use arrays of chars. Original test is > still compiled wrongly. I also see that a patch very similar to what I > posted was proposed as a solution but it was reported to cause a > problem with glibc/nss/nss_files/files-init.c. There is a > corresponding testcase in the tracker which results wrong padding when > patch is applied but it seems to be another problem because I do not > see any problem when use mpx compiler branch for this testcase. > >> >> It looks ok to me - did you test with all languages? In particular did >> you test Ada? > > I configure compiler with no language disabling and then run bootstrap > and make check. Does it mean all languages are covered? Will make more > testing if required. That only enables default languages which excludes Objective-C++, Ada and Go. To enable really all languages you have to specify --enable-languages=all,ada,obj-c++,go Richard. > Thanks, > Ilya > >> >> Thanks, >> Richard. >> >>> Bootstrapped and tested on linux-x86_64. >>> >>> Thanks, >>> Ilya >>> -- >>> gcc/ >>> >>> 2014-06-11 Ilya Enkovich <ilya.enkovich@intel.com> >>> >>> * config/elfos.h (ASM_DECLARE_OBJECT_NAME): Use decl size >>> instead of type size. >>> (ASM_FINISH_DECLARE_OBJECT): Likewise. >>> >>> >>> diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h >>> index c1d5553..7929708 100644 >>> --- a/gcc/config/elfos.h >>> +++ b/gcc/config/elfos.h >>> @@ -313,7 +313,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see >>> && (DECL) && DECL_SIZE (DECL)) \ >>> { \ >>> size_directive_output = 1; \ >>> - size = int_size_in_bytes (TREE_TYPE (DECL)); \ >>> + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ >>> ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, size); \ >>> } \ >>> \ >>> @@ -341,7 +341,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see >>> && !size_directive_output) \ >>> { \ >>> size_directive_output = 1; \ >>> - size = int_size_in_bytes (TREE_TYPE (DECL)); \ >>> + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ >>> ASM_OUTPUT_SIZE_DIRECTIVE (FILE, name, size); \ >>> } \ >>> } \
On 06/12/14 17:38, Ilya Enkovich wrote: >> It looks ok to me - did you test with all languages? In particular did >> you test Ada? > > I configure compiler with no language disabling and then run bootstrap > and make check. Does it mean all languages are covered? Will make more > testing if required. In addition to Richi's comment able --enable-languages= ... You also need an Ada compiler installed to build Ada. Most Linux distributions provide Ada packages of some sort. It may be called ada or gnat. Jeff
2014-06-13 19:40 GMT+04:00 Jeff Law <law@redhat.com>: > On 06/12/14 17:38, Ilya Enkovich wrote: >>> >>> It looks ok to me - did you test with all languages? In particular did >>> you test Ada? >> >> >> I configure compiler with no language disabling and then run bootstrap >> and make check. Does it mean all languages are covered? Will make more >> testing if required. > > In addition to Richi's comment able --enable-languages= ... You also need > an Ada compiler installed to build Ada. Most Linux distributions provide > Ada packages of some sort. It may be called ada or gnat. > Thanks for tips! I added --enable-languages=all,ada,obj-c++,go and got no new failures with this patch. Ilya > Jeff > >
diff --git a/gcc/config/elfos.h b/gcc/config/elfos.h index c1d5553..7929708 100644 --- a/gcc/config/elfos.h +++ b/gcc/config/elfos.h @@ -313,7 +313,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see && (DECL) && DECL_SIZE (DECL)) \ { \ size_directive_output = 1; \ - size = int_size_in_bytes (TREE_TYPE (DECL)); \ + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ ASM_OUTPUT_SIZE_DIRECTIVE (FILE, NAME, size); \ } \ \ @@ -341,7 +341,7 @@ see the files COPYING3 and COPYING.RUNTIME respectively. If not, see && !size_directive_output) \ { \ size_directive_output = 1; \ - size = int_size_in_bytes (TREE_TYPE (DECL)); \ + size = tree_to_uhwi (DECL_SIZE_UNIT (DECL)); \ ASM_OUTPUT_SIZE_DIRECTIVE (FILE, name, size); \ } \ } \
Hi, This patch fixes problem with size emitted for static structures with flexible array. I found a couple of trackers in guzilla for this problem but all of them are marked as fixed and problem still exists. For a simple testcase struct S { int a; int b[0]; } s = { 1, { 0, 0} }; current trunk produces (no flags): .globl s .data .align 4 .type s, @object .size s, 4 s: .long 1 .long 0 .long 0 which has wrong size for object s. This problem is important for checker because wrong size leads to wrong bounds and false bounds violations. Following patch uses DECL_SIZE_UNIT instead of type size and works well for me. Does it look OK? Bootstrapped and tested on linux-x86_64. Thanks, Ilya -- gcc/ 2014-06-11 Ilya Enkovich <ilya.enkovich@intel.com> * config/elfos.h (ASM_DECLARE_OBJECT_NAME): Use decl size instead of type size. (ASM_FINISH_DECLARE_OBJECT): Likewise.