Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for #2885 #2893

Closed
wants to merge 2 commits into from
Closed

Fix for #2885 #2893

wants to merge 2 commits into from

Conversation

kloczek
Copy link

@kloczek kloczek commented Mar 1, 2021

This PR fixes #2885.
It also removes using AC_PROG_CC_C99 as autoupdate from autoconf 2.71 suggests.

Your Name added 2 commits March 1, 2021 14:29
…2885

This commit fixes universal-ctags#2885 which is caused by use custom AX_PROG_CC_FOR_BUILD
aclocal macro which does not honor $CC abd other env varuiables.
With that simplification packcc binary is compiled and linked ans non installeable
program with only standard AC_PROG_CC macro.
It simplifirs as well automake by building packcc as noinst_PROGRAMS, and because
that custon clean-local targert can be removed as well.
After run autoupdate from autoconf 2.71 it removes using AC_PROG_CC_C99
as obsoleted macro

-AC_PROG_CC_C99
+m4_warn([obsolete],
+[AC_PROG_CC_C99 is obsolete; use AC_PROG_CC
+])dnl
+AC_REQUIRE(AC_PROG_CC)

After clean above left comment this patch only removes using AC_PROG_CC_C99.
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #2893 (7a1d5e0) into master (98ff77d) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2893   +/-   ##
=======================================
  Coverage   87.09%   87.09%           
=======================================
  Files         194      194           
  Lines       44403    44403           
=======================================
  Hits        38674    38674           
  Misses       5729     5729           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61166a7...59f61bb. Read the comment docs.

@masatake
Copy link
Member

masatake commented Mar 1, 2021

run units target on OpenBSD / testing (pull_request) Failing after 8m — testing :

    default:   CC       main/libctags_a-debug.o
    default:   CC       misc/packcc/packcc.o
    default: misc/packcc/packcc.c: In function 'unescape_string':
    default: misc/packcc/packcc.c:414: error: 'for' loop initial declaration used outside C99 mode
    default: misc/packcc/packcc.c:426: error: 'for' loop initial declaration used outside C99 mode
    default: *** Error 1 in . (Makefile:2199 'misc/packcc/packcc.o': @echo "  CC      " misc/packcc/packcc.o;depbase=`echo misc/packcc/packcc.o | sed 's|...)
    default: *** Error 2 in /home/vagrant/universal-ctags/ctags (Makefile:1348 'all')
The SSH command responded with a non-zero exit status. Vagrant

bin_PROGRAMS = ctags
noinst_LIBRARIES = libctags.a

noinst_PROGRAMS =
noinst_PROGRAMS = packcc
noinst_PROGRAMS += mini-geany

if HAVE_STRNLEN_FOR_BUILD
EXTRA_CPPFLAGS_FOR_BUILD = -DUSE_SYSTEM_STRNLEN
Copy link
Member

@masatake masatake Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must pass this flag when building packcc.
We can use packcc_CFLAGS.

AC_PROG_RANLIB
AC_C_BIGENDIAN

# https://www.gnu.org/software/autoconf-archive/ax_prog_cc_for_build.html
AX_PROG_CC_FOR_BUILD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was introduced in #2747 to fix the cross-compilation failure.
Removing this will break it again, I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. Why the circle-ci doesn't report the failure of the cross-compilation.

@k-takata
Copy link
Member

k-takata commented Mar 1, 2021

Removing AC_PROG_CC_C99 breaks the builds on older systems.
Even if it causes a warning on autoconf 2.71, it should be kept for those systems.

@masatake
Copy link
Member

masatake commented Mar 1, 2021

I just remembered that we used noinst_PROGRAMS, and @leleliu008 rodded it cross-compilation.
See https://github.com/universal-ctags/ctags/pull/2747/files#diff-0462e381b2fb3286568215681c8983490a37ac9ae0f0c5ee304df7fa6426d4afL39.

@masatake
Copy link
Member

masatake commented Mar 1, 2021

BTW, Is there a way to trigger circle-ci forcedly?

@kloczek
Copy link
Author

kloczek commented Mar 1, 2021

I just remembered that we used noinst_PROGRAMS, and @leleliu008 rodded it cross-compilation.
See https://github.com/universal-ctags/ctags/pull/2747/files#diff-0462e381b2fb3286568215681c8983490a37ac9ae0f0c5ee304df7fa6426d4afL39.

Maybe it fixes crosscompilation but it breaks normal build as well when LTO and autoconf 2.71 are used.
Issue is that using AX_PROG_CC_FOR_BUILD faiils LTO and probably PGO optimisation which are controlled over env variables like $CFLAGS $CXXFLAGS $FFLAGS $FCFLAGS $LDFLAGS $CC $CXX $FC $AR $NM $RANLIB.
In case of that macro $CC is not used but other are still in use ($FCFLAGS $LDFLAGS $AR $NM $RANLIB)

I would suggest to repeat crosscompilation with autoconf 2.71.
After 7 years since last release of autoconf 2.69 in 2014 many changes related to crosscompilation in autoconf have been made.

In other words whoever will try to use LTO will bounce with that issue.

@kloczek
Copy link
Author

kloczek commented Mar 1, 2021

Removing AC_PROG_CC_C99 breaks the builds on older systems.

As I wrote I'm using now autoconf 2.71 and if yopu will try to run autoupdate it will add comment that with that version of ac AC_PROG_CC_C99 iis now obsoleted and ``AC_PROG_CC` provides all what is needed.

@masatake
Copy link
Member

masatake commented Mar 1, 2021

We cannot accept the change breaking the cross-compilation feature and the support for OpenBSD.
circle.yml at the top of the source tree shows how we test the cross-compilation feature.

Please, submit a pull request that can pass all test cases on the all CI/CD environment.

If you say LTO is more important than the cross-compilation feature and the support for OpenBSD, please show how it improves the performance of ctags. You can use https://github.com/univrsal-ctags/coebase. If LTO improves the performance much, we have to consider the way to introduce LTO without breaking the cross-compilation feature and the support for OpenBSD.

@masatake masatake closed this Mar 1, 2021
@kloczek
Copy link
Author

kloczek commented Mar 1, 2021

I understand that.
Nevertheless when more people will be using latest autoconf that patch still may be usefull :)

