Feedback required on proper dllexport/import implementation

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

Feedback required on proper dllexport/import implementation

Nico Rieck
Hello,

while improving and extending support for dllexport/import I have
noticed that the current way these are implemented is problematic and I
would like some input on how to proceed.

Currently dllexport/dllimport is treated as linkage type. This conflicts
with inlined functions because there is no linkage for the combination
of both. On first though, combining both doesn't make sense, but take
this class as an example:

  struct __declspec(dllexport/dllimport) X {
    X() {}
  };

Such constructs with empty or simple inline functions can be found for
example in Microsoft headers or even libc++ headers.

Ignoring the dllexport/import attribute for such functions would seem
most sensible (and can be implemented easily), but MSVC does the
opposite: For imported inline functions the function body is dropped and
__imp_ calls are emitted, and exported inline functions are placed into
COMDAT sections. The latter cannot be expressed because it requires
linkonce_odr and dllexport linkage.

The question now is how to implement this. After a brief discussion, I
can think of four ways:

1. Add additional linkage type(s) for the combinations to
   GlobalValue::LinkageTypes.

   This appears to be the least invasive way, but adds new linkage types
   to an already large list.

2. Handle dllexport/import similar to ELF visibility by adding new
   "visibility" types to GlobalValue::VisibilityTypes and IR visibility
   styles.

   This feels like kind of a band-aid. While dllexport could be
   construed as similar to default visibility (some code uses both in
   the same place depending on platform), dllimport feels wrong here.
   This would also prevent mixing ELF visibility with dllexport/import.

   The size of GlobalValue can be kept the same by simply adjusting the
   bit-fields for linkage and visibility.

3. Add a new enum for dllimport/export to GlobalValue and IR global
   variables and functions, similar to ELF visibility.

   This is similar to (2.), without the awkward piggybacking on
   visibility. But it requires extensions to IR just for a single
   platform.

   The size of GlobalValue can be kept the same by simply adjusting the
   bit-fields.

4. Handle dllexport/import as platform-specific IR function attributes.

   Because dllexport/import can also apply to globals which have no
   attributes, this requires keeping the dllexport/import linkage types
   just for them.

   Inline does not apply to globals, but MSVC can actually produce
   initialized dllexported globals, placed in COMDAT sections, with
   __declspec(selectany). I have no idea if anyone actually does this.
   LLVM also does not support __declspec(selectany) yet.

There may be even more or better ways to implement this. It may be good
to keep a future implementation of __declspec(selectany) in mind when
thinking about this issue.


-Nico

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Reid Kleckner-2
On Mon, Mar 25, 2013 at 9:43 PM, Nico Rieck <[hidden email]> wrote:
Hello,

while improving and extending support for dllexport/import I have
noticed that the current way these are implemented is problematic and I
would like some input on how to proceed.

Currently dllexport/dllimport is treated as linkage type. This conflicts
with inlined functions because there is no linkage for the combination
of both. On first though, combining both doesn't make sense, but take
this class as an example:

  struct __declspec(dllexport/dllimport) X {
    X() {}
  };

Such constructs with empty or simple inline functions can be found for
example in Microsoft headers or even libc++ headers.

Ignoring the dllexport/import attribute for such functions would seem
most sensible (and can be implemented easily), but MSVC does the
opposite: For imported inline functions the function body is dropped and
__imp_ calls are emitted, and exported inline functions are placed into
COMDAT sections. The latter cannot be expressed because it requires
linkonce_odr and dllexport linkage.

Hang on, to me the docs seem to say the dllimport case is slightly different:

dllexport: May be inlined, but always gets instantiated as COMDAT and ultimately gets exported after linking.

dllimport: May be inlined.  If inlining fails, an import-style call (__imp_*) is emitted.

Basically, it avoids COMDAT cruft when we can be sure a definition will be provided by some imported dll.  That feels like a quality of implementation optimization.  I suppose someone could be using /Ob0 or -fno-inline to ignore the definition from the header file and always get the import, but that seems like a corner case.
 
The question now is how to implement this. After a brief discussion, I
can think of four ways:

1. Add additional linkage type(s) for the combinations to
   GlobalValue::LinkageTypes.

   This appears to be the least invasive way, but adds new linkage types
   to an already large list.

2. Handle dllexport/import similar to ELF visibility by adding new
   "visibility" types to GlobalValue::VisibilityTypes and IR visibility
   styles.

   This feels like kind of a band-aid. While dllexport could be
   construed as similar to default visibility (some code uses both in
   the same place depending on platform), dllimport feels wrong here.
   This would also prevent mixing ELF visibility with dllexport/import.

   The size of GlobalValue can be kept the same by simply adjusting the
   bit-fields for linkage and visibility.

3. Add a new enum for dllimport/export to GlobalValue and IR global
   variables and functions, similar to ELF visibility.

   This is similar to (2.), without the awkward piggybacking on
   visibility. But it requires extensions to IR just for a single
   platform.

   The size of GlobalValue can be kept the same by simply adjusting the
   bit-fields.

4. Handle dllexport/import as platform-specific IR function attributes.

   Because dllexport/import can also apply to globals which have no
   attributes, this requires keeping the dllexport/import linkage types
   just for them.

   Inline does not apply to globals, but MSVC can actually produce
   initialized dllexported globals, placed in COMDAT sections, with
   __declspec(selectany). I have no idea if anyone actually does this.
   LLVM also does not support __declspec(selectany) yet.

There may be even more or better ways to implement this. It may be good
to keep a future implementation of __declspec(selectany) in mind when
thinking about this issue.

I'm not super familiar with this code, but I think approach 1 is most consistent with the existing code.  There are already a handful of linkage types that represent combinations of properties (weak, odr, external, internal, etc).  It kind of limits the number of linkage combinations that LLVM needs to think about or support. 

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Anton Korobeynikov-2
In reply to this post by Nico Rieck
Hi Nico,

We already discussed this on IRC, I just wanted to mention something explicitly.

