Make -DCMAKE_INTERPROCEDURAL_OPTIMIZATION Enable LTO

[linuxrocks123]

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]

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

[linuxrocks123]

@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.

[linuxrocks123]

@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

[fruffy]

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

[linuxrocks123]

@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]

@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]

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]

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]

Please add a comment why these prototypes are necessary.

[vlstill]

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]

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

[linuxrocks123]

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]

Maybe we should replace ENABLE_LTO with CMAKE_INTERPROCEDURAL_OPTIMIZATION.

[fruffy]

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]

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]

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

[linuxrocks123]

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]

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]

@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]

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]

@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]

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]

@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]

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]

@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]

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]

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]

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.