@k-takata
Copy link
Member

k-takata commented Mar 1, 2021

You mentioned LTO. Did you set $CFLAGS or something to enable LTO?
(You didn't show the command line of ./configure in #2885.)

@masatake
Copy link
Member

masatake commented Mar 2, 2021

You mentioned LTO. Did you set $CFLAGS or something to enable LTO?

I got the same question.
For turning on LTO in building ctags, I cannot find any reason to modify the way for cross-compiling.
packcc is a code generator. We don't have to turn on LTO for packcc.

@kloczek
Copy link
Author

kloczek commented Mar 2, 2021

You mentioned LTO. Did you set $CFLAGS or something to enable LTO?

Yes.

[tkloczko@barrel SPECS]$ rpm -E %set_build_flags

CFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none";
CXXFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none";
FFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -I/usr/lib64/gfortran/modules";
FCFLAGS="-O2 -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fdata-sections -ffunction-sections -flto=auto -flto-partition=none -I/usr/lib64/gfortran/modules";
LDFLAGS="-Wl,-z,relro -Wl,--as-needed -Wl,--gc-sections -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -flto=auto -flto-partition=none -fuse-linker-plugin";
CC="/usr/bin/gcc"; CXX="/usr/bin/g++"; FC="/usr/bin/gfortran";
AR="/usr/bin/gcc-ar"; NM="/usr/bin/gcc-nm"; RANLIB="/usr/bin/gcc-ranlib";
export CFLAGS CXXFLAGS FFLAGS FCFLAGS LDFLAGS CC CXX FC AR NM RANLIB;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enabling LTO (Was: p5.9.20210221.0: build fails)
3 participants