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

Do not mark as C extern functions with C++ types #44601

Closed
wants to merge 1 commit into from

Conversation

giordano
Copy link
Contributor

Some functions with a C++ return type were marked as extern "C", but this
would cause warnings when compiling with clang:

./processor.h:190:73: warning: 'jl_get_llvm_target' has C-linkage specified, but returns incomplete type 'std::pair<std::string, std::vector<std::string>>' (aka 'pair<basic_string<char>, vector<basic_string<char>>>') which could be incompatible with C [-Wreturn-type-c-linkage]
extern "C" JL_DLLEXPORT std::pair<std::string,std::vector<std::string>> jl_get_llvm_target(bool imaging, uint32_t &flags);
                                                                        ^
./processor.h:197:67: warning: 'jl_get_llvm_disasm_target' has C-linkage specified, but returns user-defined type 'const std::pair<std::string, std::string> &' (aka 'const pair<basic_string<char>, basic_string<char>> &') which is incompatible with C [-Wreturn-type-c-linkage]
extern "C" JL_DLLEXPORT const std::pair<std::string,std::string> &jl_get_llvm_disasm_target(void);
                                                                  ^
./processor.h:214:55: warning: 'jl_get_llvm_clone_targets' has C-linkage specified, but returns incomplete type 'std::vector<jl_target_spec_t>' which could be incompatible with C [-Wreturn-type-c-linkage]
extern "C" JL_DLLEXPORT std::vector<jl_target_spec_t> jl_get_llvm_clone_targets(void);
                                                      ^

We also need to explicitly mark the C++-mangled names of these functions to the
list of exported symbols, otherwise they'd be hidden in libjulia-internal.so,
and linking libjulia-codegen.so would fail because of the undefined symbols.
This wasn't needed when the functions were marked as extern "C" because the
exported symbols wouldn't have the C++ name-mangling and so they'd match the
jl_* entry.


This commit was originally in #44350, but I took it out from that PR because it's slightly orthogonal (this fixes a warning from Clang, not GCC), it's also a bit more convoluted that the rest of changes in #44350, and with this error I was seeing a strange build error on FreeBSD which I didn't quite understand.

Some functions with a C++ return type were marked as `extern "C"`, but this
would cause warnings when compiling with clang:

```
./processor.h:190:73: warning: 'jl_get_llvm_target' has C-linkage specified, but returns incomplete type 'std::pair<std::string, std::vector<std::string>>' (aka 'pair<basic_string<char>, vector<basic_string<char>>>') which could be incompatible with C [-Wreturn-type-c-linkage]
extern "C" JL_DLLEXPORT std::pair<std::string,std::vector<std::string>> jl_get_llvm_target(bool imaging, uint32_t &flags);
                                                                        ^
./processor.h:197:67: warning: 'jl_get_llvm_disasm_target' has C-linkage specified, but returns user-defined type 'const std::pair<std::string, std::string> &' (aka 'const pair<basic_string<char>, basic_string<char>> &') which is incompatible with C [-Wreturn-type-c-linkage]
extern "C" JL_DLLEXPORT const std::pair<std::string,std::string> &jl_get_llvm_disasm_target(void);
                                                                  ^
./processor.h:214:55: warning: 'jl_get_llvm_clone_targets' has C-linkage specified, but returns incomplete type 'std::vector<jl_target_spec_t>' which could be incompatible with C [-Wreturn-type-c-linkage]
extern "C" JL_DLLEXPORT std::vector<jl_target_spec_t> jl_get_llvm_clone_targets(void);
                                                      ^
```

We also need to explicitly mark the C++-mangled names of these functions to the
list of exported symbols, otherwise they'd be hidden in `libjulia-internal.so`,
and linking `libjulia-codegen.so` would fail because of the undefined symbols.
This wasn't needed when the functions were marked as `extern "C"` because the
exported symbols wouldn't have the C++ name-mangling and so they'd match the
`jl_*` entry.
@giordano giordano requested a review from JeffBezanson March 13, 2022 15:53
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

I'm not a fan. Forcing C linking here is intentional to make them easy to call in contexts where a C++ mangler is not available.

@giordano
Copy link
Contributor Author

giordano commented Mar 13, 2022

This is already done for other functions though:

julia/src/julia.expmap

Lines 39 to 42 in 6a9737d

__ZN4llvm23createLowerSimdLoopPassEv;
_Z24jl_coverage_data_pointerN4llvm9StringRefEi;
_Z22jl_coverage_alloc_lineN4llvm9StringRefEi;
_Z22jl_malloc_data_pointerN4llvm9StringRefEi;

@Keno
Copy link
Member

Keno commented Mar 13, 2022

Well, those functions aren't part of the same set of functions. I don't really the problem. The warning is new. We can turn if off on the command line or just via #pragma.

@giordano
Copy link
Contributor Author

The goal is to reduce noise when compiling julia, either fixing the warnings or simply silencing them with pragmas/command line would be fine with me.

Actually, I just noticed this is already the case:

julia/src/Makefile

Lines 29 to 31 in 98b4b06

ifeq ($(USECLANG),1)
FLAGS += -Wno-return-type-c-linkage
endif
but USECLANG is set automatically only if you're building for FreeBSD or macOS:

julia/Make.inc

Lines 468 to 477 in 98b4b06

ifeq ($(OS), FreeBSD)
USEGCC := 0
USECLANG := 1
endif
# Note: Supporting only macOS Yosemite and above
ifeq ($(OS), Darwin)
APPLE_ARCH := $(shell uname -m)
USEGCC := 0
USECLANG := 1
and I got this warning on Linux. Perhaps USECLANG should be instead set based on the output of $(CC) --version?

Anyway, I'm going to close this PR.

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.

2 participants