[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8267464: Circular-dependency resilient inline headers [v8]
From:       Stefan Karlsson <stefank () openjdk ! java ! net>
Date:       2021-05-31 9:02:31
Message-ID: ARWfm7kOPHXGZRJRYsnnJNPFSMMSlCV4Mj85tE9FpTI=.f5a7a6e3-ad08-46e5-8fa8-dbc318a2d24e () github ! com
[Download RAW message or body]

On Mon, 31 May 2021 06:44:45 GMT, Stefan Karlsson <stefank@openjdk.org> wrote:

> > I'd like to propose a small adjustment to how we write .inline.hpp files, in the \
> > hope to reduce include problems because of circular dependencies between inline \
> > headers. 
> > This is a small, contrived example to show the problem:
> > 
> > // a.hpp
> > #pragma once
> > 
> > void a1();
> > void a2();
> > 
> > 
> > // a.inline.hpp
> > #pragma once
> > 
> > #include "a.hpp"
> > #include "b.inline.hpp"
> > 
> > inline void a1() {
> > b1();
> > }
> > 
> > inline void a2() {
> > }
> > 
> > 
> > // b.hpp
> > #pragma once
> > 
> > void b1();
> > void b2();
> > 
> > 
> > // b.inline.hpp
> > #pragma once
> > 
> > #include "a.inline.hpp"
> > #include "b.hpp"
> > 
> > inline void b1() {
> > }
> > 
> > inline void b2() {
> > a1();
> > }
> > 
> > The following code compiles fine:
> > 
> > // a.cpp
> > #include "a.inline.hpp"
> > 
> > int main() {
> > a1();
> > 
> > return 0;
> > }
> > 
> > But the following:
> > 
> > // b.cpp
> > #include "b.inline.hpp"
> > 
> > int main() {
> > b1();
> > 
> > return 0;
> > }
> > 
> > 
> > fails with the this error message:
> > 
> > In file included from b.inline.hpp:3,
> > from b.cpp:1:
> > a.inline.hpp: In function ‘void a1()':
> > a.inline.hpp:8:3: error: ‘b1' was not declared in this scope; did you mean \
> > ‘a1'? 
> > We can see the problem with g++ -E:
> > 
> > # 1 "a.inline.hpp" 1
> > # 1 "a.hpp" 1
> > 
> > void a1();
> > void a2();
> > 
> > # 4 "a.inline.hpp" 2
> > 
> > inline void a1() {
> > b1();
> > }
> > 
> > inline void a2() {
> > }
> > 
> > # 4 "b.inline.hpp" 2
> > # 1 "b.hpp" 1
> > 
> > void b1();
> > void b2();
> > 
> > # 5 "b.inline.hpp" 2
> > 
> > inline void b1() {
> > }
> > 
> > inline void b2() {
> > a1();
> > }
> > 
> > # 2 "b.cpp" 2
> > 
> > int main() {
> > b1();
> > 
> > return 0;
> > }
> > 
> > 
> > b1() is called before it has been declared. b.inline.hpp inlined a.inline.hpp, \
> > which uses functions declared in b.hpp. However, at that point we've not included \
> > enough of b.inline.hpp to have declared b1(). 
> > This is a small and easy example. In the JVM the circular dependencies usually \
> > involves more than two files, and often from different sub-systems of the JVM. We \
> > have so-far solved this by restructuring the code, making functions out-of-line, \
> > creating proxy objects, etc. However, the more we use templates, the more this is \
> > going to become a reoccurring nuisance. And in experiments with lambdas, which \
> > requires templates, this very quickly showed up as a problem. 
> > I propose that we solve most (all?) of these issues by adding a rule that states \
> > that .inline.hpp files should start by including the corresponding .hpp, and that \
> > the .hpp should contain the declarations of all externally used parts of the \
> > .inline.hpp file. This should guarantee that the declarations are always present \
> > before any subsequent include can create a circular include dependency back to \
> > the original file. 
> > In the example above, b.inline.hpp would become:
> > 
> > // b.inline.hpp
> > #pragma once
> > 
> > #include "b.hpp"
> > #include "a.inline.hpp"
> > 
> > inline void b1() {
> > }
> > 
> > inline void b2() {
> > a1();
> > }
> > 
> > and now both a.cpp and b.cpp compiles. The generated output now looks like this:
> > 
> > # 1 "b.inline.hpp" 1
> > # 1 "b.hpp" 1
> > 
> > void b1();
> > void b2();
> > 
> > # 4 "b.inline.hpp" 2
> > # 1 "a.inline.hpp" 1
> > 
> > # 1 "a.hpp" 1
> > 
> > void a1();
> > void a2();
> > 
> > # 4 "a.inline.hpp" 2
> > 
> > inline void a1() {
> > b1();
> > }
> > 
> > inline void a2() {
> > }
> > 
> > # 5 "b.inline.hpp" 2
> > 
> > inline void b1() {
> > }
> > 
> > inline void b2() {
> > a1();
> > }
> > 
> > # 2 "b.cpp" 2
> > 
> > int main() {
> > b1();
> > 
> > return 0;
> > }
> > 
> > The declarations come first, and the compiler is happy. 
> > 
> > An alternative to this, that has been proposed by other HotSpot GC devs have been \
> > to always include all .hpp files first, and then have a section of .inline.hpp \
> > includes. I've experimented with that as well, but I think it requires more \
> > maintenance and vigilance of  *users* .inline.hpp files (add .hpp include to the \
> > first section, add .inline.hpp section to second). The proposed structures only \
> > requires extra care from *writers* of .inline.hpp files. All others can include \
> > .hpp and .inline.hpp as we've been doing for a long time now. 
> > Some notes about this patch:
> > 1) Some externally-used declarations have been placed in .inline.hpp files, and \
> > not in the .hpp. Those should be moved. I have not done that. 2) Some .inline.hpp \
> > files don't have a corresponding .hpp file. I could either try to fix that in \
> > this patch, or we could take care of that as separate patches later. 3) The style \
> > I chose was to add the .hpp include and a extra blank line, separating it from \
> > the rest of the includes. I think I like that style, because it makes it easy for \
> > those writing .inline.hpp to recognize this pattern. And it removes the confusion \
> > why this include isn't sorted into the other includes. 4) We have a few *special* \
> > platform dependent header files. Both those that simply are included into \
> > platform independent files, and those that inject code *into* the platform \
> > independent classes. Extra care, as always, need to be taken around those files. \
> > 5) Mostly tested locally, but I'll test on more platforms if the idea is \
> > accepted. 
> > What do you think?
> 
> Stefan Karlsson has updated the pull request with a new target base due to a merge \
> or a rebase. The pull request now contains eight commits: 
> - Merge remote-tracking branch 'origin/master' into \
>                 8267464_circular-dependency_resilient_inline_headers
> - Merge remote-tracking branch 'origin/master' into \
>                 8267464_circular-dependency_resilient_inline_headers
> - Review kbarrett
> - Review dholmes
> - Update HotSpot style guide documents
> - Merge remote-tracking branch 'origin/master' into \
>                 8267464_circular-dependency_resilient_inline_headers
> - Clean up assembler_<cpu>.inline.hpp
> - 8267464: Circular-dependency resiliant inline headers

Thanks all for reviewing!

-------------

PR: https://git.openjdk.java.net/jdk/pull/4127


[prev in list] [next in list] [prev in thread] [next in thread]