Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix Issue 12146 - Linker error with __xopCmp, __xopEq, TypeInfo #3255

Closed
wants to merge 1 commit into from

Conversation

MartinNowak
Copy link
Member

  • The root cause for this bug was, that the TypeInfo
    for structs can only be generated after semantic3 for
    that struct was run. This is not possible if the TypeInfo
    is needed during the obj generation pass, e.g. because
    it is referenced by an array TypeInfo.
  • Fixed by eagerly emitting TypeInfo for structs,
    this is similar to TypeInfos for classes.

@MartinNowak MartinNowak restored the fix12146 branch February 14, 2014 00:40
@MartinNowak
Copy link
Member Author

@9rnsr can you have a look at this?

@9rnsr
Copy link
Contributor

9rnsr commented Feb 14, 2014

No, the root cause is that semantic3 is not run for StructDeclaration::xeq and xcmp in certain cases. Even if a struct is in non-root module, we should invoke their semantic3 to generate correct TypeInfo object. To run the additional analysis, Type::getTypeinfo will register deferred semantic3 for such non-root structs.

On the other hand, TypeInfo data itself is correctly generated today. So this is not a solution.

- The root cause for this bug was, that the TypeInfo
  for structs can only be generated after semantic3 for
  that struct was run. This is not possible if the TypeInfo
  is needed during the obj generation pass, e.g. because
  it is referenced by an array TypeInfo.

- Fixed by eagerly emitting TypeInfo for structs,
  this is similar to TypeInfos for classes.
@MartinNowak
Copy link
Member Author

Even if a struct is in non-root module, we should invoke their semantic3 to generate correct TypeInfo object. To run the additional analysis, Type::getTypeinfo will register deferred semantic3 for such non-root structs.

I could not use deferred semantic3, because the TypeInfo for the struct was only needed during obj file generation.
Even if there was a way to run deferred semantic3, there is no compelling reason to use COMDATs and vague linkage for struct TypeInfos. Just emit the TypeInfo with the struct once and there is no need to run semantic3 when the struct is imported.

@rainers
Copy link
Member

rainers commented Feb 14, 2014

If the type info is only generated with the struct declaration, you will have to compile deimos modules to a library and not be able to use them as "header" files only.

@MartinNowak
Copy link
Member Author

I think you're right, but we should think about it thoroughly.
The failing test case for example is ridiculous, the linker complains that it can't find the TypeInfo for a predefined struct. Now what TypeInfo do we generate normally?

@MartinNowak
Copy link
Member Author

@MartinNowak
Copy link
Member Author

IMO we should only weakly link against TypeInfo. Then if it isn't defined as in the Deimos case you'll get a null reference. I really don't see why we would generate RTTI for imported types.
Alternatively to null references we could weakly link against a bare minimal TypeInfo.

@rainers
Copy link
Member

rainers commented Feb 14, 2014

http://dpaste.dzfl.pl/6210dab32b1d

I'm not sure what this is showing? That the error message for typeid of an undefined struct is misleading?

@rainers
Copy link
Member

rainers commented Feb 14, 2014

IMO we should only weakly link against TypeInfo.

That won't work. You need it for example if you create an dynamic array of a struct type declared in deimos.

It should be fine if the struct definition outputs another COMDAT if the containing module is compiled. We just have to guarantee that only one instance is generated in into the object file. Optlink accepts that, but the MS linker does not.

@MartinNowak
Copy link
Member Author

I think we can solve this, when a struct doesn't require linkage, we already know it's TypeInfo (no eq, cmp, hash, whatsoever), no init {all zero), no rtInfo, just it's size and maybe the name.
This will suffice for all runtime interfaces.

@rainers
Copy link
Member

rainers commented Feb 15, 2014

You must not generate a "false" type info, because if a different module requires and generates the full type info, the linker will pick any of the generated COMDATs, and it could be the "false" version.

With the precise GC, any TypeInfo needs RTInfo, so the minimal TypeInfo will never be good enough.

@MartinNowak
Copy link
Member Author

Full TypeInfo would not be a COMDAT but global.
We already generate false TypeInfo for forward declared structs.

With the precise GC, any TypeInfo needs RTInfo, so the minimal TypeInfo will never be good enough.

One could regress to conservative scanning though, but that's not optimal of course.
Then OTOH to allocate a struct, the compiler needs to know the size, so that inevitably requires semantic analysis (not sure if pass 3 is required).

@rainers
Copy link
Member

rainers commented Feb 15, 2014

Full TypeInfo would not be a COMDAT but global.

I suspect that some linker will complain if both exist for the same symbol.

We already generate false TypeInfo for forward declared structs.

That's a bug, not a feature.

One could regress to conservative scanning though, but that's not optimal of course.

That's actually done in the precise GC with the current fallback in the compiler to generate 0/1 instead of proper RTInfo. But that doesn't help if you want other stuff in RTInfo aswell.

Then OTOH to allocate a struct, the compiler needs to know the size, so that inevitably requires semantic analysis (not sure if pass 3 is required).

Calling runtime function, e.g. for array appending and comparing, also needs type info, and just the size isn't always enough.

@MartinNowak
Copy link
Member Author

Calling runtime function, e.g. for array appending and comparing, also needs type info, and just the size isn't always enough.

They are zero initialized and don't implement any x... function, so everything required is already known.

@MartinNowak
Copy link
Member Author

I suspect that some linker will complain if both exist for the same symbol.

Why and which linker? It's very common to define weak fallbacks that are optionally replaced with a strong symbol.

@rainers
Copy link
Member

rainers commented Feb 16, 2014

Why and which linker? It's very common to define weak fallbacks that are optionally replaced with a strong symbol.

The MS linker for example:

//testc.c:
int fun() { return 2; } // writes to section .text$mn (what's the $mn?)
// testd.d
extern (C) int fun() { return 1; } // generates COMDAT
int main() { return fun(); }
>$VCINSTALLDIR)bin\amd64\cl.exe /c testc.c
>dmd -m64 testd.d testc.obj
testc.obj : error LNK2005: fun already defined in testd.obj
testd.exe : fatal error LNK1169: one or more multiply defined symbols found

@MartinNowak
Copy link
Member Author

// writes to section .text$mn (what's the $mn?)

The $mn determines the section order, not sure why it's needed here.

extern (C) int fun() { return 1; } // generates COMDAT

That not a COMDAT, right?

How about this instead?

pragma(mangle, "fun") void fun(T)() {}
alias funi = fun!int;

@rainers
Copy link
Member

rainers commented Feb 16, 2014

That not a COMDAT, right?

I checked with dumpbin, it is a COMDAT.

@MartinNowak
Copy link
Member Author

Weird that it's a COMDAT.
In any case, OMF, COFF, ELF and MACH-O support the notion of weak definitions, so we could implement this if we think it's the right approach.
I was talking about minimal TypeInfo before, but for structs, that do not require linkage it would be the same, except for the RTInfo.
For forward defined structs the correct solution would be to emit weak undefined symbols, that is the TypeInfo is null unless defined elsewhere.

@WalterBright
Copy link
Member

@MartinNowak optlink has problems with weak definitions, I'd rather avoid them at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants