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

Cpp target: allow building runtime with newer C++ standards #3071

Merged
merged 4 commits into from
Feb 16, 2021

Conversation

felixn
Copy link
Contributor

@felixn felixn commented Feb 8, 2021

Currently, the C++ runtime compilation is forced to used the C++11 standard, since CMAKE_CXX_STANDARD 11 is set in runtime/Cpp/CMakeLists.txt.
When linking an executable built with a newer C++ standard (for example, C++17) against the runtime build with C++11, the following linker error occurs:

FAILED: src/antlr/demo 
: && /usr/bin/clang++-11  -fuse-ld=lld src/antlr/CMakeFiles/demo.dir/src/demo.cpp.o src/antlr/CMakeFiles/demo.dir/antlr4cpp_generated_src/TLexer/TLexer.cpp.o src/antlr/CMakeFiles/demo.dir/antlr4cpp_generated_src/TParser/TParser.cpp.o -o src/antlr/demo  src/antlr/antlr4_runtime/src/antlr4_runtime/runtime/Cpp/dist/libantlr4-runtime.a && :
ld.lld: error: undefined symbol: antlr4::ANTLRInputStream::ANTLRInputStream(std::basic_string_view<char, std::char_traits<char> >)
>>> referenced by demo.cpp:20 (../src/antlr/src/demo.cpp:20)
>>>               src/antlr/CMakeFiles/demo.dir/src/demo.cpp.o:(main)

This occurs due to the following snippet in ANTLRInputStream.h:

#if __cplusplus >= 201703L
    ANTLRInputStream(std::string_view input = "");
#else
    ANTLRInputStream(const std::string &input = "");
#endif

-> the runtime - built with C++11 - contains a different constructor than an executable - built with C++17 - tries to use

This pull request allows overriding the C++ standard for the runtime build - default is still C++11.
Therefore, projects using the C++ can choose to build the runtime with a newer standard.

@mike-lischke - what do you think?

set(CMAKE_CXX_STANDARD_REQUIRED ON)
set(CMAKE_CXX_EXTENSIONS OFF)
Copy link
Member

Choose a reason for hiding this comment

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

Why would you want to disable CXX extensions? Is there a rational behind this decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The runtime doesn't need/builds without GNU C++ Extensions.
(And that's a good thing, because e.g. Clang might not support (all) the GNU C++ extensions).
So I feel like it's good to make it explicit, but I'm ok with reverting the change if you want.

@@ -104,6 +104,7 @@ else()
CMAKE_CACHE_ARGS
-DCMAKE_BUILD_TYPE:STRING=${CMAKE_BUILD_TYPE}
-DWITH_STATIC_CRT:BOOL=${ANTLR4_WITH_STATIC_CRT}
# -DCMAKE_CXX_STANDARD:STRING=17 # if desired, compile the runtime with a different C++ standard
Copy link
Member

Choose a reason for hiding this comment

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

This setting should not be done in the cmake file, but be set by the outer project which needs to build ANTLR4. As such it makes not so much sense to have this here, even if it is only a comment.

Additionally, it should be documented somewhere (readme).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell, ExternalProject_Add() runs in a separate CMake instance, and does not use the same args as the outer project. So I've had to pass the build arguments explicitly here.
(The alternative seems too complicated: https://stackoverflow.com/a/48555098)

Either way, you're right, I have to update the readme - will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing the snippet above with -DCMAKE_CXX_STANDARD:STRING=${CMAKE_CXX_STANDARD} builds the runtime with the same C++ standard as the outer project. Currently, I've added it as a commented out line. Do you want me to un-comment it and make it the new default behaviour? I think the risk of breaking things is pretty small, because projects using the ExternalAntlr4Cpp build flow have a local copy of ExternalAntlr4Cpp.cmake in their source tree anyways.

@mike-lischke
Copy link
Member

Btw. if this is your first patch for ANTLR4 please also add yourself to the contributors.txt file.

@felixn
Copy link
Contributor Author

felixn commented Feb 9, 2021

Btw. if this is your first patch for ANTLR4 please also add yourself to the contributors.txt file.

Already done:

2019/11/17, felixn, Felix Nieuwenhuizhen, felix@tdlrali.com

Thanks for the quick feedback!

@AdrianBunk
Copy link

I would say commit 254b144 is bogus.

The problem is that this commit creates two different ABI versions of the library, depending on whether the library was built with a compiler set to >= C++17 or < C++17.

Exposing both in the header is wrong, and gives library users a link error at the end of the build when they use the option different from how the library was built.

In practice that currently means that code building with >= C++17 fails to link with the library.

The proper solutions would be that you either:
1 .compile your library with >= C++ 17 and require all library users to use C++17 with an #error on older C++, or
2. revert (or comment out) commit 254b144

@mike-lischke
Copy link
Member

But the fact that there are 2 different ABI versions doesn't make it bogus. As with many other libraries you have to use the same C++ dialect for the importing code, as was used for the library to build it. That is common practice (and has occasionally led to some frustration).

Usually, and that's the recommended use of the C++ runtime, you should add it in source form to your project and build it as part of that. The build time is short and you avoid any trouble because of different build settings.

@AdrianBunk
Copy link

But the fact that there are 2 different ABI versions doesn't make it bogus. As with many other libraries you have to use the same C++ dialect for the importing code, as was used for the library to build it.

The only effect of commit 254b144 so far has been always breaking using >= C++17 when building against your library.

Currently CMakeLists.txt sets the standard when building the library itself to exactly C++11.
Commit 254b144 changes the header to an ABI the library cannot cannot even be built for when a library user uses >= C++17.

And if you really want different ABIs depending on the C++ version your library was built with, please expose only this ABI in the header instead of making the ABI in the header depend on something unrelated to how the library has been built.

@mike-lischke
Copy link
Member

@parrt Currently two build targets fail here and it seems as if this patch has nothing to do with that. One is a C++ build where no output is generated after the build and the other is for Javascript with some test failures which need to be fixed elsewhere, before we can continue with accepting patches.

@felixn Once you updated the documenation (and the builds succeed again), we can accept your patch.

@AdrianBunk Commit #254b144 adds another constructor, which is only available when compiling with C++ 17 and above. So, if you build both the library and your app with C++ 17 it will work. The fixed setting of C++ 11 is what's being addressed in this patch (so you can choose afterwards), but otherwise I think the string_view constructor is ok.

@ericvergnaud
Copy link
Contributor

@mike-lischke FYI if you click on details you can re-run a build from where it failed
(I just triggered one for the failing for cpp-recursion)
the JS issue has been fixed in parallel

@mike-lischke
Copy link
Member

@ericvergnaud Thanks for the hint, but it seems I have not the privilege to rerun a job. So, atm, the JS job is still marked as failed.

@AdrianBunk
Copy link

@AdrianBunk Commit #254b144 adds another constructor, which is only available when compiling with C++ 17 and above. So, if you build both the library and your app with C++ 17 it will work.

As of today compiling the library with >= C++ 17 not possible.

The fixed setting of C++ 11 is what's being addressed in this patch (so you can choose afterwards), but otherwise I think the string_view constructor is ok.

I am a Debian Developer, and I ended up here due to the problems commit 254b144 is causing when trying to build some software from Oracle using C++ 17 with the version of your library in the upcoming Debian 11.

Exposing ABIs you are not providing in the headers is an absolute nightmare for distributions shipping binary builds of your library.
Iif you want to change library ABI depending on the C++ version used when compiling the library, please also generate the header so that it provides exactly the one ABI that is also provided by the library.

@felixn
Copy link
Contributor Author

felixn commented Feb 15, 2021

@felixn Once you updated the documentation (and the builds succeed again), we can accept your patch.

@mike-lischke - Done!

Iif you want to change library ABI depending on the C++ version used when compiling the library, please also generate the header so that it provides exactly the one ABI that is also provided by the library.

@AdrianBunk - what would that look like? Currently the header files are not generated at all, but are static files. Either way, I think that's out of scope of this PR? This PR is purely about enabling building the runtime with a C++ standard != C++11

@AdrianBunk
Copy link

Currently the header files are not generated at all, but are static files.

Are Linux distributions supposed to manually patch the headers themselves?

Either way, I think that's out of scope of this PR? This PR is purely about enabling building the runtime with a C++ standard != C++11

The scope should be enabling building code with C++ >= 17 against the library.
I still don't see why people here are so eager to create and keep multiple incompatible ABIs.

@felixn
Copy link
Contributor Author

felixn commented Feb 15, 2021

Are Linux distributions supposed to manually patch the headers themselves?

That's a rhetorical question, of course, but I don't think it's that helpful. Look, I asked "what would that look like?" because I don't know what it is exactly that you need. I'm not the author of the commit that's causing the issue nor all that involved in the antlr project aside from a few bug fixes, I'm just trying to get my use case (build with C++17) to work again. Still, I'm trying to figure out how we can address your use case as well, but I get the feeling that the changes required (generated header files?) are out of scope of this PR. So I think a separate issue or PR would be better. If you disagree, then it would be helpful to make to make concrete suggestions.

@AdrianBunk
Copy link

Are Linux distributions supposed to manually patch the headers themselves?

That's a rhetorical question, of course, but I don't think it's that helpful.

This is not a rhetorical question, this is pretty much the only alternative option.

Look, I asked "what would that look like?" because I don't know what it is exactly that you need. I'm not the author of the commit that's causing the issue nor all that involved in the antlr project aside from a few bug fixes, I'm just trying to get my use case (build with C++17) to work again. Still, I'm trying to figure out how we can address your use case as well, but I get the feeling that the changes required (generated header files?) are out of scope of this PR. So I think a separate issue or PR would be better. If you disagree, then it would be helpful to make to make concrete suggestions.

A revert can be a better option than trying to fix something that never worked.

Your use case (build with C++17) is broken because the headers are currently exposing a different ABI with C++ >= 17 that is never provided.
Instead of trying to provide the ABI, the simple solution would be to fix the header.

Status quo is that it was never possible to compile the library with C++ >= 17.
In other words, it was never possible to compile it for the second ABI introduced by commit 254b144.

I would suggest to edit ANTLRInputStream.h to change the #if __cplusplus >= 201703L into #if 0.
And keep it that way until everything becomes C++ >= 17 only. At that point the header should also get an #if __cplusplus < 201703L that gives an #error.

@mike-lischke
Copy link
Member

mike-lischke commented Feb 16, 2021

@AdrianBunk You might have a valid point here, which we should investigate. However, that's not the purpose of this PR here. Instead please open a new issue and describe in more detail what the issue is with commit #254b144 is.

In order to ensure the current code builds with C++17 I did a local build in Xcode, which went fine (for both C++11 and C++17, without GNU extensions).

@mike-lischke
Copy link
Member

@parrt This PR is ready to merge, once the builds are green again.

This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants