Fix Link-time optimization build error

While this makes sense, I've never seen an error while compiling this. Is it system-specific?

Please add more info about the reasoning behind this. To me to looks like a hack.

FYI, I think this is suggested here:
https://gcc.gnu.org/wiki/LinkTimeOptimizationFAQ

I would rather not add optimization flags to our configuration and leave that up to the person building KiCad. I'm guessing most linux package devs would agree with this. It should be easy enough to pass this compiler flag on the command line. As it is, this compiler flag is gcc specific and may cause issues with other compilers.

GCC and Clang: https://llvm.org/docs/LinkTimeOptimization.html

The document Seth referred to is indeed what I based the patch on.

I submitted the patch exactly because this is controlled by the KiCad build system, without patching it one can only disable the whole LTO system because of the homebrew co-routine library (hack).

How about setting it only if GCC or Clang is detected?

if ("${CMAKE_CXX_COMPILER_ID}" MATCHES "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
    set_source_files_properties(system/libcontext.cpp PROPERTIES COMPILE_FLAGS "-fno-lto")
endif()

Matches is used for Clang to include Apple's Clang.

My rebuild without the patch just came to a halt at the error message related to this. Several similar instances follow.

CXXFLAGS="-march=haswell -mtune=skylake -O3 -pipe -fstack-protector-strong -flto -fno-fat-lto-objects -fuse-linker-plugin -fdiagnostics-color=always"
LDFLAGS="-Wl,-O1,--sort-common,--as-needed,-z,relro"

Some further background / results:

The KiCad package built from git master today is 93.97MiB according to pacman.
The LTO enabled package from the same source: 76.25MiB, roughly 20% smaller. It also, very subjectively, seems to load faster.

I would be willing to accept your second patch as long as we do some due diligence and thoroughly test this on every build platform we support to make sure it doesn't cause grief elsewhere. Also, please note the coding style of the cmake file you are modifying. While we do not have an official cmake coding policy, our cmake files try to follow the c coding policy as closely as possible.

I updated the formatting to match the rest of the file and added a comment with a short explanation for why the code is there.

@Stefan can you rebase this patch? It no longer applies cleanly to master.

Sure! Here's a patch that applies to the current head.

Thanks. This compiles without error under Ubuntu 16.04, 18.04 and Debian Buster using both clang and gcc.

Would someone with a Windows build machine be able to test a compile?

No problem to compile on W7 32 bits / mingw.
I made a fast test, and did not found issues.

Thanks JP!

My mac compile is running slowly. No issues so far (clang-only). Should finish in 1-2 hours :)

The conditional keeps it out of msvc, so assuming no issues with mac, this should be clear to merge.

@Wayne, any objections?

@Seth, I just completed a 64 bit build and quick test on windows without any issues so I don't see any reason not to merge this patch. We probably should cherry-pick this to the 5.1 branch.

Fixed in revision e58d9606ddceb3cfd757fd3e203b2f974d566bd0
https://git.launchpad.net/kicad/patch/?id=e58d9606ddceb3cfd757fd3e203b2f974d566bd0

Sounds good. Patch pushed to master and 5.1. Thank you Stefan!

PLD linux carries a patch which uses a "naked" function instead of a toplevel asm function to allow the compiler to properly track the function.

http://git.pld-linux.org/?p=packages/kicad.git;f=lto.patch;h=ad2c2f023eb7ffeefef2c1fd4cff1fdc9f6b3156;hb=HEAD

I have tried it, and it works fine. The resulting assembly is (almost) the same as the plain assembly, you can try it with the following code:

$> cat ./test.cpp
---
void foo(int* a, char b) __attribute__ ((naked, aligned(4)));
void foo(int* a, char b)
{
asm volatile("mov $1, %eax;");
}
---
$> gcc -S ./test.cpp; gcc -c ./test.cpp; cat ./test.s; objdump -d ./test.o
---
        .file "test.cpp"
        .text
        .align 4
        .globl foo
        .type foo, @function
foo:
.LFB0:
        .cfi_startproc
#APP
# 5 "./test.cpp" 1
        mov $1, %eax;
# 0 "" 2
#NO_APP
        nop
        ud2
        .cfi_endproc
.LFE0:
        .size foo, .-foo
        .ident "GCC: (SUSE Linux) 9.1.1 20190703 [gcc-9-branch revision 273008]"
        .section .note.GNU-stack,"",@progbit
---
0000000000000000 <foo>:
   0: b8 01 00 00 00 mov $0x1,%eax
   5: 90 nop
   6: 0f 0b ud2
---

The only difference is the added nop,ud2. ud2 is the guaranteed intel illegal instruction, but as the code is not reached (last asm instruction is "hlt" respectively "jmp"), this does not matter.

The corresponding GCC BR is https://gcc.gnu.org/bugzilla/show_bug.cgi?id=57703

If this yielded a tangible benefit, it might be interesting. But I don't see the inherent upside (I may just be slow here)

We generate the basic libcontext from the Boost library, so diverging from them would make things harder going forward.

Now, if this helped with backtraces, perhaps the approach is worth looking into. Otherwise, I suspect that mucking about in the assembler for a few percent performance is asking for trouble.