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

cmd/cgo: inject preamble before other include directives #35315

Open
philtay opened this issue Nov 2, 2019 · 16 comments
Open

cmd/cgo: inject preamble before other include directives #35315

philtay opened this issue Nov 2, 2019 · 16 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@philtay
Copy link

philtay commented Nov 2, 2019

What version of Go are you using (go version)?

go version go1.13.3 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you see?

The cgo generated file _cgo_export.c includes stdlib.h before anything else (see here). It precludes the possibility to use some "include first" headers without errors or warnings, such as Python.

From the Python docs:

Since Python may define some pre-processor definitions which affect the standard headers on some systems, you must include Python.h before any standard headers are included.

Cgo should allow to inject a C preamble before any other include directive.

@smasher164 smasher164 changed the title _cgo_export.c and include directives cmd/cgo: inject preamble before other include directives Nov 3, 2019
@smasher164 smasher164 added FeatureRequest NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 3, 2019
@smasher164
Copy link
Member

/cc @ianlancetaylor

@ianlancetaylor
Copy link
Contributor

Can you show an example where it would be appropriate for cgo code to include Python.h?

@philtay
Copy link
Author

philtay commented Nov 5, 2019

Sure. Basically just before any generated include directive. E.g.:

Actual _cgo_export.c:

/* Code generated by cmd/cgo; DO NOT EDIT. */

#include <stdlib.h>
#include "_cgo_export.h"

#pragma GCC diagnostic ignored "-Wunknown-pragmas"
...

Desired _cgo_export.c:

/* Code generated by cmd/cgo; DO NOT EDIT. */

#include <Python.h>
#include <stdlib.h>
#include "_cgo_export.h"

#pragma GCC diagnostic ignored "-Wunknown-pragmas"
...

The same goes for any other generated file which may contain Go exported functions and/or their declaration.

@philtay
Copy link
Author

philtay commented Nov 5, 2019

A possible solution could be to remove #include <stdlib.h> keeping only #include "_cgo_export.h". Then in _cgo_export.h the preamble from import "C" comments should be injected before any other include (i.e. stdlib.h and stddef.h).

@ianlancetaylor
Copy link
Contributor

@philtay Thanks. I understand the change you want to make. What I'm hoping to see is an example of actual Go code that requires that change. Not the output of cgo, but the code that a user would write that requires this change.

@philtay
Copy link
Author

philtay commented Nov 8, 2019

@ianlancetaylor Ah, ok. Please find below a self contained example:

main.go

package main

/*
#cgo pkg-config: python3
#cgo CFLAGS: -std=c99
#include <Python.h>
*/
import "C"

//export foobar
func foobar() *C.PyObject {
	return nil
}

func main() {}

main.c

#include <Python.h>
#include "_cgo_export.h"

PyObject *
foobar_wrap(PyObject *self, PyObject *Py_UNUSED(ignored))
{
    return foobar();
}

Run main.go to get an error like this one:

In file included from /usr/include/python3.7m/Python.h:8,
                 from main.go:6,
                 from _cgo_export.c:4:
/usr/include/python3.7m/pyconfig.h:1522: warning: "_POSIX_C_SOURCE" redefined
 1522 | #define _POSIX_C_SOURCE 200809L
      | 
In file included from /usr/include/bits/libc-header-start.h:33,
                 from /usr/include/stdlib.h:25,
                 from _cgo_export.c:3:
/usr/include/features.h:295: note: this is the location of the previous definition
  295 | # define _POSIX_C_SOURCE 199506L
      | 

@ianlancetaylor
Copy link
Contributor

Thanks for the example.

I'm sorry to be so unclear, but that is still not what I am looking for. I'm trying to understand in what cases someone would want to write Go code that uses cgo that does a #include <Python.h>. What is the actual Go code where this case arises?

Looking at your example, why would someone write Go code that returns a *PyObject? That seems like a memory leak or dangling pointer waiting to happen. Why not instead write C code that calls Go code to do some operation and then do the Python conversion in C? That seems much more likely to get memory issues right across the three languages.

@philtay
Copy link
Author

philtay commented Nov 8, 2019

@ianlancetaylor Two full blown examples in the wild:

My code is just a minimal reproducible example.

I think the author of this blog post is on your team:

https://blog.filippo.io/building-python-modules-with-go-1-5/

It explains why someone would want to write cgo code that does a #include <Python.h>.

@ianlancetaylor
Copy link
Contributor

Thanks for the pointers.

@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Nov 8, 2019
@philtay
Copy link
Author

philtay commented Nov 8, 2019

/cc @FiloSottile

@philtay
Copy link
Author

philtay commented Nov 20, 2019

@ianlancetaylor I'd like to help with this issue. Can you point me in the right direction? Thanks.

@ianlancetaylor
Copy link
Contributor

At this point I don't know what the right direction is. Changes in this area have a history of breaking existing code.

@philtay
Copy link
Author

philtay commented Nov 20, 2019

What about a cgo directive? Something like:

// #cgo include <foo.h>

@ianlancetaylor
Copy link
Contributor

A #cgo directive seems like kind of a last resort. I would hate to have to write the documentation that explains when you should write #include and when you should write #cgo include. While cgo is not particularly simple, our goal as to always be to make it as simple as possible, and distinguishing #include and #cgo include cannot be described as simple.

@odeke-em
Copy link
Member

Quite a tricky change to get into this cycle, thus I shall move it to Go1.16.

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 23, 2020
@aclements
Copy link
Member

Moving to Unplanned, since currently we don't have a plan to address this.

@aclements aclements modified the milestones: Go1.16, Unplanned Dec 7, 2020
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FeatureRequest help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants