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

"make profile-opt" overrides CFLAGS_NODIST #79680

Closed
vstinner opened this issue Dec 14, 2018 · 10 comments
Closed

"make profile-opt" overrides CFLAGS_NODIST #79680

vstinner opened this issue Dec 14, 2018 · 10 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build

Comments

@vstinner
Copy link
Member

BPO 35499
Nosy @gpshead, @vstinner, @ned-deily
PRs
  • bpo-35499: make profile-opt don't override CFLAGS_NODIST #11164
  • [3.7] bpo-35499: make profile-opt don't override CFLAGS_NODIST (GH-11164) #11179
  • bpo-35499: Fix LDFLAGS in "make build_all_generate_profile" #11219
  • [3.6] bpo-35499: make profile-opt don't override CFLAGS_NODIST (GH-11164) #11267
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2018-12-19.12:22:20.280>
    created_at = <Date 2018-12-14.18:25:57.266>
    labels = ['3.8', 'build', '3.7']
    title = '"make profile-opt" overrides CFLAGS_NODIST'
    updated_at = <Date 2018-12-24.16:32:11.222>
    user = 'https://github.com/vstinner'

    bugs.python.org fields:

    activity = <Date 2018-12-24.16:32:11.222>
    actor = 'ned.deily'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-12-19.12:22:20.280>
    closer = 'vstinner'
    components = ['Build']
    creation = <Date 2018-12-14.18:25:57.266>
    creator = 'vstinner'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 35499
    keywords = ['patch']
    message_count = 10.0
    messages = ['331847', '331851', '331856', '331930', '331940', '332079', '332104', '332134', '332253', '332486']
    nosy_count = 3.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'ned.deily']
    pr_nums = ['11164', '11179', '11219', '11267']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue35499'
    versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

    @vstinner
    Copy link
    Member Author

    Makefile.pre.in contains the rule:

    build_all_generate_profile:
    $(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS) $(PGO_PROF_GEN_FLAG)" LDFLAGS="$(LDFLAGS) $(PGO_PROF_GEN_FLAG)" LIBS="$(LIBS)"

    I'm not sure that CFLAGS_NODIST="$(CFLAGS) $(PGO_PROF_GEN_FLAG)" is correct: it overrides user $CFLAGS_NODIST variable. I suggest to replace it with CFLAGS_NODIST="$(CFLAGS_NODIST) $(PGO_PROF_GEN_FLAG)": add $(PGO_PROF_GEN_FLAG) to CFLAGS_NODIST, don't copy $CFLAGS to $CFLAGS_NODIST (and add $(PGO_PROF_GEN_FLAG)).

    The code comes from bpo-23390:

    commit 2f90aa6
    Author: Gregory P. Smith <greg@krypto.org>
    Date: Wed Feb 4 02:11:56 2015 -0800

    Fixes bpo-23390: make profile-opt causes -fprofile-generate and related flags
    to end up in distutils CFLAGS.
    

    (...)
    build_all_generate_profile:

    •   $(MAKE) all CFLAGS="$(CFLAGS) -fprofile-generate" LIBS="$(LIBS) -lgcov"
      

    + $(MAKE) all CFLAGS_NODIST="$(CFLAGS) -fprofile-generate" LDFLAGS="-fprofile-generate" LIBS="$(LIBS) -lgcov"
    (...)

    CFLAGS_NODIST has been added by bpo-21121:

    commit acb8c52
    Author: Benjamin Peterson <benjamin@python.org>
    Date: Sat Aug 9 20:01:49 2014 -0700

    add -Werror=declaration-after-statement only to stdlib extension modules (closes bpo-21121)
    
    Patch from Stefan Krah.
    

    This issue is related to bpo-35257: "Avoid leaking linker flags into distutils: add PY_LDFLAGS_NODIST".

    @vstinner vstinner added 3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build labels Dec 14, 2018
    @vstinner
    Copy link
    Member Author

    I wrote PR 11164 to fix the issue.

    Example:

    $ git clean -fdx
    $ ./configure --with-pydebug
    $ make profile-opt CFLAGS_NODIST="-O1"
    (...)
    gcc -pthread -c -Wno-unused-result -Wsign-compare -g -Og -Wall    -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fprofile-generate -I./Include/internal  -I. -I./Include    -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c
    (...)

    => CFLAGS_NODIST is missing: I don't see -O1 in the command line.

    With my change:

    $ git clean -fdx
    $ ./configure --with-pydebug
    $ make profile-opt CFLAGS_NODIST="-O1"
    (...)
    gcc -pthread -c -Wno-unused-result -Wsign-compare -g -Og -Wall    -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -O1 -fprofile-generate -I./Include/internal  -I. -I./Include    -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c
    (...)

    => CFLAGS_NODIST is used: I see "-O1" in the command line.

    @vstinner
    Copy link
    Member Author

    I also tested CFLAGS, just in case.

    Current behavior:

    $ git clean -fdx
    $ ./configure --with-pydebug
    $ make profile-opt CFLAGS="-O1"
    (...)
    gcc -pthread -c -Wno-unused-result -Wsign-compare -g -Og -Wall  -O1  -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fprofile-generate -I./Include/internal  -I. -I./Include    -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c
    (...)

    => CFLAGS is respected: I see -O1 in the command line.

    With PR 11164:

    $ git clean -fdx
    $ ./configure --with-pydebug
    $ make profile-opt CFLAGS="-O1"
    (...)
    gcc -pthread -c -Wno-unused-result -Wsign-compare -g -Og -Wall  -O1  -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -fprofile-generate -I./Include/internal  -I. -I./Include    -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c
    (...)

    => CFLAGS is respected: I see -O1 in the command line.

    @vstinner
    Copy link
    Member Author

    New changeset 640ed52 by Victor Stinner in branch 'master':
    bpo-35499: make profile-opt don't override CFLAGS_NODIST (GH-11164)
    640ed52

    @vstinner
    Copy link
    Member Author

    New changeset 9a47585 by Victor Stinner (Miss Islington (bot)) in branch '3.7':
    bpo-35499: make profile-opt don't override CFLAGS_NODIST (GH-11164) (GH-11179)
    9a47585

    @vstinner
    Copy link
    Member Author

    Oh wait, "make build_all_generate_profile" and "make profile-opt" have another issue. They modify LDFLAGS, whereas PGO flags seem to be very specific to the compiler, not to the linker.

    I reopen the issue.

    build_all_generate_profile:
    $(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS_NODIST) $(PGO_PROF_GEN_FLAG)" LDFLAGS="$(LDFLAGS) $(PGO_PROF_GEN_FLAG)" LIBS="$(LIBS)"

    profile-opt: profile-run-stamp
    ...
    $(MAKE) @DEF_MAKE_RULE@ CFLAGS_NODIST="$(CFLAGS_NODIST) $(PGO_PROF_USE_FLAG)" LDFLAGS="$(LDFLAGS)"

    LDFLAGS="$(LDFLAGS) $(PGO_PROF_GEN_FLAG)" of "make build_all_generate_profile" looks harmless: passing PGO flags to the linker works since gcc is used as the linker. Except that LDFLAGS is exported and used by distutils, and passing PGO flags to build third party code is not ok: see bpo-35257.

    For "make profile-opt", LDFLAGS="$(LDFLAGS)" looks useless.

    PGO flags depend on the compiler:

    • clang

      • PGO_PROF_GEN_FLAG="-fprofile-instr-generate"
      • PGO_PROF_USE_FLAG="-fprofile-instr-use=code.profclangd"
    • gcc:

    • PGO_PROF_GEN_FLAG="-fprofile-instr-generate"

    • PGO_PROF_USE_FLAG="-fprofile-instr-use=code.profclangd"

    • ICC:

      • PGO_PROF_GEN_FLAG="-prof-gen"
      • PGO_PROF_USE_FLAG="-prof-use"
    • Default:

      • PGO_PROF_GEN_FLAG="-fprofile-generate"
      • PGO_PROF_USE_FLAG="-fprofile-use -fprofile-correction"

    I don't think that any of these flags should be passed to the LDFLAGS. Passing these flags to CFLAGS should be enough.

    @vstinner vstinner reopened this Dec 18, 2018
    @gpshead
    Copy link
    Member

    gpshead commented Dec 19, 2018

    But the build_all_generate_profile build is an intermediate instrumented interpreter build, it isn't shipped and things like PGO often require flags that the linker sees in order to generate the instrumented binary.

    If those are left off of the link step, you won't have an instrumented binary and won't generate profile data.

    @vstinner
    Copy link
    Member Author

    (...) things like PGO often require flags that the linker sees in order to generate the instrumented binary. If those are left off of the link step, you won't have an instrumented binary and won't generate profile data.

    Oh, I didn't try my PR...

    $ ./configure --enable-optimizations
    $ make
    ...
    /usr/bin/ld: libpython3.8m.a(myreadline.o):(.data+0xa0): undefined reference to `__gcov_merge_add'
    ...

    My PR simply doesn't work: we have to pass PGO flags to the linker. At least for the first step generating a profile.

    My bad, sorry, I close my PR 11219.

    @ned-deily
    Copy link
    Member

    New changeset 782e1d5 by Ned Deily (Victor Stinner) in branch '3.6':
    bpo-35499: make profile-opt don't override CFLAGS_NODIST (GH-11164) (GH-11267)
    782e1d5

    @ned-deily
    Copy link
    Member

    New changeset 7e4e4bd by Ned Deily (Miss Islington (bot)) in branch '3.7':
    bpo-35499: make profile-opt don't override CFLAGS_NODIST (GH-11164) (GH-11179)
    7e4e4bd

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes build The build process and cross-build
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants