Make -DCMAKE_INTERPROCEDURAL_OPTIMIZATION Enable LTO #4908

linuxrocks123 · 2024-09-12T20:00:34Z

Turning on CMAKE_INTERPROCEDURAL_OPTIMIZATION is CMake's standard way of enabling LTO. This PR makes that option enable LTO for p4c. It also fixes linker errors which occur due to p4c's garbage-collected implementations of the C standard memory allocation functions being discarded when LTO is enabled. This is avoided by prototyping these functions with __attribute__((used)). Exception specifications are included for these prototypes when compiling on glibc since Clang requires the prototypes to exactly match those from whatever C standard library it is being compiled against.

ChrisDodd

IMO it would be a good idea to check this against more OS versions than we do with just CI, but I'm not sure how easy it is to do that. Adding more builds to CI might be getting excessive.

ChrisDodd · 2024-09-12T21:32:14Z

lib/gc.cpp

@@ -81,6 +81,33 @@ static void maybe_initialize_gc() {
    }
 }

+#ifdef __GLIBC__
+#define GNOEXCEPT __THROW
+#else


It seems that this should depend on the version of glibc you're linking against as different version have different exception specifications?

@ChrisDodd the __THROW specifier is defined as a macro in cdefs.h and appears to be whatever glibc wants to use. With any remotely modern compiler, it will always decay noexcept, but it will decay to throw() for C++03 and earlier. I don't believe p4c supports C++03, so we could just use noexcept, but perhaps __THROW is slightly more future-proof in case glibc changes its mind about what exception specification it wants to use.

@ChrisDodd and that __THROW macro has been in glibc since at least 1999, so it should work: https://github.com/bminor/glibc/blob/c1422e5b7cdb4400f934c91bcefa3a1a96d789fb/misc/sys/cdefs.h

I would stick to noexcept to keep things simple? If the semantics of __THROW changed we can revisit.

@fruffy I originally had it as noexcept, and I don't like __THROW because we're reaching using internal glibc macros, which I'm pretty sure we're not supposed to do. So, I'm happy to change it back.

linuxrocks123 · 2024-09-12T22:34:50Z

@ChrisDodd __attribute__((used)) is present in both GCC and Clang, so that part of the code shouldn't break anything except MSVC++, which it seems like p4c already doesn't support. If p4c does support MSVC++, the __attribute__((used)) will need to be #ifdefed to whatever MSVC uses to mean the same thing.

The __THROW thing is #ifdefed to nothing on Clang, and it appears glibc will always define its memory allocation functions using that macro, so that shouldn't break anything either.

If we had one additional platform to test, I'd recommend GCC compiled against glibc on MacOS. However, I can't personally test that.

linuxrocks123 · 2024-09-13T02:34:50Z

The sanitizer failure is very plausible since the constructor for MoveInitializers leaves the loopsBackToStart boolean uninitialized, but it is not particularly likely that this PR could itself contain a bug manifesting as memory corruption, so my guess is that the memory corruption bug is real but unrelated to this PR. Do we know whether test-p4c-ubuntu-20.04-sanitizers is passing on master?

fruffy · 2024-09-13T06:34:33Z

The sanitizer failure is very plausible since the constructor for MoveInitializers leaves the loopsBackToStart boolean uninitialized, but it is not particularly likely that this PR could itself contain a bug manifesting as memory corruption, so my guess is that the memory corruption bug is real but unrelated to this PR. Do we know whether test-p4c-ubuntu-20.04-sanitizers is passing on master?

To save CI resources that check is optional, I am guessing #4909 was merged without enabling the sanitizer for it. ~~@kfcripps~~ I see there is already a related PR #4910.

fruffy · 2024-09-13T06:38:17Z

lib/gc.cpp

+#else
+#define GNOEXCEPT
+#endif
+void *operator new(std::size_t size) __attribute__((used));


Please add a comment why these prototypes are necessary.

Suggested change

void *operator new(std::size_t size) __attribute__((used));

[[gnu::used]] void *operator new(std::size_t size);

Would this work? This syntax is safer as other compilers will just ignore it, unlike the __attribute__ syntax.

Also, the exception declarations don't still don't match those in <new> which could be a problem.

It may be that we don't need to be messing with new and friends at all. Let me check to see. I'm going to try to reproduce the original issue with p4c master since the problem originally presented in an internal branch.

fruffy · 2024-09-13T06:40:38Z

CMakeLists.txt

@@ -56,7 +56,7 @@ STATIC_BUILD_WITH_DYNAMIC_STDLIB." OFF)
 OPTION (STATIC_BUILD_WITH_DYNAMIC_STDLIB "Build a (mostly) statically linked release binary. \
 Glibc and C++ standard library is linked dynamically." OFF)

-if (STATIC_BUILD_WITH_DYNAMIC_GLIBC OR STATIC_BUILD_WITH_DYNAMIC_STDLIB)
+if (STATIC_BUILD_WITH_DYNAMIC_GLIBC OR STATIC_BUILD_WITH_DYNAMIC_STDLIB OR CMAKE_INTERPROCEDURAL_OPTIMIZATION)


Maybe we should replace ENABLE_LTO with CMAKE_INTERPROCEDURAL_OPTIMIZATION.

fruffy · 2024-09-13T06:42:06Z

lib/gc.cpp

@@ -81,6 +81,33 @@ static void maybe_initialize_gc() {
    }
 }

+#ifdef __GLIBC__
+#define GNOEXCEPT __THROW
+#else


I would stick to noexcept to keep things simple? If the semantics of __THROW changed we can revisit.

vlstill

Why is the __extrarnally_visible__ attribute and _GLIBCXX_NODISCARD used in <new> not enough? This seems hightly suspicious to me. Also, you may be hiding that attribute by the new declarations, I'm not sure how the double declaration gets resolved.

vlstill · 2024-09-13T07:39:14Z

lib/gc.cpp

+#else
+#define GNOEXCEPT
+#endif
+void *operator new(std::size_t size) __attribute__((used));


Suggested change

void *operator new(std::size_t size) __attribute__((used));

[[gnu::used]] void *operator new(std::size_t size);

Would this work? This syntax is safer as other compilers will just ignore it, unlike the __attribute__ syntax.

vlstill · 2024-09-13T07:44:20Z

lib/gc.cpp

+#else
+#define GNOEXCEPT
+#endif
+void *operator new(std::size_t size) __attribute__((used));


Also, the exception declarations don't still don't match those in <new> which could be a problem.

linuxrocks123 · 2024-09-13T15:20:29Z

I'm off today and Monday and so will make changes to the PR to address reviewer comments on Tuesday. For now, I have no objections to replacing __THROW with noexcept and agree that [[gnu::used]] is better than __attribute__((used)).

asl · 2024-09-13T17:43:25Z

We had issues in the past at least with unity builds as things might got redeclared multiple times due to sources / headers combined. This is why we had this __THROW optionally added for posix_memalign for glibc. Glibc is just broken as it adds noexcept specifier to C-style declarations (contrary to what is in the standard). Therefore we just need to exactly match glibc to ensure both forward and backward compatibility.

gcc is currently quite lax in terms of checking (invalid) redeclaration w/o noexcept. clang is much more strict, but has explicit egregious workaround for this particular case. As far as I know, other standard libraries including musl and bionic are fine and standard-compliant wrt these declarations.

The unity scenario was as follows:

gc.cpp was added to unity pack first, and had declaration with noexcept
further, declaration from standard library / other header was added causing a redeclaration
the redeclaration was w/o noexcept causing type mismatches

linuxrocks123 · 2024-09-17T19:51:05Z

@asl-AMD, glibc has never claimed to restrict its implementing code to anything other than the GNU standards of C and C++. clang can't even compile glibc because clang only has partial support for the GNU standards.

The only guarantee glibc makes about the ISO standards is that ISO-compliant code should build and run when compiled and linked against glibc using gcc with an ISO dialect flag. Whether malloc and friends are declared with noexcept doesn't break that guarantee since it doesn't break ISO code compiled with gcc.

So, I don't think we need to reach into glibc's internals and use __THROW instead of noexcept. Putting noexcept on malloc and friends is not a bug, so glibc is unlikely to de-optimize their code by removing noexcept from those prototypes.

asl · 2024-09-17T19:57:03Z

The only guarantee glibc makes about the ISO standards is that ISO-compliant code should build and run when compiled and linked against glibc using gcc with an ISO dialect flag.

I really do not want to debate about things like this. But do you happen to have a link to this specification? Is would be very useful to insert reference to this specification in the comments so whoever will change the code in the future would be aware about these restrictions.

There are C and C++ standards. And the standard library is expected to follow these standard before everything else. The standard specifies that these functions are declared without noexcept specifier. And the same standard specifies that you cannot redeclare a noexcept function afterwards without noexcept.

linuxrocks123 · 2024-09-17T20:52:53Z

@asl well, sure, I don't want to debate this either :) That's why I originally changed it for you without discussion. But, I do think it's somewhat better with noexcept since the alternative uses an internal glibc macro that I don't think we have glibc's permission to be relying on.

Here's the link I was referring to: https://www.gnu.org/software/libc/manual/html_node/ISO-C.html

asl · 2024-09-17T20:55:07Z

Here's the link I was referring to: https://www.gnu.org/software/libc/manual/html_node/ISO-C.html

Great, thanks. Could you please refer me to noexcept in ISO/IEC 9899:1990, “Programming languages—C” standard then?

linuxrocks123 · 2024-09-17T21:11:14Z

@asl I don't think the noexcept keyword exists in ISO C or in GNU C. It looks like __THROW decays to __attribute__ ((__nothrow__ __LEAF)) when compiling under C instead of C++. __LEAF is another macro that sometimes decays into more compiler-specific junk.

p4c is written in C++, though, so __THROW will always decay to noexcept for us.

Re what glibc is "expected" to do, the GNU Project has the same rights as anyone to publish whatever code it wants to. ISO standards have no legal or moral authority; they merely exist to help implementations and users when they happen to be helpful. If glibc feels its users are better served by putting noexcept on those prototypes than by not doing that, that makes sense to me, since putting noexcept helps gcc generate better code, and glibc expects to be compiled with gcc.

asl · 2024-09-17T21:25:00Z

Re what glibc is "expected" to do, the GNU Project has the same rights as anyone to publish whatever code it wants to. ISO standards have no legal or moral authority; they merely exist to help implementations and users when they happen to be helpful. If glibc feels its users are better served by putting noexcept on those prototypes than by not doing that, that makes sense to me, since putting noexcept helps gcc generate better code, and glibc expects to be compiled with gcc.

This is certainly fine and understood. However, p4c as a project is expected to be compiled with different compilers (e.g. clang) and run on different platforms that may deploy different standard libraries. This includes, for example:

clang on Linux and MacOS
I do not know if p4c is expected to be compiled and run on other POSIX-aware platforms such as FreeBSD, so let's put this away for a moment
Different standard libraries even if compiled by gcc: we can have libc on MacOS, musl on Linux (e.g. inside a docker container), there is also bionic around. Some of these cases are more ephemeral, but others are pretty real
Besides LTO p4c supports unity builds as a "poor-man LTO" and historically this was the default configuration

My point is that these cases should be supported as well. We know that in the past that not accounting for glibc differences caused issues. At least in unity builds IIRC. So when introducing changes into functions that are supposed to override whatever standard library provides we need to be very careful and certainly not to prefer one configurations to others. I believe one plausible breakage scenario in the past was mentioned above. Maybe there are others.

linuxrocks123 · 2024-09-17T22:13:25Z

@asl yes, we shouldn't push code to p4c that breaks clang in a supported or common configuration. That's why we need the GNOEXCEPT thing. The two proposals are:

#define GNOEXCEPT noexcept on glibc
#define GNOEXCEPT __THROW on glibc

Either of these works, and I don't think it's a huge deal which one we use. noexcept has the advantage that we don't invoke a macro which appears to be internal to glibc and not for public consumption. __THROW has the advantage of possibly continuing to work if glibc changes its mind about putting noexcept on malloc and friends.

I think the better way is to #define GNOEXCEPT noexcept because I don't think glibc will pessimize its implementation by removing noexcept from those function declarations. That project only supports being compiled with GCC, so breaking Clang or carrying non-ISO code in its implementation aren't going to be things it cares about addressing.

I've said all I care to say about this issue now and will defer to the consensus of the reviewers on which way to go.

linuxrocks123 · 2024-09-18T00:27:51Z

I'm having difficulty reproducing the link error with p4c master. If I can't, I'm not sure if there is sufficient motivation to have the malloc() and friends prototypes in this PR. I will continue investigating.

linuxrocks123 · 2024-09-18T20:05:33Z

The only place I can reproduce this error is inside a docker with a locally-compiled GCC 9.5 running on CentOS 7. So, the link problem is almost certainly a host compiler bug. That's probably a very unusual build environment, so I don't think we should add prototypes to work around that issue.

fruffy · 2024-09-18T23:51:10Z

The only place I can reproduce this error is inside a docker with a locally-compiled GCC 9.5 running on CentOS 7. So, the link problem is almost certainly a host compiler bug. That's probably a very unusual build environment, so I don't think we should add prototypes to work around that issue.

It looks like the only change here now is to add CMAKE_INTERPROCEDURAL_OPTIMIZATION as an ENABLE_LTO alternative. Is CMAKE_INTERPROCEDURAL_OPTIMIZATION the canonical CMake way to enable LTO? If so, we should use it and deprecate ENABLE_LTO in p4c.

linuxrocks123 · 2024-11-05T23:51:21Z

Hi @fruffy, yes I do think CMAKE_INTERPROCEDURAL_OPTIMIZATION is the canonical way to do this.

fruffy · 2024-11-06T03:05:08Z

Hi @fruffy, yes I do think CMAKE_INTERPROCEDURAL_OPTIMIZATION is the canonical way to do this.

If so, could you replace ENABLE_LTO with it? And for backward compatibility use ENABLE_LTO to enable CMAKE_INTERPROCEDURAL_OPTIMIZATION instead.

Signed-off-by: Patrick Simmons <[email protected]>

linuxrocks123 · 2024-11-06T23:06:26Z

@fruffy okay, done.

vlstill · 2024-11-07T05:55:41Z

CMakeLists.txt

 else()
-    set (_ENABLE_LTO_DEFAULT OFF)
+    set (CMAKE_INTERPROCEDURAL_OPTIMIZATION OFF)


Wouldn't this mess with the setting of CMAKE_INTERPROCEDRAL_OPTIMIZATION done on the commandline? Actually probably both branches would mess with it. The idea behind _ENABLE_LTO_DEFAULT was that it is still possible to do STATIC_* with LTO explicitly disabled, I'm worried this would no longer work.

@vlstill command-line arguments take precedence over anything in CMakeLists.txt

CMakeLists.txt

linuxrocks123 force-pushed the main branch from 271ac55 to 08cf16f Compare September 12, 2024 20:01

asl added run-ubuntu18 Use this tag to trigger a Ubuntu-18 CI run. run-sanitizer Use this tag to run a Clang+Sanitzers CI run. run-static Use this tag to trigger static build CI run. labels Sep 12, 2024

kfcripps requested review from asl, vlstill, fruffy and ChrisDodd September 12, 2024 21:09

ChrisDodd reviewed Sep 12, 2024

View reviewed changes

linuxrocks123 force-pushed the main branch from b9850b9 to 8200beb Compare September 12, 2024 22:42

fruffy reviewed Sep 13, 2024

View reviewed changes

vlstill reviewed Sep 13, 2024

View reviewed changes

linuxrocks123 force-pushed the main branch from f365f6b to 887a833 Compare September 18, 2024 20:12

kfcripps requested review from fruffy, vlstill and ChrisDodd September 18, 2024 21:36

linuxrocks123 force-pushed the main branch 2 times, most recently from 28d94ad to febf96b Compare November 6, 2024 19:57

linuxrocks123 added 2 commits November 6, 2024 14:58

Add option to enable interprocedural optimization

b2016e3

Signed-off-by: Patrick Simmons <[email protected]>

Use CMake-standard LTO option and deprecate ENABLE_LTO

539f11f

Signed-off-by: Patrick Simmons <[email protected]>

linuxrocks123 force-pushed the main branch from febf96b to 539f11f Compare November 6, 2024 19:59

vlstill requested changes Nov 7, 2024

View reviewed changes

linuxrocks123 requested a review from vlstill November 19, 2024 22:43

	void *operator new(std::size_t size) __attribute__((used));
	[[gnu::used]] void *operator new(std::size_t size);

Make -DCMAKE_INTERPROCEDURAL_OPTIMIZATION Enable LTO #4908

Are you sure you want to change the base?

Make -DCMAKE_INTERPROCEDURAL_OPTIMIZATION Enable LTO #4908

Conversation

linuxrocks123 commented Sep 12, 2024

ChrisDodd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linuxrocks123 Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linuxrocks123 commented Sep 12, 2024

linuxrocks123 commented Sep 13, 2024

fruffy commented Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vlstill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linuxrocks123 commented Sep 13, 2024

asl commented Sep 13, 2024 • edited Loading

linuxrocks123 commented Sep 17, 2024

asl commented Sep 17, 2024 • edited Loading

linuxrocks123 commented Sep 17, 2024

asl commented Sep 17, 2024

linuxrocks123 commented Sep 17, 2024 • edited Loading

asl commented Sep 17, 2024

linuxrocks123 commented Sep 17, 2024

linuxrocks123 commented Sep 18, 2024

linuxrocks123 commented Sep 18, 2024

fruffy commented Sep 18, 2024 • edited Loading

linuxrocks123 commented Nov 5, 2024

fruffy commented Nov 6, 2024 • edited Loading

linuxrocks123 commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linuxrocks123 Sep 12, 2024 •

edited

Loading

fruffy commented Sep 13, 2024 •

edited

Loading

asl commented Sep 13, 2024 •

edited

Loading

asl commented Sep 17, 2024 •

edited

Loading

linuxrocks123 commented Sep 17, 2024 •

edited

Loading

fruffy commented Sep 18, 2024 •

edited

Loading

fruffy commented Nov 6, 2024 •

edited

Loading