> 1. Add additional linkage type(s) for the combinations to
>    GlobalValue::LinkageTypes.
>
>    This appears to be the least invasive way, but adds new linkage types
>    to an already large list.
It also will require modifying all the existing code wrt linkonce
semantics of "dllexpport_odr" and "dllimport_odr" stuff. Which might
be nontrivial (it's hard to say anything in advance).

After all dllexport is basically the same as external linkage and
dllimport - as  well (but just decl). So, instead of adding new
entities here we'd think about the overall picture once again and make
dllexport / dllimport as some sort of flags?

Target-specific attributes seems the best solution here, but they
cannot be applied to globals. Maybe we'd think into this direction?

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Nico Rieck
On 27.03.2013 21:39, Anton Korobeynikov wrote:
> Target-specific attributes seems the best solution here, but they
> cannot be applied to globals. Maybe we'd think into this direction?

I agree. I actually was thinking of this earlier. Globals already have a
comma-separated list for additional properties (namely section and
alignment), but nothing easily extensible like function attributes. This
could be extended for dllexport/import.
This begs the question whether Globals should have proper attributes
like functions. Are there other things this might be beneficial for?

-Nico
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Gao, Yunzhong
In reply to this post by Nico Rieck
Hi Nico, Reid, and Anton,

I missed the discussion when I implemented dllexport/dllimport for our local tree. I
essentially implemented your approach#1. I was trying to avoid the various
external_linkage + some_attribute approaches because it seems that external_linkage
would imply the external linkage without the dllimport/dllexport semantics, and there
may be existing compiler codes that rely on this semantics. If I used external_linkage
+ some attribute, then any place in existing codes that uses hasExternalLInkage() will
have to be modified to check both the linkage type as well as the attribute list, which
seems a bit fragile to maintain; somebody somewhere will forget to check the attributes
when writing their new optimization or garbage collection passes.

I think dllimport inline functions are more closely related to the available_externally
linkage than linkonce_odr; no COMDAT is involved and no .linkonce discard directive
is needed. I would name it dllimport_inline or something similar.

While it makes sense to mimic MSVC behavior and MSDN doc for C++ codes, it is
not very clear to me what should be the correct semantics when a C99 inline function
carries a dllimport/dllexport declspec. For example,

inline __declspec(dllimport) int foo() { return 100; }
extern inline int foo();

Currently clang ignores the dllimport attribute, so what's left will be:
inline int foo() { return 100; }
extern inline int foo(); // this declaration demands an out-of-line definition

The second declaration with the "extern" keyword demands that foo() have an out-
of-line function definition. If foo() is actually being dllexported from a DLL, there will be
duplicate definitions at load time (I think some calls will resolve to one symbol and others
will resolve to a different symbol foo).

If clang wants to respect the dllimport attribute, then the two declarations would have a
conflict regarding whether a function body should be generated/externally visible.

An alternative is to generate an error message whenever a C99 inline function is used
with a dllimport/dllexport declspec.

In a similar manner, I wonder what should be the correct semantics of:
inline __declspec(dllexport) int foo() { return 100; }

I guess I could let dllexport imply "extern", so that an out-of-line definition always gets
generated, but is that the expected behavior?

I built a mingw compiler from this following revision and asked it to compile three C files.
i386-mingw32-gcc (GCC) 4.9.0 20130416 (experimental)

For the following test case, GCC ignores the dllimport attribute with a warning (similar
to clang's current behavior)

/* test1.c */
inline __declspec(dllimport) void foo();
inline void foo();
inline void foo() { }
void foo_user() { foo(); }
/* end of file */

But for the following two cases, GCC produces both an out-of-line definition and also a
reference to an dllimported symbol foo with no diagnostics. It seems wrong, unless
there is some problem with my build process.

/* test2.c */
inline void foo();
__declspec(dllimport) void foo();
inline void foo() { }
void foo_user() { foo(); }
/* end of file */

/* test3.c */
inline void foo();
__declspec(dllimport) void foo();
__declspec(dllimport) void foo() { } // clang complains that this is a redeclaration without
                                                        // "dllimport" attribute", which seems wrong...
void foo_user() { foo(); }
/* end of file */

What are your thoughts?
- Gao.

________________________________________
From: [hidden email] [[hidden email]] on behalf of Nico Rieck [[hidden email]]
Sent: Monday, March 25, 2013 9:43 PM
To: LLVM Developers Mailing List
Subject: [LLVMdev] Feedback required on proper dllexport/import implementation

Hello,

while improving and extending support for dllexport/import I have
noticed that the current way these are implemented is problematic and I
would like some input on how to proceed.

Currently dllexport/dllimport is treated as linkage type. This conflicts
with inlined functions because there is no linkage for the combination
of both. On first though, combining both doesn't make sense, but take
this class as an example:

  struct __declspec(dllexport/dllimport) X {
    X() {}
  };

Such constructs with empty or simple inline functions can be found for
example in Microsoft headers or even libc++ headers.

Ignoring the dllexport/import attribute for such functions would seem
most sensible (and can be implemented easily), but MSVC does the
opposite: For imported inline functions the function body is dropped and
__imp_ calls are emitted, and exported inline functions are placed into
COMDAT sections. The latter cannot be expressed because it requires
linkonce_odr and dllexport linkage.

The question now is how to implement this. After a brief discussion, I
can think of four ways:

1. Add additional linkage type(s) for the combinations to
   GlobalValue::LinkageTypes.

   This appears to be the least invasive way, but adds new linkage types
   to an already large list.

2. Handle dllexport/import similar to ELF visibility by adding new
   "visibility" types to GlobalValue::VisibilityTypes and IR visibility
   styles.

   This feels like kind of a band-aid. While dllexport could be
   construed as similar to default visibility (some code uses both in
   the same place depending on platform), dllimport feels wrong here.
   This would also prevent mixing ELF visibility with dllexport/import.

   The size of GlobalValue can be kept the same by simply adjusting the
   bit-fields for linkage and visibility.

3. Add a new enum for dllimport/export to GlobalValue and IR global
   variables and functions, similar to ELF visibility.

   This is similar to (2.), without the awkward piggybacking on
   visibility. But it requires extensions to IR just for a single
   platform.

   The size of GlobalValue can be kept the same by simply adjusting the
   bit-fields.

4. Handle dllexport/import as platform-specific IR function attributes.

   Because dllexport/import can also apply to globals which have no
   attributes, this requires keeping the dllexport/import linkage types
   just for them.

   Inline does not apply to globals, but MSVC can actually produce
   initialized dllexported globals, placed in COMDAT sections, with
   __declspec(selectany). I have no idea if anyone actually does this.
   LLVM also does not support __declspec(selectany) yet.

There may be even more or better ways to implement this. It may be good
to keep a future implementation of __declspec(selectany) in mind when
thinking about this issue.


-Nico

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Reid Kleckner-2
On Tue, Apr 23, 2013 at 1:10 PM, Gao, Yunzhong <[hidden email]> wrote:

> Hi Nico, Reid, and Anton,
>
> I missed the discussion when I implemented dllexport/dllimport for our local tree. I
> essentially implemented your approach#1. I was trying to avoid the various
> external_linkage + some_attribute approaches because it seems that external_linkage
> would imply the external linkage without the dllimport/dllexport semantics, and there
> may be existing compiler codes that rely on this semantics. If I used external_linkage
> + some attribute, then any place in existing codes that uses hasExternalLInkage() will
> have to be modified to check both the linkage type as well as the attribute list, which
> seems a bit fragile to maintain; somebody somewhere will forget to check the attributes
> when writing their new optimization or garbage collection passes.
>
> I think dllimport inline functions are more closely related to the available_externally
> linkage than linkonce_odr; no COMDAT is involved and no .linkonce discard directive
> is needed. I would name it dllimport_inline or something similar.

Ah, you're right.  dllimport_inline is basically available_externally,
with an indirect call through [__imp_foo] if we choose not to inline.

> While it makes sense to mimic MSVC behavior and MSDN doc for C++ codes, it is
> not very clear to me what should be the correct semantics when a C99 inline function
> carries a dllimport/dllexport declspec. For example,
>
> inline __declspec(dllimport) int foo() { return 100; }
> extern inline int foo();
>
> Currently clang ignores the dllimport attribute, so what's left will be:
> inline int foo() { return 100; }
> extern inline int foo(); // this declaration demands an out-of-line definition
>
> The second declaration with the "extern" keyword demands that foo() have an out-
> of-line function definition. If foo() is actually being dllexported from a DLL, there will be
> duplicate definitions at load time (I think some calls will resolve to one symbol and others
> will resolve to a different symbol foo).
>
> If clang wants to respect the dllimport attribute, then the two declarations would have a
> conflict regarding whether a function body should be generated/externally visible.
>
> An alternative is to generate an error message whenever a C99 inline function is used
> with a dllimport/dllexport declspec.

Yep, diagnosing this sounds best.  MSVC doesn't support C99 inline so
there shouldn't be much of this kind of code out there.

> In a similar manner, I wonder what should be the correct semantics of:
> inline __declspec(dllexport) int foo() { return 100; }
>
> I guess I could let dllexport imply "extern", so that an out-of-line definition always gets
> generated, but is that the expected behavior?

Yes, I believe dllexport requires that we instantiate a foo that gets
linked once, and is also exported, like C99 extern inline.

> I built a mingw compiler from this following revision and asked it to compile three C files.
> i386-mingw32-gcc (GCC) 4.9.0 20130416 (experimental)
>
> For the following test case, GCC ignores the dllimport attribute with a warning (similar
> to clang's current behavior)
>
> /* test1.c */
> inline __declspec(dllimport) void foo();
> inline void foo();
> inline void foo() { }
> void foo_user() { foo(); }
> /* end of file */
>
> But for the following two cases, GCC produces both an out-of-line definition and also a
> reference to an dllimported symbol foo with no diagnostics. It seems wrong, unless
> there is some problem with my build process.
>
> /* test2.c */
> inline void foo();
> __declspec(dllimport) void foo();
> inline void foo() { }
> void foo_user() { foo(); }
> /* end of file */
>
> /* test3.c */
> inline void foo();
> __declspec(dllimport) void foo();
> __declspec(dllimport) void foo() { } // clang complains that this is a redeclaration without
>                                                         // "dllimport" attribute", which seems wrong...
> void foo_user() { foo(); }
> /* end of file */
>
> What are your thoughts?
> - Gao.
>
> ________________________________________
> From: [hidden email] [[hidden email]] on behalf of Nico Rieck [[hidden email]]
> Sent: Monday, March 25, 2013 9:43 PM
> To: LLVM Developers Mailing List
> Subject: [LLVMdev] Feedback required on proper dllexport/import implementation
>
> Hello,
>
> while improving and extending support for dllexport/import I have
> noticed that the current way these are implemented is problematic and I
> would like some input on how to proceed.
>
> Currently dllexport/dllimport is treated as linkage type. This conflicts
> with inlined functions because there is no linkage for the combination
> of both. On first though, combining both doesn't make sense, but take
> this class as an example:
>
>   struct __declspec(dllexport/dllimport) X {
>     X() {}
>   };
>
> Such constructs with empty or simple inline functions can be found for
> example in Microsoft headers or even libc++ headers.
>
> Ignoring the dllexport/import attribute for such functions would seem
> most sensible (and can be implemented easily), but MSVC does the
> opposite: For imported inline functions the function body is dropped and
> __imp_ calls are emitted, and exported inline functions are placed into
> COMDAT sections. The latter cannot be expressed because it requires
> linkonce_odr and dllexport linkage.
>
> The question now is how to implement this. After a brief discussion, I
> can think of four ways:
>
> 1. Add additional linkage type(s) for the combinations to
>    GlobalValue::LinkageTypes.
>
>    This appears to be the least invasive way, but adds new linkage types
>    to an already large list.
>
> 2. Handle dllexport/import similar to ELF visibility by adding new
>    "visibility" types to GlobalValue::VisibilityTypes and IR visibility
>    styles.
>
>    This feels like kind of a band-aid. While dllexport could be
>    construed as similar to default visibility (some code uses both in
>    the same place depending on platform), dllimport feels wrong here.
>    This would also prevent mixing ELF visibility with dllexport/import.
>
>    The size of GlobalValue can be kept the same by simply adjusting the
>    bit-fields for linkage and visibility.
>
> 3. Add a new enum for dllimport/export to GlobalValue and IR global
>    variables and functions, similar to ELF visibility.
>
>    This is similar to (2.), without the awkward piggybacking on
>    visibility. But it requires extensions to IR just for a single
>    platform.
>
>    The size of GlobalValue can be kept the same by simply adjusting the
>    bit-fields.
>
> 4. Handle dllexport/import as platform-specific IR function attributes.
>
>    Because dllexport/import can also apply to globals which have no
>    attributes, this requires keeping the dllexport/import linkage types
>    just for them.
>
>    Inline does not apply to globals, but MSVC can actually produce
>    initialized dllexported globals, placed in COMDAT sections, with
>    __declspec(selectany). I have no idea if anyone actually does this.
>    LLVM also does not support __declspec(selectany) yet.
>
> There may be even more or better ways to implement this. It may be good
> to keep a future implementation of __declspec(selectany) in mind when
> thinking about this issue.
>
>
> -Nico
>
> _______________________________________________
> LLVM Developers mailing list
> [hidden email]         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Nico Rieck
In reply to this post by Gao, Yunzhong
On 23.04.2013 19:10, Gao, Yunzhong wrote:
> I missed the discussion when I implemented dllexport/dllimport for our local tree. I
> essentially implemented your approach#1. I was trying to avoid the various
> external_linkage + some_attribute approaches because it seems that external_linkage
> would imply the external linkage without the dllimport/dllexport semantics, and there
> may be existing compiler codes that rely on this semantics. If I used external_linkage
> + some attribute, then any place in existing codes that uses hasExternalLInkage() will
> have to be modified to check both the linkage type as well as the attribute list, which
> seems a bit fragile to maintain; somebody somewhere will forget to check the attributes
> when writing their new optimization or garbage collection passes.

What problems do you expect with another approach? I have a local
prototype that removes dllimport/export as linkage and uses function
attributes and an extension to globals. I decorate dllexported functions
with the Used attribute and it seems to work fine. This allows such
functions to have linkonce_odr and weak_odr linkage and not be discarded.

> I think dllimport inline functions are more closely related to the available_externally
> linkage than linkonce_odr; no COMDAT is involved and no .linkonce discard directive
> is needed. I would name it dllimport_inline or something similar.

Hm, I don't think so, because dllimported inline functions must never be
instantiated, or else you get duplicated symbols. They are only allowed
to be expanded. At the moment I solve this by checking for definitions
during codegen (this means they survived the inliner) and dropping their
body.

> While it makes sense to mimic MSVC behavior and MSDN doc for C++ codes, it is
> not very clear to me what should be the correct semantics when a C99 inline function
> carries a dllimport/dllexport declspec. [...]
> An alternative is to generate an error message whenever a C99 inline function is used
> with a dllimport/dllexport declspec.

I agree. This should be rejected.

> In a similar manner, I wonder what should be the correct semantics of:
> inline __declspec(dllexport) int foo() { return 100; }
>
> I guess I could let dllexport imply "extern", so that an out-of-line definition always gets
> generated, but is that the expected behavior?

Yes, such a function is always instanciated and exported.

> For the following test case, GCC ignores the dllimport attribute with a warning (similar
> to clang's current behavior)
>
> /* test1.c */
> inline __declspec(dllimport) void foo();
> inline void foo();
> inline void foo() { }
> void foo_user() { foo(); }
> /* end of file */

Generally, MSVC allows a dllimport definition if the first declaration
is specified as inline. This test case should import foo() (or expand it
inline).

> But for the following two cases, GCC produces both an out-of-line definition and also a
> reference to an dllimported symbol foo with no diagnostics. It seems wrong, unless
> there is some problem with my build process.
>
> /* test2.c */
> inline void foo();
> __declspec(dllimport) void foo();
> inline void foo() { }
> void foo_user() { foo(); }
> /* end of file */

Having both seems wrong, as it leads to linker errors. MSVC rejects this
code because it does not allow redeclaring a function and adding dllimport.

> /* test3.c */
> inline void foo();
> __declspec(dllimport) void foo();
> __declspec(dllimport) void foo() { } // clang complains that this is a redeclaration without
>                                                          // "dllimport" attribute", which seems wrong...
> void foo_user() { foo(); }
> /* end of file */

Same as above.

> What are your thoughts?

I have one issue in mind where I wonder if it's beneficial to be less
strict than MSVC. As stated before, MSVC requires the inline keyword
together with dllimport. This extends to member functions and prevents
out-of-line definitions with inline:

   struct X1 {
     inline void foo();
     void bar();
   };
   void X::foo() {} // OK
   inline void X::bar() {} // Error

Same goes for class and function templates. I stumbled on this in libc++
where internal class templates are externed.


-Nico
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Gao, Yunzhong
Hi Nico,
Thank you for your feedback. I'll try to answer your questions below.

> What problems do you expect with another approach? I have a local
> prototype that removes dllimport/export as linkage and uses function
> attributes and an extension to globals. I decorate dllexported functions with
> the Used attribute and it seems to work fine. This allows such functions to
> have linkonce_odr and weak_odr linkage and not be discarded.

My primary concern is what needs to be added to preserve current
compiler behavior where necessary. All the other approaches appear
to involve adding flags or attribute to an existing linkage type, so any
existing compiler codes that deal with that particular linkage type will
potentially have different behavior. For example, if you replace the
DLLExportLinkage with ExternalLinkage+DLLExport_attribute, then
codes that check hasExternalLinkage() will affect DLLExportLinkage
symbols as well, which may or may not be the right thing. For example,
LTO::addDefinedSymbol() currently does not handle dllimport/dllexport
symbols, but if you change the semantics of ExternalLinkage to include
dllimport/dllexport, then you could potentially have changed LTO
(although, that is not necessarily a bad thing; you just need to check
those places to make sure the compiler behavior still makes sense after
your change). And some contributors who have not caught up with
your change may still submit patches with the old semantics in mind,
which seems prone to errors.

If you really like your approach, I wonder if it makes sense to rename
the existing linkage type to something different, e.g., OldLinkage to
NewLinkage, so that future patches using hasOldLinkage() will have
to check their codes to make sure they work with the new semantics.

> Hm, I don't think so, because dllimported inline functions must never be
> instantiated, or else you get duplicated symbols. They are only allowed to be
> expanded. At the moment I solve this by checking for definitions during
> codegen (this means they survived the inliner) and dropping their body.

I agree. Hmm that is exactly why I said that dllimport inline functions bear
some similarity to the existing available_externally linkage. Maybe some
miscommunication has happened? I will make a guess as to what you
were saying.

Do you mean that you are dropping the function definition during clang
codegen instead of llvm backend? If the function definition does not
appear in the IR file, then the inliner won't be able to inline-expand the
function at call site, which seems to defeat the purpose of having an
inline definition. I solve this by having a new linkage type which works
just like available_externally (only for inline expansion but not for
out-of-line instantiation).

If I guessed wrong, please tell more details about your approach.


> > /* test1.c */
> > inline __declspec(dllimport) void foo(); inline void foo(); inline
> > void foo() { } void foo_user() { foo(); }
> > /* end of file */
>
> Generally, MSVC allows a dllimport definition if the first declaration is
> specified as inline. This test case should import foo() (or expand it inline).

These examples are meant to demonstrate the theoretical issues when
C99 inline is used with dllexport/dllimport. MSVC does not support C99, so I
am not sure MSVC behavior is relevant. It appears to me that MSVC will use
C++ inline semantics even when compiling C files. It appears that both you
and Reid are okay with the approach of issuing an error message whenever a
C99 inline function is used with dllimport. You also seem okay with generating
an out-of-line, dllexported, definition when a C99 inline function is used with
dllexport.

> I have one issue in mind where I wonder if it's beneficial to be less strict than
> MSVC. As stated before, MSVC requires the inline keyword together with
> dllimport. This extends to member functions and prevents out-of-line
> definitions with inline:
>
>    struct X1 {
>      inline void foo();
>      void bar();
>    };
>    void X::foo() {} // OK
>    inline void X::bar() {} // Error
>
> Same goes for class and function templates. I stumbled on this in libc++
> where internal class templates are externed.

I guess you meant
__declspec(dllimport) struct X1 {
  inline void foo();
  void bar();
};
And you meant to have this declaration in a C++ file instead of a C file.

I have no objection to adding support for this usage.

- Gao.



_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Anton Korobeynikov-2
> My primary concern is what needs to be added to preserve current
> compiler behavior where necessary. All the other approaches appear
> to involve adding flags or attribute to an existing linkage type, so any
> existing compiler codes that deal with that particular linkage type will
> potentially have different behavior.
Right. But normally LLVM API does not provide backward compatibility.
The point is to make good code / API, not to fix the broken one with
hinges and supports.

> DLLExportLinkage with ExternalLinkage+DLLExport_attribute, then
> codes that check hasExternalLinkage() will affect DLLExportLinkage
> symbols as well, which may or may not be the right thing.
In this case everything should work fine. If the behavior will be
changed, then we'll find some subtle bug hiding there for ages. normal
dllexport'ed functions are essentially external functions with some
additional semantics.

 > LTO::addDefinedSymbol() currently does not handle dllimport/dllexport
> symbols, but if you change the semantics of ExternalLinkage to include
> dllimport/dllexport, then you could potentially have changed LTO
> (although, that is not necessarily a bad thing; you just need to check
> those places to make sure the compiler behavior still makes sense after
> your change).
Right. The change should be thoroughly tested. Some fallout from such
invasive change is a normal thing, really.

> And some contributors who have not caught up with
> your change may still submit patches with the old semantics in mind,
> which seems prone to errors.
This is irrelevant here. They need to submit patches against ToT, so,
they will need to submit already "new" patches.

> If you really like your approach, I wonder if it makes sense to rename
> the existing linkage type to something different, e.g., OldLinkage to
> NewLinkage, so that future patches using hasOldLinkage() will have
> to check their codes to make sure they work with the new semantics.
The point here is that patches should come with tests. And this is how
we will make sure they will work with "new" semantics.

> I agree. Hmm that is exactly why I said that dllimport inline functions bear
> some similarity to the existing available_externally linkage. Maybe some
> miscommunication has happened? I will make a guess as to what you
> were saying.
This is yet another point of not introducing new linkage types :)
Almost all the stuff can be represented with current linkage types +
some modeling of additional dll semantics.

--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Reply | Threaded
Open this post in threaded view
|

Re: Feedback required on proper dllexport/import implementation

Gao, Yunzhong
> In this case everything should work fine. If the behavior will be changed,
> then we'll find some subtle bug hiding there for ages. normal dllexport'ed
> functions are essentially external functions with some additional semantics.

Okay.

> Right. The change should be thoroughly tested. Some fallout from such
> invasive change is a normal thing, really.

Yes, it makes sense.
I think I am now convinced that making an invasive change is not such a scary thing.

> The point here is that patches should come with tests. And this is how we will
> make sure they will work with "new" semantics.

Agreed.

- Gao.

_______________________________________________
LLVM Developers mailing list
[hidden email]         http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev