Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix issue 120 #30

Merged
merged 6 commits into from

2 participants

@jpf91
Collaborator

This pull request includes these changes to fix issue #120:

  • The static array initializer fix posted in the bug report (Pad static arrays with single elements )
  • The TypeInfoDeclaration::toSymbol code did not correctly set DECL_SIZE
  • The size of ModuleInfo and ClassInfo is not known until the initializer is constructed. Added a helper method which generates a struct node with unknown size. Then infer the size in outdata and set it correctly.
  • TypeInfoStruct::toDt produced variable length output. Fixed by using an extra static symbol for name. (This should also be fixed in upstream dmd and is also contained in this pull request: D-Programming-Language/dmd#1128)
  • The TypeInfo_Function and TypeInfo_Delegate declarations in druntime are wrong. The fix is already applied in upstream druntime (D-Programming-Language/druntime#307), but I included it here, so we don't have to wait for version 2.061.
  • That dmd pull request will also error out if a TypeInfo declaration in druntime doesn't match the one in the compiler. So issues like that shouldn't happen again.
  • Added code to outdata which detects size mismatches between initializer size, type size and declaration size and ices on these errors. (We sometimes have smaller initializers than DECL_SIZE, but I guess that's not a problem. So the code only checks for bigger initializers). This should make it easier to detect bugs which lead to problems with section anchors.

I tested those changes on x86, x64-64 and arm. There are no regressions in the test suite (which is especially important since this code introduces a new ice). Those changes produce the same testsuite output on ARM as using -fno-section-anchors (Actually, they're even a little better. Seems like I didn't correctly pass the -fno-section-anchors to the testsuite runner). So I hope these changes should fix all section anchor issues.

The testsuite results for reference: http://dl.dropbox.com/u/24218791/fix120/testsuite.html
(You might wonder why there is one more 'unresolved' runnable test with the new code on ARM: This is the runnable/test23.d test. It went from FAIL to UNRESOLVED. It now shows the same error on ARM as on X86)

Note: With this solution we don't have debug info for ClassInfos and ModuleInfos. This should really be fixed in the frontend, but that's more complicated than I thought. I'll file a bug report on the dmd bugtracker though, to make sure it won't be forgotten.

@jpf91 jpf91 Fix static array initializers
partial fix for issue #120 (fsection-anchors)

Pad out the rest of static arrays with single elements.
Issue #120 - breaks -fsection-anchors on ARM when
backend calculates field positions for array members.
eca4a43
gcc/d/d-decls.cc
@@ -347,6 +347,9 @@
gcc_assert (TREE_CODE (TREE_TYPE (csym->Stree)) == REFERENCE_TYPE);
TREE_TYPE (csym->Stree) = TREE_TYPE (TREE_TYPE (csym->Stree));
TREE_USED (csym->Stree) = 1;
+ DECL_SIZE (csym->Stree) = TYPE_SIZE (TREE_TYPE (csym->Stree));
+ DECL_SIZE_UNIT (csym->Stree) = TYPE_SIZE_UNIT (TREE_TYPE (csym->Stree));
+ DECL_ALIGN (csym->Stree) = TYPE_ALIGN (TREE_TYPE (csym->Stree));
@ibuclaw Owner
ibuclaw added a note

layout_decl (csym->Stree); should do just fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
gcc/d/d-decls.cc
@@ -679,9 +693,7 @@
tree decl;
csym = toSymbolX ("__Class", SCextern, 0, "Z");
decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (csym->Sident),
- TREE_TYPE (ClassDeclaration::classinfo != NULL
- ? ClassDeclaration::classinfo->type->toCtype() // want the RECORD_TYPE, not the REFERENCE_TYPE
- : error_mark_node));
+ fakeStructType());
csym->Stree = decl;
@ibuclaw Owner
ibuclaw added a note

Just make_node (RECORD_TYPE) will do here, no need for fakeStructType function. The node is initialised to zero by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
gcc/d/d-decls.cc
((6 lines not shown))
csym = toSymbolX ("__ModuleInfo", SCextern, 0, "Z");
tree decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (csym->Sident),
- TREE_TYPE (obj_type->toCtype())); // want the RECORD_TYPE, not the REFERENCE_TYPE
+ fakeStructType());
g.ofile->setDeclLoc (decl, this);
@ibuclaw Owner
ibuclaw added a note

