universal-ctags / ctags

A maintained ctags implementation
https://ctags.io
GNU General Public License v2.0
6.51k stars 622 forks source link

build-sys: make lto optional #3978

Closed iLeeWell closed 5 months ago

iLeeWell commented 5 months ago

In addition to non-cross-compilation and compiler support for flto, add the condition that flto can only be enabled by actively using --enable-lto option.

I found a segment fault when ctags generates tags against parser_overflow.c (https://github.com/llvm/llvm-project/blob/main/clang/test/Parser/parser_overflow.c). The reason is that there are 16384 pairs of curly braces in the file (rare, deliberate and even tricky, but not wrong), and ctags, with flto optimization turned on, inflates the stack frame size with a large number of IPOs, which easily exceeds the default system specified stack size. The fastest solution is to disable flto, or to expand the stack size by the user (ulimit -s unlimted). However, from the point of view of prioritizing stability over performance, the flto optimization should not be used by default (even if it is supported by the compiler), and the choice of whether or not to turn it on should be left to the user, although there may be better solutions.

This is my rough opinion so far.

leleliu008 commented 5 months ago

I think you want to change to "x$enable_lto" = "xyes"

leleliu008 commented 5 months ago

"x$enable_lto" != "xno" means --disable-lto isn't specified, in other words, if neither --enable-lto nor disable-lto is specified, then it would check if the comipler support LTO.

iLeeWell commented 5 months ago

You are right, thanks for your comments, i will make changes immediately.

leleliu008 commented 5 months ago

you need to run git rebase -i command to delete the first one commit and leave the last one commit.

iLeeWell commented 5 months ago

i tried, but it seems not work as expected..., when i use git rebase -i and then push, my fork ctags reject so i add -f to force this. I'll study it further.

masatake commented 5 months ago

I frequently use the following command sequence.

$ git checkout master
$ git pull upstream master:master
$ git checkout add-condition-for-lto
$ git rebase master

or

$ git checkout master
$ git pull upstream master:master
$ git checkout -b add-condition-for-lto-v2
$ git cherry-pick b2e37a2
$ git push force origin add-condition-for-lto-v2:add-condition-for-lto
masatake commented 5 months ago

The change looks good.

I would like you to add the document about --enable-lto to docs/autotools.rst.

diff --git a/docs/autotools.rst b/docs/autotools.rst
index 0c0693105..2fdbf3639 100644
--- a/docs/autotools.rst
+++ b/docs/autotools.rst
@@ -71,6 +71,11 @@ To completely change the program's name run the following:

 Please remember there is also an 'etags' installed alongside 'ctags' which you may also want to rename as shown above.

+
+Link time optimization
+,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
+...TBW...
+
 Cross-compilation
 ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
iLeeWell commented 5 months ago

Thank you so much. @masatake @leleliu008

iLeeWell commented 5 months ago

The change looks good.

I would like you to add the document about --enable-lto to docs/autotools.rst.

diff --git a/docs/autotools.rst b/docs/autotools.rst
index 0c0693105..2fdbf3639 100644
--- a/docs/autotools.rst
+++ b/docs/autotools.rst
@@ -71,6 +71,11 @@ To completely change the program's name run the following:

 Please remember there is also an 'etags' installed alongside 'ctags' which you may also want to rename as shown above.

+
+Link time optimization
+,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
+...TBW...
+
 Cross-compilation
 ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,

ok, i will do this tomorrow.

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.39%. Comparing base (1223439) to head (dba80c6). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3978 +/- ## ======================================= Coverage 85.39% 85.39% ======================================= Files 235 235 Lines 56609 56609 ======================================= Hits 48339 48339 Misses 8270 8270 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

iLeeWell commented 5 months ago

The change looks good. I would like you to add the document about --enable-lto to docs/autotools.rst.

diff --git a/docs/autotools.rst b/docs/autotools.rst
index 0c0693105..2fdbf3639 100644
--- a/docs/autotools.rst
+++ b/docs/autotools.rst
@@ -71,6 +71,11 @@ To completely change the program's name run the following:

 Please remember there is also an 'etags' installed alongside 'ctags' which you may also want to rename as shown above.

+
+Link time optimization
+,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,
+...TBW...
+
 Cross-compilation
 ,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,

ok, i will do this tomorrow.

Supplementary description of Link time optimization. @masatake

masatake commented 5 months ago

@iLeeWell Thank you very much.

Could you change the subject of the commit log to "build-sys: make lto optional" (or something better) ?

iLeeWell commented 5 months ago

@iLeeWell Thank you very much.

Could you change the subject of the commit log to "build-sys: make lto optional" (or something better) ?

Of course.

masatake commented 5 months ago

Thank you.