[Cython] Fwd: Question about how best require compiler options for C sources

[Erik Bray]

On Mon, Apr 11, 2016 at 7:49 PM, Ian Henriksen
<insertinterestingnamehere at gmail.com> wrote:
> On Mon, Apr 11, 2016 at 7:46 AM Erik Bray <erik.m.bray at gmail.com> wrote:
>>
>> On Mon, Apr 11, 2016 at 2:51 PM, Nathaniel Smith <njs at vorpus.org> wrote:
>> > On Apr 11, 2016 04:18, "Erik Bray" <erik.m.bray at gmail.com> wrote:
>> >>
>> >> On Fri, Apr 8, 2016 at 5:49 PM, Nathaniel Smith <njs at vorpus.org> wrote:
>> >> > Can you give a tiny concrete example? My questions are basic enough
>> >> > that
>> >> > I
>> >> > feel like I'm missing something fundamental :-)
>> >>
>> >> Yes, I think you might be missing something, but I'm not sure exactly
>> >> where.  In the issue I provided a tiny example which you can see here:
>> >>
>> >> https://gist.github.com/embray/12a67edb82b213217e31f408007898e6
>> >>
>> >> The C code generated by this example currently does not compile on
>> >> Windows, because of how Cython uses DL_IMPORT incorrectly.  Regardless
>> >> of what it *does* do, Cython should not be using the DL_IMPORT macro
>> >> at all actually since that no longer even exists in Python.
>> >
>> > Sure.
>> >
>> > For that example, the correct thing to do is to *not* export the
>> > function.
>>
>> So as I wrote in my previous message, my example was incomplete.  But
>> indeed if the intent was *only* to share the declaration between TUs
>> of the same library then I agree the *most* correct thing would be to
>> not use dllexport. Unfortunately there isn't currently a way in Cython
>> to make this distinction.
>>
>> > Backing up, to make sure we're on the same page:
>>
>> Yep, I agree with your analysis that follows, and it's helpful to have
>> all the cases laid out in one place, so thanks for that!
>>
>> There are three levels of
>> > symbol visibility in C: file-internal, shared library internal
>> > (different .c
>> > files that make up the library can see it, but users of the shared
>> > library
>> > can't see it), and shared library exported (everyone can see it; can
>> > also
>> > carry other consequences, e.g. on Linux then internal calls will become
>> > noticeably slower, and it becomes possible for weird symbol
>> > interposition
>> > issues to occur). So the rule of thumb is to make everything as private
>> > as
>> > you can get away with.
>> >
>> > Making this more interesting:
>> > - vanilla C only includes 2 ways to mark symbol visibility, which is not
>> > enough to make a 3-way distinction. Hence the need for an extra
>> > attribute
>> > thingummy.
>> > - everyone agrees that symbols marked 'static' should be file-internal,
>> > but
>> > different platforms disagree about what should happen if the extra
>> > attribute
>> > thingummy is missing.
>> >
>> > So on Windows, the convention is:
>> > 'static' -> file internal
>> > no marking -> shared library internal
>> > 'dllexport' -> public
>> >
>> > And on Linux it's:
>> > 'static' -> file internal
>> > 'visibility (hidden)' -> shared library internal
>> > no marking -> public
>> >
>> > It's generally agreed that Linux got this wrong and that you should
>> > always
>> > use '-fvisibility=hidden' to switch it to the windows style, but cython
>> > doesn't control compiler options and thus should probably generate code
>> > that
>> > works correctly regardless of such compiler settings. Fortunately, Linux
>> > does provide some markings to explicitly make things public: you can
>> > mark a
>> > symbol 'visibility (default)' (which means public), or you can use the
>> > dllexport syntax, just like Windows, because gcc is helpful like that.
>> >
>> > OTOH, windows is annoying because of this dllimport thing that started
>> > this
>> > whole thread: on other systems just marking symbols as extern is enough
>> > to
>> > handle both shared-object-internal and shared-library-exported symbols,
>> > and
>> > the linker will sort it out. On Windows, you have to explicitly
>> > distinguish
>> > between these. (And annoyingly, if you accidentally leave out the
>> > dllimport
>> > making on functions then it will use some fallback hack that works but
>> > silently degrades performance; on other symbols it just doesn't work,
>> > and
>> > ditto for if you use it when it isn't needed.)
>>
>> Yep. Agreed with all of the above.
>>
>> > So final conclusion: for non-static symbols, cython should first decide
>> > whether they are supposed to be shared-library-internal or actually
>> > exported
>> > from the shared library.
>> >
>> > For shared-library-internal symbols: their definition should be marked
>> > 'visibility(hidden)' on Linux, and unmarked on Windows. This is easy
>> > using
>> > some preprocessor gunk. (Or maybe simplest is: marked that everywhere
>> > except
>> > if using msvc, because I think everyone else will understand 'visibility
>> > (hidden)' even if it's a no op.) Their declaration in the header file
>> > should
>> > just be 'extern' everywhere.
>> >
>> > For shared-library-exported symbols: I am dubious about whether cython
>> > should even support these at all. But if it does, then the definitions
>> > should be marked 'dllexport' (no macro trickery needed, because everyone
>> > understands this syntax), and their declaration in the header file needs
>> > some extra hack that is the subject of this thread.
>>
>> I understand your doubts here, but for the purpose of discussion let's
>> just assume that it should be supported? :)
>>
>> And yes, the issue that the header file needs some hacky
>> context-dependent declarations.  A possible alternative, which in fact
>> is exactly the work-around I'm using right now, would be for Cython to
>> generate two headers.  In the case of my example they would be "foo.h"
>> and "foo_internal.h".  They would be almost exactly the same except
>> that the former declares the function dllimport and the latter
>> declares the function dllexport, and only the _internal.h should be
>> used to share a declaration between TUs.  I'm currently doing this by
>> writing the "foo_internal.h" manually.
>>
>> > Now, back to your example: Here the caller and callee are both compiled
>> > into
>> > the same shared library, so you don't want dllexport/dllimport at all,
>> > you
>> > just want a shared-library-internal symbol, which as we see is much
>> > easier.
>>
>> Well, again, in this case I do want dllexport/dllimport but I should
>> have been more clear about that.  But supposing I didn't want it, then
>> this is true.
>>
>> > NumPy also ran into some problems with this in our experiments with
>> > using
>> > cython internally. Our temporary solution was to use the preprocessor to
>> > monkeypatch DL_IMPORT into expanding to the appropriate
>> > shared-library-internal thing :-).
>>
>> Ah, how did you manage that?  Do you have to make sure to do it before
>> Python.h is ever included?
>>
>> >> > My first question is why you even need this, since AFAIK there are no
>> >> > cases
>> >> > where it is correct to have a cython module dllexporting symbols that
>> >> > appear
>> >> > in header files. This is what cimport is for, right?
>> >>
>> >> I don't think this has anything to do with cimport.  Could you explain
>> >> what you mean?
>> >
>> > We only need to solve the dllimport issue if we want to support
>> > shared-library-exported symbols from cython extensions. This is only
>> > useful
>> > if you have different extensions that are directly linking to each other
>> > using the platform linker. But this simply doesn't work in almost any
>> > cases,
>> > because platform linkers have all kinds of quirks that don't play well
>> > with
>> > python packages -- to start with, your runtime linker probably does not
>> > follow Python's rules for which directories to search to find a shared
>> > library... To solve these problems, the Cython devs invented the cimport
>> > mechanism, which is basically a portable, python-centric way of doing
>> > shared-library-exports while avoiding all the problems caused by using
>> > the
>> > platform linker. So my question was, what's your use case that's better
>> > served by linking directly against an extension module and using the
>> > platform's shared-library-export functionality, instead of cython's?
>>
>> Well, yes, but you can't use cimport from C code :)  As for the use
>> case I'll have to get back to you on that.  I'm trying to help the
>> Sage project fix some issues related to that and my understanding is
>> that it is needed for C code to be able to link directly against code
>> that's compiled into an extension module.  I could be wrong about that
>> in which case we're free to ignore that case (unless someone does come
>> up with a use case).  So yes, I had better double-check on that :)
>>
>> Still, I'm not necessarily talking about linking Cython modules
>> together, which is why I don't think cimport really comes into it.
>>
>> >> > My second question is why you would want to do this via the command
>> >> > line,
>> >> > when compiling the dll means that you are compiling some
>> >> > cython-generated
>> >> > .c, which means that you can put the #define directly in the source
>> >> > code,
>> >> > no?
>> >>
>> >> Not necessarily. As you can see in my example the file fooutil.c is
>> >> hand-written C code that was not generated by Cython, but which uses a
>> >> function in the Cython-generated code.  It includes "foo.h".  In
>> >> principle you're right--the hand-written C code could set the proper
>> >> #defines but it would have to do so *before* including "foo.h".  It's
>> >> not very obvious that this would be needed.  Most other code I've seen
>> >> that addresses this issue--including cpython itself--do so by passing
>> >> an appropriate define to the compiler via the command-line, and that
>> >> seems the clearest to me.
>> >
>> > I see, right. I guess my suggestion would be that if a symbol really
>> > does
>> > need to be marked for shared-library-export *and simultaneously* used by
>> > different files within the same shared library -- which is the only case
>> > where this arises -- then possibly the simplest and most robust thing is
>> > to
>> > set up the header file so that external users just do #include "foo.h",
>> > and
>> > the internal users do
>> >
>> >   #define FOO_INTERNAL
>> >   #include "foo.h"
>>
>> Okay, this is similar to my above suggestion of using separate
>> headers.  I for one prefer the separate headers better, as I think
>> it's easy to forget to set a #define like that--and to anyone not
>> working on Windows it's not at all clear why that would be needed.  In
>> fact on Linux it will "just work" without the "#define FOO_INTERNAL"
>> so without regular testing on Windows it will be too easy to forget.
>>
>> I'm not sure if having a separate _internal.h header is any better.
>> But it *might* be--in particular if it's generated by Cython then it
>> would force the developer to ask what the difference is between
>> "foo.h" and "foo_internal.h". And they can both contain comments
>> explaining when to use them.
>>
>> > I think the obvious thing is that cython should provide a natural way to
>> > mark a function as being shared-library-internal; that covers 99% of
>> > real
>> > use cases *and* just works without any further macros needed. Probably
>> > the
>> > existing "public" annotation should be changed to mean this.
>>
>> Probably, yeah.  Ignoring the DL_IMPORT stuff the other effect of
>> marking a symbol "public" in Cython is to add the appropriate
>> __PYX_EXTERN_C.  And if that were *all* it did then its behavior would
>> be consistent between platforms :)
>>
>> > (Obviously it
>> > wasn't quite fully thought through in the first place and has few if any
>> > users, since it got this very wrong without anyone noticing. So fixing
>> > it
>> > seems viable to me.)
>>
>> +1
>>
>> > And then *maybe* there should also be a way to make a symbol
>> > shared-library-exported, if that really is useful, but as a non-default
>> > experts-only kind of thing, and as such it would be OK to require these
>> > rare
>> > expert users to be a bit more careful about how they #include the
>> > resulting
>> > header within their own project.
>>
>> Okay. My belief is that there is a case for this, but I should
>> substantiate it better.  Would you be amenable to the generation of a
>> "<modulename>_internal.h"?  The more I think about it the more I'm
>> convinced this would be the simplest way to handle this, and would
>> simplify matters by not requiring Cython to impose any compiler flags
>> (making my original quesition moot).
>>
>> Agreed that it could be non-default.
>>
>> Thanks again,
>>
>> Erik
>>
>
> To answer the original question about define macros, it appears that the
> canonical
> way to pass preprocessor defines through distutils is to use the
> define_macros
> keyword when constructing your Extension class. You should also be able to
> do
> this within a Cython source file by including a directive like:
>
> # distutils: define_macros = MY_DEFINE, MY_DEFINE_2=2
>
> Unfortunately, it looks like there's a bug in that that's making it so that
> these
> macros are undef'ed rather than being defined, so, for now, just pass the
> appropriate flags to your Extension object.
>
> That aside, I agree with Nathaniel that exporting public declarations as a
> part of the
> shared object interface was a design mistake. That aside, however, using an
> api
> declaration lets you get equivalent results without exposing anything as a
> part of
> the shared object API. Here's how this all works:
>
> public declarations: export things to C/C++ through the shared object
> interface.
> Provide a header that exports this interface.
> api declarations: export things to C/C++ through capsule objects. Provide a
> header
> for the Python module that exports that interface.
> cimports: Use capsule objects and parsing of pxd files to share things like
> external
> declarations, header includes, inline Cython functions, and Cython functions
> exported by modules between Cython modules.
>
> The public and api use cases are essentially the same most of the time, but
> api
> declarations use capsules rather than the OS's linker.
>
> There are still some trade-offs between public and api functions.
> Technically, the
> api functions require that the initialization routine exported in the api
> header be
> called for each translation unit that uses them. The public api just
> requires that the
> module already be initialized. In cases where no Python functionality is
> used in a
> public function, you may be able to get away with using the function without
> initializing the module, though I really wouldn't recommend that.
>
> There are some more subtle issues here though. The reason api functions need
> to
> be initialized on a per-translation unit basis is that things exported as
> api
> declarations are exported as translation-unit-local (static) function
> pointers. They
> aren't shared by the different translation units within a module built from
> multiple
> source files. I think that's a mistake. It'd be ideal if we could have api
> interfaces (or
> something like them) provide things with shared object local visibility
> rather than
> translation unit local visibility. This would require that the API headers
> have more
> carefully structured ifdef directives so that a macro could be set in a
> given
> translation unit to designate when to emit the actual declarations for the
> needed
> pointers rather than just forward declaring them. It would also require that
> the main
> generated c/cpp file define the pointers it uses as shared-object-local
> rather rather
> than static.
>
> In dynd-python we currently solve this problem by defining shared object
> local
> wrappers for the api exported function pointers and then using those
> instead, but
> I'm not a huge fan of that approach. It works well, but results in another
> unnecessary layer of indirection through the source files to connect the C++
> code
> back to its Python bindings.
>
> With regards to the dllexporting/dllimporting of things: given that public
> declarations
> are already designed to export things through the shared object interface,
> we may
> as well fix the current setup to export the right things. It's a bad design
> that
> probably ought to be deprecated or at least documented better so people know
> not
> to use it unless their case actually requires sidestepping best practices.
> On the
> other hand, it's also a supported interface, so there's value in "fixing"
> it.
>
> I think the best way to do that is the following:
> - mark public symbols as dllimport unless a given (module specific)
> preprocessor
> define is set.
> - people using the public header outside of the module exporting the symbols
> should not have to set the define at all.
> - people using the public header to compile other source files that are
> linked in to
> the same Python module should set the preprocessor flag for that module.
>
> On top of that, at some point we still need to fix our api and public
> headers so that
> they still work when included into the translation unit for the main
> Cython-generated
> c/cpp file. This use-case should just forward-declare everything since the
> needed
> symbols are all defined later on in the Cython module. Since static
> variables
> cannot be forward declared in C, this will require that api declarations use
> shared
> object local symbols or that the main generated c/cpp file use some ifdef
> guards
> when it initializes the various pointers in question.
>
> As far as making an additional header goes, I personally prefer the extra
> preprocessor define. On the other hand, if people think an additional header
> is
> easier to use, then why not make it do something like
>
> #define USE_DLLEXPORT_NOT_DLLIMPORT_FOR_PARTICULAR_MODULE
> #include <the_original_public_header_with_improved_defines.h>
>
> I think, that'd cover all the use cases better.
>
> Anyway, sorry for the length of my remarks on this. There are several issues
> here
> that have been bothering me for quite some time.