LIkewise, as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
gcc/d/d-objfile.cc
@@ -933,6 +933,18 @@
void
outdata (Symbol *sym)
{
+ //Incomplete fakeStruct
+ if(sym->Stree && sym->Sdt && TYPE_SIZE(TREE_TYPE(sym->Stree)) == NULL_TREE)
+ {
+ tree fdecl = sym->Stree;
+ tree ftype = TREE_TYPE(fdecl);
+ size_t fsize = dt_size(sym->Sdt);
+ TYPE_SIZE_UNIT(ftype) = size_int (fsize);
+ TYPE_SIZE(ftype) = bitsize_int (fsize * BITS_PER_UNIT);
+ DECL_SIZE_UNIT(fdecl) = size_int (fsize);
+ DECL_SIZE(fdecl) = bitsize_int (fsize * BITS_PER_UNIT);
+ }
+
tree t = check_static_sym (sym);
@ibuclaw Owner
ibuclaw added a note

Could be moved lower down.

-  if (sym->Sdt && DECL_INITIAL (t) == NULL_TREE)
-    DECL_INITIAL (t) = dt2tree (sym->Sdt);
+  if (sym->Sdt)
+    {
+      if (!COMPLETE_TYPE_P (TREE_TYPE (t)))
+        {
+          size_t fsize = dt_size (sym->Sdt);
+          TYPE_SIZE (TREE_TYPE (t)) = bitsize_int (fsize * BITS_PER_UNIT);
+          TYPE_SIZE_UNIT (TREE_TYPE (t)) = size_int (fsize);
+        }
+
+      if (DECL_INITIAL (t) == NULL_TREE)
+        DECL_INITIAL (t) = dt2tree (sym->Sdt);
+    }

relayout_decl takes care of DECL_SIZE, so you don't have to worry about that here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
gcc/d/d-objfile.cc
((18 lines not shown))
+ double_int_to_uhwi(TREE_INT_CST(declSize)));
+ fprintf(stderr, "\tInitializer size: "HOST_WIDE_INT_PRINT_DEC"\n",
+ double_int_to_uhwi(TREE_INT_CST(initSize)));
+ gcc_assert(false);
+ }
+ if(!tree_int_cst_equal(declSize, typeSize))
+ {
+ fprintf(stderr, "ICE: Mismatch between declaration size and type size.\n");
+ fprintf(stderr, "\tSymbol: %s (%s)\n", sym->Sident ? sym->Sident : "",
+ sym->prettyIdent ? sym->prettyIdent : "");
+ fprintf(stderr, "\tDeclaration size: "HOST_WIDE_INT_PRINT_DEC"\n",
+ double_int_to_uhwi(TREE_INT_CST(declSize)));
+ fprintf(stderr, "\tType size: "HOST_WIDE_INT_PRINT_DEC"\n",
+ double_int_to_uhwi(TREE_INT_CST(typeSize)));
+ gcc_assert(false);
+ }
@ibuclaw Owner
ibuclaw added a note

Maybe something more like below ?

      size_t typesize = int_size_in_bytes (TREE_TYPE (t));
      size_t initsize = int_size_in_bytes (TREE_TYPE (DECL_INITIAL (t)));
      size_t declsize = gen.getTargetSizeConst (DECL_SIZE (t));

      if (typesize < initsize)
        internal_error ("Mismatch between declaration '%s' size (%wd) and it's initializer size (%wd).",
                        sym->prettyIdent ? sym->prettyIdent : sym->Sident, declsize, initsize);
      else if (declsize != typesize)
        internal_error ("Mismatch between declaration '%s' size (%wd) and it's type size (%wd).",
                        sym->prettyIdent ? sym->prettyIdent : sym->Sident, declsize, typesize);
@jpf91 Collaborator
jpf91 added a note

The original check compared declsize < initsize your code typesize < initsize is that intentional?

And getTargetSizeConst seems to be in bits, so it has to be divided by BITS_PER_UNIT.
And the whole code needs to be moved down below relayout_decl.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jpf91
Collaborator

OK, updated. layout_decl needs a second parameter:

Second argument is the boundary that this field can be assumed to
be starting at (in bits).  Zero means it can be assumed aligned
on any boundary that may be needed. 

Is 0 ok in this case?

and the printf %wd complains on 32bit:

../../gcc-4.8-20120826/gcc/d/d-objfile.cc:971:94: warning: format '%wd' expects argument of type 'long long int', but argument 4 has type 'size_t {aka unsigned int}' [-Wformat]

is this ok or should we use some different format specifier?

jpf91 added some commits
@jpf91 jpf91 Set correct DECL_SIZE for TypeInfoDeclaration
partial fix for issue #120 (fsection-anchors)
8d72ec9
@jpf91 jpf91 Set correct DECL_SIZE for ModuleInfo and ClassInfo
partial fix for issue #120 (fsection-anchors)
b6d337b
@jpf91 jpf91 Fix TypeInfoStruct initializer
partial fix for issue #120 (fsection-anchors)
ab0a5c3
@jpf91 jpf91 Fix TypeInfo_Function and TypeInfo_Delegate declarations
partial fix for issue #120 (fsection-anchors)
c50933e
@jpf91
Collaborator

OK, implemented you suggestions. There are no regressions on x86, so as the check should turn those errors into ices I just assume it works on ARM (recompiling gdc and running the testsuite takes more than 10 hours).

The relayout_decl and similar functions are not really documented, right? I didn't know these functions existed at all. I also searched for something like internal_error but couldn't find it ;-)

BTW: Does GDC not use HOST_WIDE_INT at all? I thought that's needed when using a 32bit->64bit cross compiler (but probably sizes > uint.max won't happen anyway).

@ibuclaw
Owner
  1. Yep, use 0 in layout_decl.
  2. %wd is the formatter for HOST_WIDE_INT types (GCC has it's own formatting parser with use of it's warning and error routines (see diagnostic.c / *-pretty-print.c) - I assume you were getting that because I put down size_t.
  3. No, that wasn't intentional - I just misread the patch when I was quickly typing minor corrections away. To be honest, I didn't test anything I wrote - I just wrote it think "this works in my head". :-)
  4. I'm pretty certain GCC documentation consists of only the MACROS that frontends / targets can use. For everything else, either look how other frontends work, or look at the headers.
  5. GDC uses dinteger_t when handling const values - which should be typed as the same size.
@jpf91 jpf91 ICE on all issue #120 bugs
The problems in issue #120 are caused by an initializer
for a variable with a bigger size than the variables
DECL_SIZE. Detect all those cases and ICE.
b47bb89
@jpf91
Collaborator

OK, it's using HOST_WIDE_INT and %wd now. Is there anything more that needs to be done for this pull request?

@ibuclaw
Owner
@jpf91
Collaborator

OK, thanks.

@ibuclaw
Owner

Not sure if it's problems with my set-up, or the fact that I'm using qemu emulation to test, but compiling with this patch causes segv in gc_malloc.

Will try without to see what happens.

@jpf91
Collaborator

On ARM only or on x86? I guess this means even a simple hello world segfaults? Sounds exectly like the first symptom related to -fsection-anchors we had found and should be fixed by the static array padding commit.

If you try it on ARM without this test, please also try compilling druntime with -fno-section-anchors, as this really sounds like the same old problem.

But it's weird, I've run hello worlds and more advanced applications (dustmite) with those changes and everything worked fine.

@ibuclaw
Owner
@ibuclaw ibuclaw commented on the diff
gcc/d/d-objfile.cc
((7 lines not shown))
+ //(see ARM issue #120, section anchors)
+ //According to gcc manual, DECL_SIZE is authoritative:
+ HOST_WIDE_INT typesize = int_size_in_bytes (TREE_TYPE (t));
+ HOST_WIDE_INT initsize = int_size_in_bytes (TREE_TYPE (DECL_INITIAL (t)));
+ HOST_WIDE_INT declsize = gen.getTargetSizeConst (DECL_SIZE (t)) / BITS_PER_UNIT;
+
+ if (declsize < initsize)
+ internal_error ("Mismatch between declaration '%s' size (%wd) and it's initializer size (%wd).",
+ sym->prettyIdent ? sym->prettyIdent : sym->Sident,
+ declsize, initsize);
+ else if (declsize != typesize)
+ internal_error ("Mismatch between declaration '%s' size (%wd) and it's type size (%wd).",
+ sym->prettyIdent ? sym->prettyIdent : sym->Sident,
+ declsize, typesize);
+ }
+
@ibuclaw Owner
ibuclaw added a note

This is the only thing itching me at the moment.

This code above: relayout_decl (t); guarantees that declsize == typesize by zero'ing out DECL_SIZE (t) and re-evaluating it. So that leaves the check for declsize < initsize, which can be changed to typesize < initsize.

if (int_size_in_bytes (TREE_TYPE (t))
    < int_size_in_bytes (TREE_TYPE (DECL_INITIAL (t))))
  {
    internal_error ( ... );
  }

#120 references an old ticketing system, so that part of the comment may be removed in the future.

@jpf91 Collaborator
jpf91 added a note

OK, I'll change this as soon as I have access to my main laptop again (which should be this weekend)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ibuclaw ibuclaw commented on the diff
gcc/d/d-decls.cc
@@ -347,6 +347,7 @@
gcc_assert (TREE_CODE (TREE_TYPE (csym->Stree)) == REFERENCE_TYPE);
TREE_TYPE (csym->Stree) = TREE_TYPE (TREE_TYPE (csym->Stree));
TREE_USED (csym->Stree) = 1;
+ layout_decl (csym->Stree, 0);
@ibuclaw Owner
ibuclaw added a note

Do you know what breakage required this? Now I'm mauling it over again, I don't think this is required, as outdata() takes care of setting DECL_SIZE in relayout_decl as per below comment.

@jpf91 Collaborator
jpf91 added a note

I currently don't have access to my ARM board so I can't check this right now, but I think it was related to initializer symbols for type info instances. You could try to comment this change and then run the test suite, the check in outdata should detect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ibuclaw
Owner

Right, I'm gonna stamp my LTGM on this. :-)

@ibuclaw ibuclaw merged commit 21ff1ac into D-Programming-GDC:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 19, 2012
  1. @jpf91

    Fix static array initializers

    jpf91 authored
    partial fix for issue #120 (fsection-anchors)
    
    Pad out the rest of static arrays with single elements.
    Issue #120 - breaks -fsection-anchors on ARM when
    backend calculates field positions for array members.
Commits on Sep 21, 2012
  1. @jpf91

    Set correct DECL_SIZE for TypeInfoDeclaration

    jpf91 authored
    partial fix for issue #120 (fsection-anchors)
  2. @jpf91

    Set correct DECL_SIZE for ModuleInfo and ClassInfo

    jpf91 authored
    partial fix for issue #120 (fsection-anchors)
  3. @jpf91

    Fix TypeInfoStruct initializer

    jpf91 authored
    partial fix for issue #120 (fsection-anchors)
  4. @jpf91

    Fix TypeInfo_Function and TypeInfo_Delegate declarations

    jpf91 authored
    partial fix for issue #120 (fsection-anchors)
  5. @jpf91

    ICE on all issue #120 bugs

    jpf91 authored
    The problems in issue #120 are caused by an initializer
    for a variable with a bigger size than the variables
    DECL_SIZE. Detect all those cases and ICE.
This page is out of date. Refresh to see the latest.
View
9 gcc/d/d-decls.cc
@@ -347,6 +347,7 @@ TypeInfoDeclaration::toSymbol (void)
gcc_assert (TREE_CODE (TREE_TYPE (csym->Stree)) == REFERENCE_TYPE);
TREE_TYPE (csym->Stree) = TREE_TYPE (TREE_TYPE (csym->Stree));
TREE_USED (csym->Stree) = 1;
+ layout_decl (csym->Stree, 0);
@ibuclaw Owner
ibuclaw added a note

Do you know what breakage required this? Now I'm mauling it over again, I don't think this is required, as outdata() takes care of setting DECL_SIZE in relayout_decl as per below comment.

@jpf91 Collaborator
jpf91 added a note

I currently don't have access to my ARM board so I can't check this right now, but I think it was related to initializer symbols for type info instances. You could try to comment this change and then run the test suite, the check in outdata should detect this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
/* DMD makes typeinfo decls one-only by doing: s->Sclass = SCcomdat;
in TypeInfoDeclaration::toObjFile. The difference is that,
@@ -679,9 +680,7 @@ ClassDeclaration::toSymbol (void)
tree decl;
csym = toSymbolX ("__Class", SCextern, 0, "Z");
decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (csym->Sident),
- TREE_TYPE (ClassDeclaration::classinfo != NULL
- ? ClassDeclaration::classinfo->type->toCtype() // want the RECORD_TYPE, not the REFERENCE_TYPE
- : error_mark_node));
+ make_node (RECORD_TYPE));
csym->Stree = decl;
d_keep (decl);
@@ -720,12 +719,10 @@ Module::toSymbol (void)
{
if (!csym)
{
- Type *obj_type = gen.getObjectType();
-
csym = toSymbolX ("__ModuleInfo", SCextern, 0, "Z");
tree decl = build_decl (UNKNOWN_LOCATION, VAR_DECL, get_identifier (csym->Sident),
- TREE_TYPE (obj_type->toCtype())); // want the RECORD_TYPE, not the REFERENCE_TYPE
+ make_node (RECORD_TYPE));
g.ofile->setDeclLoc (decl, this);
csym->Stree = decl;
View
33 gcc/d/d-objfile.cc
@@ -936,8 +936,18 @@ outdata (Symbol *sym)
tree t = check_static_sym (sym);
gcc_assert (t);
- if (sym->Sdt && DECL_INITIAL (t) == NULL_TREE)
- DECL_INITIAL (t) = dt2tree (sym->Sdt);
+ if (sym->Sdt)
+ {
+ if (!COMPLETE_TYPE_P (TREE_TYPE (t)))
+ {
+ size_t fsize = dt_size (sym->Sdt);
+ TYPE_SIZE (TREE_TYPE (t)) = bitsize_int (fsize * BITS_PER_UNIT);
+ TYPE_SIZE_UNIT (TREE_TYPE (t)) = size_int (fsize);
+ }
+
+ if (DECL_INITIAL (t) == NULL_TREE)
+ DECL_INITIAL (t) = dt2tree (sym->Sdt);
+ }
gcc_assert (!g.irs->isErrorMark (t));
@@ -963,6 +973,25 @@ outdata (Symbol *sym)
gcc_assert (!DECL_EXTERNAL (t));
relayout_decl (t);
+ if (DECL_INITIAL (t) != NULL_TREE)
+ {
+ //Initializer must never be bigger than symbol size
+ //(see ARM issue #120, section anchors)
+ //According to gcc manual, DECL_SIZE is authoritative:
+ HOST_WIDE_INT typesize = int_size_in_bytes (TREE_TYPE (t));
+ HOST_WIDE_INT initsize = int_size_in_bytes (TREE_TYPE (DECL_INITIAL (t)));
+ HOST_WIDE_INT declsize = gen.getTargetSizeConst (DECL_SIZE (t)) / BITS_PER_UNIT;
+
+ if (declsize < initsize)
+ internal_error ("Mismatch between declaration '%s' size (%wd) and it's initializer size (%wd).",
+ sym->prettyIdent ? sym->prettyIdent : sym->Sident,
+ declsize, initsize);
+ else if (declsize != typesize)
+ internal_error ("Mismatch between declaration '%s' size (%wd) and it's type size (%wd).",
+ sym->prettyIdent ? sym->prettyIdent : sym->Sident,
+ declsize, typesize);
+ }
+
@ibuclaw Owner
ibuclaw added a note

This is the only thing itching me at the moment.

This code above: relayout_decl (t); guarantees that declsize == typesize by zero'ing out DECL_SIZE (t) and re-evaluating it. So that leaves the check for declsize < initsize, which can be changed to typesize < initsize.

if (int_size_in_bytes (TREE_TYPE (t))
    < int_size_in_bytes (TREE_TYPE (DECL_INITIAL (t))))
  {
    internal_error ( ... );
  }

#120 references an old ticketing system, so that part of the comment may be removed in the future.

@jpf91 Collaborator
jpf91 added a note

OK, I'll change this as soon as I have access to my main laptop again (which should be this weekend)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
g.ofile->outputStaticSymbol (sym);
}
View
17 gcc/d/dfrontend/todt.c
@@ -266,6 +266,8 @@ dt_t *ArrayInitializer::toDt()
dt = val->toDt();
if (dts[length])
error(loc, "duplicate initializations for index %d", length);
+ if (tn->ty == Tsarray)
+ dt = createTsarrayDt(dt, tb->nextOf());
dts[length] = dt;
length++;
}
@@ -313,21 +315,26 @@ dt_t *ArrayInitializer::toDt()
tadim = ta->dim->toInteger();
if (dim < tadim)
{
+
+
+#ifdef IN_GCC
+ // Pad out the rest of the array with single elements.
+ // Issue #120 - breaks -fsection-anchors on ARM when
+ // backend calculates field positions for array members.
+ for (size_t i = dim; i < tadim; i++)
+ pdtend = dtcontainer(pdtend, NULL, sadefault);
+#else
if (edefault->isBool(FALSE))
// pad out end of array
- // (ok for GDC as well)
pdtend = dtnzeros(pdtend, size * (tadim - dim));
else
{
for (size_t i = dim; i < tadim; i++)
-#ifdef IN_GCC
- pdtend = dtcontainer(pdtend, NULL, sadefault);
-#else
{ for (size_t j = 0; j < n; j++)
pdtend = edefault->toDt(pdtend);
}
-#endif
}
+#endif
}
else if (dim > tadim)
{
View
6 gcc/d/dfrontend/typinf.c
@@ -532,8 +532,7 @@ void TypeInfoStructDeclaration::toDt(dt_t **pdt)
const char *name = sd->toPrettyChars();
size_t namelen = strlen(name);
dtsize_t(pdt, namelen);
- //dtabytes(pdt, TYnptr, 0, namelen + 1, name);
- dtxoff(pdt, toSymbol(), offset, TYnptr);
+ dtabytes(pdt, TYnptr, 0, namelen + 1, name);
offset += namelen + 1;
// void[] init;
@@ -709,9 +708,6 @@ void TypeInfoStructDeclaration::toDt(dt_t **pdt)
dtsize_t(pdt, 1); // has pointers
else
dtsize_t(pdt, 0); // no pointers
-
- // name[]
- dtnbytes(pdt, namelen + 1, name);
}
void TypeInfoClassDeclaration::toDt(dt_t **pdt)
View
2  libphobos/libdruntime/object.di
@@ -138,11 +138,13 @@ class TypeInfo_AssociativeArray : TypeInfo
class TypeInfo_Function : TypeInfo
{
TypeInfo next;
+ string deco;
}
class TypeInfo_Delegate : TypeInfo
{
TypeInfo next;
+ string deco;
}
class TypeInfo_Class : TypeInfo
Something went wrong with that request. Please try again.