Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/backend.d
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ extern extern (C++) Type getTypeInfoType(Type t, Scope* sc);
extern extern (C++) Expression getInternalTypeInfo(Type t, Scope* sc);
extern extern (C++) void genObjFile(Module m, bool multiobj);
extern extern (C++) void genhelpers(Module m, bool multiobj);
extern extern (C++) void genHelpersObjFile(Module m);

extern extern (C++) Symbol* toInitializer(AggregateDeclaration sd);
extern extern (C++) Symbol* toModuleArray(Module m);
Expand Down
24 changes: 21 additions & 3 deletions src/glue.c
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,11 @@ void obj_write_deferred(Library *library)
for (size_t i = 0; i < obj_symbols_towrite.dim; i++)
{
Dsymbol *s = obj_symbols_towrite[i];
Module *m = s->getModule();
Module *m;
if (TemplateInstance *ti = s->isTemplateInstance())
m = ti->tempdecl->getModule(); // tweak object file name
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You canmake this an if-else to spare the s->getModule.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

else
m = s->getModule();

char *mname;
if (m)
Expand Down Expand Up @@ -516,11 +520,9 @@ void genhelpers(Module *m, bool iscomdat)
case 2: ma = m->munittest; rt = RTLSYM_DUNITTEST; bc = BCret; break;
default: assert(0);
}

if (!ma)
continue;


localgot = NULL;

// Call dassert(filename, line)
Expand Down Expand Up @@ -560,6 +562,22 @@ void genhelpers(Module *m, bool iscomdat)
}
}

/**************************************
* Bugzilla 14828: If some template instantiations require
* non-root module helpers, they should be placed in
* separated object file.
*/
void genHelpersObjFile(Module *m)
{
assert(!m->isRoot());

lastmname = m->srcfile->toChars();

objmod->initfile(lastmname, NULL, m->toPrettyChars());
genhelpers(m, true);
objmod->termfile();
}

/**************************************
* Search for a druntime array op
*/
Expand Down
103 changes: 87 additions & 16 deletions src/mars.c
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,10 @@ void parseConfFile(StringTable *environment, const char *path, size_t len, unsig

void genObjFile(Module *m, bool multiobj);
void genhelpers(Module *m, bool iscomdat);
void genHelpersObjFile(Module *m);

/** Normalize path by turning forward slashes into backslashes */
const char * toWinPath(const char *src)
const char *toWinPath(const char *src)
{
if (src == NULL)
return NULL;
Expand Down Expand Up @@ -1658,57 +1659,127 @@ Language changes listed by -transition=id:\n\
}
}

if (!global.params.obj)
//printf("global.params.multiobj = %d, oneobj = %d, lib = %d\n",
// global.params.multiobj, global.params.oneobj, global.params.lib);
if (!global.params.obj || !modules.dim)
{
}
else if (global.params.oneobj)
{
if (modules.dim)
obj_start(modules[0]->srcfile->toChars());
/* global.params.oneobj == true:
* Just only one object file is generated for the final link.
* e.g.
* dmd -ofout main.d // main.obj is generated
*/
obj_start(modules[0]->srcfile->toChars());

for (size_t i = 0; i < modules.dim; i++)
{
Module *m = modules[i];
if (global.params.verbose)
fprintf(global.stdmsg, "code %s\n", m->toChars());

genObjFile(m, false);
if (entrypoint && m == rootHasMain)
genObjFile(entrypoint, false);
}
for (size_t i = 0; i < Module::amodules.dim; i++)
for (size_t j = 0; j < Module::amodules.dim; j++)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why j here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In below code blocks, j is used to iterate non-root modules. It's just for the consistency.

{
Module *m = Module::amodules[i];
if (!m->isRoot() && (m->marray || m->massert || m->munittest))
genhelpers(m, true);
Module *mx = Module::amodules[j];
if (mx->isRoot())
continue;
if (!mx->marray && !mx->massert && !mx->munittest)
continue;
genhelpers(mx, true);
}
if (!global.errors && modules.dim)
{

if (!global.errors)
obj_end(library, modules[0]->objfile);
}
}
else
else if (!global.params.multiobj)
{
/* global.params.multiobj == false:
* The object files are generated per source files.
* e.g.
* dmd -c main.d code.d // main.obj and code.obj generated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why foo.d's object code should include helper functions for bar.d.

@WalterBright Here was the problematic part. If global.params.multiobj == false, it's not a problem.
But when global.params.multiobj == true, in the library compilation:

all1_order_flipped:
    ${dmd} -oflibproj6.a -lib -g project2.d foo2.d foo1.d

Both foo2 and foo1 imports stdio, then stdio->importedFrom is set to foo2. And this code has emit the stdio module helpers into the libraried foo2.obj, which also contains foo2.__ModuleInfoZ.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stdio module helpers should be there with the stdio.obj file, along with stdio's moduleinfo. There should be no reason for any other module to generate either.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stdio module helpers should be there with the stdio.obj file, along with stdio's moduleinfo.

Yes, it's still correct. When we compile stdio.d, stdio.obj will contain both non-COMDAT moduleinfo and three helper functions.

There should be no reason for any other module to generate either.

No, it's necessary to fix issue 846. When a user instantiate writeln!(), it will implicitly use the helper stdio.__array for array bounds check. But, if he don't link stdio.obj, it had got the linker error "undefined symbol stdio.__array". In #3552, I've applied those changes to fix 846:

  • All module helper functions had changed to not use ModuleInfo. It was just for getting module file name, so I changed to use bare module file name string as the alternative.
  • After that, each helper functions has changed to not dependent to the corresponding ModuleInfo. When they're required for a non-root template instance, they will be also generated and placed in COMDAT section. Finally, the use of them won't cause link failures, even if the non-COMDAT versions are not linked.

If you want to know the details, please also check the change in #3552. Honestly this PR is a supplemental change of that. Honestly, when I wrote that PR, I didn't think well about the library generation. So the COMDAT helpers had placed in incorrect places. This PR will fix that issue.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so #3552 is where it came from. Thanks for pointing it out. I did not review that PR. I believe it is the wrong solution, however. A very simple solution is to compile the helpers and moduleinfo into stdio.obj. Then, you have to link with stdio.obj. I do not think it should be a surprise to anyone that if they import stdio, they have to link with stdio.obj. (The stdio.obj should be in the library, too, meaning it will get pulled in automatically.)

This solution resolves https://issues.dlang.org/show_bug.cgi?id=846 in a simple, easy to understand, and easy to implement manner. Furthermore, it eliminates all that .obj file bloat I've noticed where the helper functions for every import get generated into every .obj file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, it eliminates all that .obj file bloat I've noticed where the helper functions for every import get generated into every .obj file.

Bloat is an exaggeration, each of those functions only adds about 50 bytes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 3 per import, and then repeated endlessly for each object file. For:

import std.stdio;
void test() { }

the following are generated:

_D6object7__arrayZ      COMDAT flags=x0 attr=x10 align=x0
_D6object8__assertFiZv  COMDAT flags=x0 attr=x10 align=x0
_D6object15__unittest_failFiZv  COMDAT flags=x0 attr=x10 align=x0
_D4core4stdc6stdint7__arrayZ    COMDAT flags=x0 attr=x10 align=x0
_D4core4stdc6stdint8__assertFiZv        COMDAT flags=x0 attr=x10 align=x0
_D4core4stdc6stdint15__unittest_failFiZv        COMDAT flags=x0 attr=x10 align=x0
_D3std8typecons7__arrayZ        COMDAT flags=x0 attr=x10 align=x0
_D3std8typecons8__assertFiZv    COMDAT flags=x0 attr=x10 align=x0
_D3std8typecons15__unittest_failFiZv    COMDAT flags=x0 attr=x10 align=x0
_D3std6traits7__arrayZ  COMDAT flags=x0 attr=x10 align=x0
_D3std6traits8__assertFiZv      COMDAT flags=x0 attr=x10 align=x0
_D3std6traits15__unittest_failFiZv      COMDAT flags=x0 attr=x10 align=x0
_D3std4meta7__arrayZ    COMDAT flags=x0 attr=x10 align=x0
_D3std4meta8__assertFiZv        COMDAT flags=x0 attr=x10 align=x0
_D3std4meta15__unittest_failFiZv        COMDAT flags=x0 attr=x10 align=x0

and:

db      066h,06fh,06fh,000h,063h,03ah,05ch,063h ;foo.c:\c
db      062h,078h,05ch,06dh,061h,072h,073h,05ch ;bx\mars\
db      064h,072h,075h,06eh,074h,069h,06dh,065h ;druntime
db      05ch,069h,06dh,070h,06fh,072h,074h,05ch ;\import\
db      06fh,062h,06ah,065h,063h,074h,02eh,064h ;object.d
db      000h,063h,03ah,05ch,063h,062h,078h,05ch ;.c:\cbx\
db      06dh,061h,072h,073h,05ch,064h,072h,075h ;mars\dru
db      06eh,074h,069h,06dh,065h,05ch,069h,06dh ;ntime\im
db      070h,06fh,072h,074h,05ch,063h,06fh,072h ;port\cor
db      065h,05ch,073h,074h,064h,063h,05ch,073h ;e\stdc\s
db      074h,064h,069h,06eh,074h,02eh,064h,000h ;tdint.d.
db      063h,03ah,05ch,063h,062h,078h,05ch,06dh ;c:\cbx\m
db      061h,072h,073h,05ch,070h,068h,06fh,062h ;ars\phob
db      06fh,073h,05ch,073h,074h,064h,05ch,074h ;os\std\t
db      079h,070h,065h,063h,06fh,06eh,073h,02eh ;ypecons.
db      064h,000h,063h,03ah,05ch,063h,062h,078h ;d.c:\cbx
db      05ch,06dh,061h,072h,073h,05ch,070h,068h ;\mars\ph
db      06fh,062h,06fh,073h,05ch,073h,074h,064h ;obos\std
db      05ch,074h,072h,061h,069h,074h,073h,02eh ;\traits.
db      064h,000h,063h,03ah,05ch,063h,062h,078h ;d.c:\cbx
db      05ch,06dh,061h,072h,073h,05ch,070h,068h ;\mars\ph
db      06fh,062h,06fh,073h,05ch,073h,074h,064h ;obos\std
db      05ch,06dh,065h,074h,061h,02eh,064h,000h ;\meta.d.

used only by those helper functions.

The object file is 2,229 bytes, although the generated code for test(){} is one byte.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generating __unittest_fail is just a bug. I'm not adding improvement for that in this PR, but I can easily kill it.

*/
for (size_t i = 0; i < modules.dim; i++)
{
Module *m = modules[i];
if (global.params.verbose)
fprintf(global.stdmsg, "code %s\n", m->toChars());

obj_start(m->srcfile->toChars());
genObjFile(m, global.params.multiobj);
genObjFile(m, false);
if (entrypoint && m == rootHasMain)
genObjFile(entrypoint, global.params.multiobj);
genObjFile(entrypoint, false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

genObjFile, what a misnomer, it doesn't actually generate a file.

for (size_t j = 0; j < Module::amodules.dim; j++)
{
// todo: This part is not the best for the total size of object files.
// For example:
// When both of main.d and code.d import mx, even if main.obj
// already contains mx->marray, it is stored in code.obj again.
Module *mx = Module::amodules[j];
if (mx != m && mx->importedFrom == m && (mx->marray || mx->massert || mx->munittest))
genhelpers(mx, true);
if (mx == m || mx->importedFrom != m)
continue;
if (!mx->marray && !mx->massert && !mx->munittest)
continue;
genhelpers(mx, true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still emitting weak duplicates of those helper functions into other modules, means it won't solve Issue 14748 for separate compilation.
This indicates a severe flaw of this approach.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then again, other than libraries, objects passed to the linker aren't optional by default, so it might not be a problem.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.

}
obj_end(library, m->objfile);

if (global.errors && !global.params.lib)
m->deleteObjFile();
}
}
else
{
/* global.params.multiobj == true:
* Each compiled symbols are stored in separated doppelganger modules.
* The generated object files have numbered names.
* e.g.
* dmd -lib main.d code.d
* dmd -multiobj main.d code.d
* // main.foo() --> main_1_<hash>.obj
* // main.bar() --> main_2_<hash>.obj
* // code.baz() --> code_3_<hash>.obj
* // std.stdio.writeln!()() --> stdio_4_<hash>.obj
* // std.stdio.__array() --> stdio_5_<hash>.obj (Bugzilla 14828)
* // --> with -lib, all *.obj will be stored in one .lib file.
*/
for (size_t i = 0; i < modules.dim; i++)
{
Module *m = modules[i];
if (global.params.verbose)
fprintf(global.stdmsg, "code %s\n", m->toChars());

obj_start(m->srcfile->toChars());
genObjFile(m, true);
if (entrypoint && m == rootHasMain)
genObjFile(entrypoint, true);
obj_end(library, m->objfile);

obj_write_deferred(library);

if (global.errors && !global.params.lib)
m->deleteObjFile();
}

for (size_t j = 0; j < Module::amodules.dim; j++)
{
Module *mx = Module::amodules[j];
if (mx->isRoot())
continue;
if (!mx->marray && !mx->massert && !mx->munittest)
continue;

obj_start(mx->srcfile->toChars());
genHelpersObjFile(mx);
obj_end(library, mx->objfile);

if (global.errors && !global.params.lib)
mx->deleteObjFile();
}
}

if (global.params.lib && !global.errors)
Expand Down
13 changes: 12 additions & 1 deletion src/template.c
Original file line number Diff line number Diff line change
Expand Up @@ -5912,14 +5912,25 @@ void TemplateInstance::semantic(Scope *sc, Expressions *fargs)
if (errors)
goto Lerror;

if (Module *m = tempdecl->scope->module) // should use getModule() instead?
// todo: should we use getModule() instead?
// --> Yes, it's consistent with the way in glue.c.
if (Module *m = tempdecl->scope->module)
{
// Generate these functions as they may be used
// when template is instantiated in other modules
// even if assertions or bounds checking are disabled in this module
toModuleArray(m);
toModuleAssert(m);
toModuleUnittest(m);

// todo: This is a workaround for the object file generation pass.
// Currently, if m is a root module, this->toObjFile() may not happen
// during m->toObjFile(). For that, we have to request the module
// helpers in the early stage.

// To avoid this hack, we should insert the primary instance 'inst'
// in the member of module m.
// It will be done by: https://github.com/D-Programming-Language/dmd/pull/4784
}

/* See if there is an existing TemplateInstantiation that already
Expand Down
8 changes: 8 additions & 0 deletions test/runnable/extra-files/link14828a.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module link14828a;

void func1()
{
import link14828stdio;

writeln("ok1");
}
6 changes: 6 additions & 0 deletions test/runnable/extra-files/link14828b.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module link14828b;

void func2()
{
import link14828stdio;
}
10 changes: 10 additions & 0 deletions test/runnable/extra-files/link14828c.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
module link14828c;

import link14828a;
import link14828b;

void test()
{
func1();
func2();
}
8 changes: 8 additions & 0 deletions test/runnable/extra-files/link14828d.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
module link14828d;

import link14828c;

void main(string[] args)
{
test();
}
9 changes: 9 additions & 0 deletions test/runnable/extra-files/link14828stdio.d
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
module link14828stdio;

void writeln()(string s)
{
int[4] sa;

sa[s.length] = 1;
// bounds check function (link14828stdio._array)
}
43 changes: 43 additions & 0 deletions test/runnable/link14828.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#!/usr/bin/env bash

src=runnable${SEP}extra-files
dir=${RESULTS_DIR}${SEP}runnable
output_file=${dir}${SEP}link14828.sh.out

rm -f ${output_file}

if [ $OS == "win32" -o $OS == "win64" ]; then
LIBEXT=.lib
else
LIBEXT=.a
fi

srcname=${src}${SEP}link14828
outname=${dir}${SEP}link14828

libname=${outname}x${LIBEXT}
exename=${outname}y${EXE}

# all0_order_flipped:
$DMD -m${MODEL} -I${src} -of${libname} -lib -g ${srcname}c.d ${srcname}a.d ${srcname}b.d || exit 1
$DMD -m${MODEL} -I${src} -of${exename} -g ${libname} ${srcname}a.d ${srcname}d.d || exit 1
${dir}/link14828y || exit 1

# all0:
$DMD -m${MODEL} -I${src} -of${libname} -lib -g ${srcname}c.d ${srcname}b.d ${srcname}a.d || exit 1
$DMD -m${MODEL} -I${src} -of${exename} -g ${libname} ${srcname}a.d ${srcname}d.d || exit 1
${dir}/link14828y || exit 1

# all1:
$DMD -m${MODEL} -I${src} -of${libname} -lib -g ${srcname}c.d ${srcname}a.d ${srcname}b.d || exit 1
$DMD -m${MODEL} -I${src} -of${exename} -g ${libname} ${srcname}b.d ${srcname}d.d || exit 1
${dir}/link14828y || exit 1

# all1_order_flipped:
$DMD -m${MODEL} -I${src} -of${libname} -lib -g ${srcname}c.d ${srcname}b.d ${srcname}a.d || exit 1
$DMD -m${MODEL} -I${src} -of${exename} -g ${libname} ${srcname}b.d ${srcname}d.d || exit 1
${dir}/link14828y || exit 1

rm ${libname} ${exename} ${outname}y${OBJ}

echo Success > ${output_file}