Hi all,

Reviving this discussion again, since I'm back to working on it and in
particular trying to improve Cython's test acceptance on Cygwin.

There is another problem related to this that is not addressed at all
by PR #360 [1] and is perhaps even trickier to handle in any sensible
way.  There is an example of this in the test suite under
Cython.Debugger.Tests.TestLibCython.

There is a Cython module in there called simply "codefile" (without
.pyx).  It contains the statement:

    cdef extern:
        void some_c_function()

The some_c_function() is defined in a separate file cfuncs.c.
Cythoning "codefile" results in a declaration for some_c_function(),
since it is not provided by any header (?):

    __PYX_EXTERN_C DL_IMPORT(void) some_c_function(void); /*proto*/

However, this blows up at link time when codefile.o and cfuncs.o are linked.

A workaround is to write a header file containing a declaration for
some_c_function, and change the cdef extern statement to:

    cdef extern from "codefile.h":

This indicates that some_c_function is found in an existing header
file, and that a declaration should not be generated for it, as
expected.

So perhaps it should simply be documented that "cdef extern" without a
header specified should never be used for objects defined in another
object file that is being statically linked to.

Alternatively, the only other workable possibility is to add to Cython
a way to explicitly indicate that a function is defined in another .c
file that we are statically linking with without actually *including*
that .c file, so that a declaration is generated with the correct DL_
macro.

Thoughts?

Erik

[1] https://github.com/cython/cython/pull/360