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

CI is not checking compatibility with MacOs clang #333

Open
huitema opened this issue Dec 7, 2020 · 8 comments
Open

CI is not checking compatibility with MacOs clang #333

huitema opened this issue Dec 7, 2020 · 8 comments

Comments

@huitema
Copy link
Collaborator

huitema commented Dec 7, 2020

@larseggert just found a bug in a recent PR because the new code introduced a variable named "xor". This works fine with many compilers (gcc, mscc) but clang throws an error because it treats "xor" as a keyword.

What is really weird is that the offending code passed the integration checks, including this run, which tests with:

    - name: Linux (clang)
      os: linux
      compiler: clang

The issue is being fixed in PR #332, but we should also fix the integration test that did not discover the problem.

@larseggert
Copy link
Contributor

FWIW, I am using clang-11 installed via homebrew, not the Apple clang

@huitema
Copy link
Collaborator Author

huitema commented Dec 7, 2020

@larseggert do you have a suggestion on how to modify the yml and catch such issues in the future?

@huitema
Copy link
Collaborator Author

huitema commented Dec 7, 2020

Also, what clang is doing here is importing in the C compiler tokens that are only defined in C++. The Clang 11 token binding source file includes:

// C++ 2.5p2: Alternative Representations.
CXX_KEYWORD_OPERATOR(and     , ampamp)
CXX_KEYWORD_OPERATOR(and_eq  , ampequal)
CXX_KEYWORD_OPERATOR(bitand  , amp)
CXX_KEYWORD_OPERATOR(bitor   , pipe)
CXX_KEYWORD_OPERATOR(compl   , tilde)
CXX_KEYWORD_OPERATOR(not     , exclaim)
CXX_KEYWORD_OPERATOR(not_eq  , exclaimequal)
CXX_KEYWORD_OPERATOR(or      , pipepipe)
CXX_KEYWORD_OPERATOR(or_eq   , pipeequal)
CXX_KEYWORD_OPERATOR(xor     , caret)
CXX_KEYWORD_OPERATOR(xor_eq  , caretequal)

Was the bug found in a C compile, or was the code imported in C++?

@larseggert
Copy link
Contributor

C++

@huitema
Copy link
Collaborator Author

huitema commented Dec 7, 2020

I think that's the issue, C++. Picotls is purely C, so the tests won't catch conflicts with C++ keywords. I had a similar problem in Picoquic, and solved it by adding an explicit C++ test. Something as simple as compiling hello word in C++ while including picotls.h might suffice.

@huitema
Copy link
Collaborator Author

huitema commented Dec 7, 2020

By the way, Clang11 should know better. The header includes

#ifdef __cplusplus
extern "C" {
#endif

A standard conformant compiler should know that what follows is C code, not C++ code...

@huitema
Copy link
Collaborator Author

huitema commented Dec 7, 2020

Here is the C++ code that I use in picoquic:


/* Simple test using C++.
 * The goal is to verify that the libraries are accessible from C++, and that compilation works.
 */
#ifdef _WINDOWS
#include "wincompat.h"
#endif
#include "picoquic.h"
#include "picohash.h"
#include "picoquic_internal.h"
#include "picoquic_utils.h"
#include "picosocks.h"
#include "picoquictest.h"
#include "picoquictest_internal.h"

extern "C" {
    int cplusplustest() {
        return 0;
    }
}

Replace picoquic headers by picotls headers, add that to the test library, and call cplusplustest from the test suite. Maybe add one of these for each of minicrypto, openssl and fusion. @kazuho, what do you think?

@kazuho
Copy link
Member

kazuho commented Dec 7, 2020

@huitema Thank you for the suggestion. I'll add something like that to our tests.

A standard conformant compiler should know that what follows is C code, not C++ code...

FWIW extern "C" changes the linkage (i.e. name mangling rule), but does not change how the source code is being interpreted. Therefore, clang is correct to complain about the header files using a C++ keyword (sigh).

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

No branches or pull requests

3 participants