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

EXTRA_CFLAGS get overrided by CFLAGS_NODIST #81812

Open
Dormouse759 mannequin opened this issue Jul 19, 2019 · 8 comments
Open

EXTRA_CFLAGS get overrided by CFLAGS_NODIST #81812

Dormouse759 mannequin opened this issue Jul 19, 2019 · 8 comments
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes build The build process and cross-build type-bug An unexpected behavior, bug, or error

Comments

@Dormouse759
Copy link
Mannequin

Dormouse759 mannequin commented Jul 19, 2019

BPO 37631
Nosy @vstinner, @stratakis, @hroncok, @Dormouse759
PRs
  • bpo-37631: Append $(EXTRA_CFLAGS) to $(CFLAGS_NODIST) as well #15020
  • 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 = None
    created_at = <Date 2019-07-19.11:49:12.515>
    labels = ['3.8', 'type-bug', '3.7', '3.9', 'build']
    title = 'EXTRA_CFLAGS get overrided by CFLAGS_NODIST'
    updated_at = <Date 2019-10-03.16:57:32.899>
    user = 'https://github.com/Dormouse759'

    bugs.python.org fields:

    activity = <Date 2019-10-03.16:57:32.899>
    actor = 'brett.cannon'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Build']
    creation = <Date 2019-07-19.11:49:12.515>
    creator = 'Dormouse759'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37631
    keywords = ['patch']
    message_count = 8.0
    messages = ['348168', '348183', '348296', '348734', '348737', '348739', '353781', '353785']
    nosy_count = 4.0
    nosy_names = ['vstinner', 'cstratak', 'hroncok', 'Dormouse759']
    pr_nums = ['15020']
    priority = 'normal'
    resolution = None
    stage = 'patch review'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue37631'
    versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

    @Dormouse759
    Copy link
    Mannequin Author

    Dormouse759 mannequin commented Jul 19, 2019

    Problem:
    If you want to override CFLAGS by setting EXTRA_CFLAGS, they may have no effect if there are contrary flags in the CFLAGS_NODIST variable.

    How to reproduce:
    make CFLAGS_NODIST="-O2" EXTRA_CFLAGS="-Og"

    If you look at GCC arguments, there is -O2 present *after* the -Og flag. This means -Og gets ignored.

    @Dormouse759 Dormouse759 mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes build The build process and cross-build type-bug An unexpected behavior, bug, or error labels Jul 19, 2019
    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Jul 19, 2019

    Could you provide more info, e.g. comparison between the proper and the erroneous output, as well as what it affects (test_gdb if I recall correctly)?

    @Dormouse759
    Copy link
    Mannequin Author

    Dormouse759 mannequin commented Jul 22, 2019

    I believe you mean this downstream issue: https://bugzilla.redhat.com/show_bug.cgi?id=1712977

    That issue is but only a consequence of a bad flag handling.
    The bad flag handling affects not only test_gdb on that specific architecture, but the entire optimization level on all architectures, hence causing inconveniences in debugging in general on all architectures. It's a pure chance that one test caught this specific case.

    It might also cause inconveniences with other use-cases for EXTRA_CFLAGS, as they might get overridden by CFLAGS_NODIST.

    You can get the erroneous output by running the reproducer. That is:

    $./configure
    ...
    $ make CFLAGS_NODIST="-O2" EXTRA_CFLAGS="-Og"
    gcc -pthread -c -Wno-unused-result -Wsign-compare -DNDEBUG -g -fwrapv -O3 -Wall -Og -std=c99 -Wextra -Wno-unused-result -Wno-unused-parameter -Wno-missing-field-initializers -Werror=implicit-function-declaration -O2 -I./Include/internal -I. -I./Include -DPy_BUILD_CORE -o Programs/python.o ./Programs/python.c

    -Og from EXTRA_CFLAGS gets overridden by -O2 from CFLAGS_NODIST.

    @vstinner
    Copy link
    Member

    Python has way too many variables to control compiler flags:
    ---
    # Compiler options
    OPT= @opt@
    BASECFLAGS= @BASECFLAGS@
    BASECPPFLAGS= @BASECPPFLAGS@
    CONFIGURE_CFLAGS= @CFLAGS@
    # CFLAGS_NODIST is used for building the interpreter and stdlib C extensions.
    # Use it when a compiler flag should _not_ be part of the distutils CFLAGS
    # once Python is installed (Issue bpo-21121).
    CONFIGURE_CFLAGS_NODIST=@CFLAGS_NODIST@
    # LDFLAGS_NODIST is used in the same manner as CFLAGS_NODIST.
    # Use it when a linker flag should _not_ be part of the distutils LDFLAGS
    # once Python is installed (bpo-35257)
    CONFIGURE_LDFLAGS_NODIST=@LDFLAGS_NODIST@
    CONFIGURE_CPPFLAGS= @CPPFLAGS@
    CONFIGURE_LDFLAGS= @LDFLAGS@
    # Avoid assigning CFLAGS, LDFLAGS, etc. so users can use them on the
    # command line to append to these values without stomping the pre-set
    # values.
    PY_CFLAGS= $(BASECFLAGS) $(OPT) $(CONFIGURE_CFLAGS) $(CFLAGS) $(EXTRA_CFLAGS)
    PY_CFLAGS_NODIST=$(CONFIGURE_CFLAGS_NODIST) $(CFLAGS_NODIST) -I$(srcdir)/Include/internal
    # Both CPPFLAGS and LDFLAGS need to contain the shell's value for setup.py to
    # be able to build extension modules using the directories specified in the
    # environment variables
    PY_CPPFLAGS= $(BASECPPFLAGS) -I. -I$(srcdir)/Include $(CONFIGURE_CPPFLAGS) $(CPPFLAGS)
    PY_LDFLAGS= $(CONFIGURE_LDFLAGS) $(LDFLAGS)
    PY_LDFLAGS_NODIST=$(CONFIGURE_LDFLAGS_NODIST) $(LDFLAGS_NODIST)
    NO_AS_NEEDED= @NO_AS_NEEDED@
    SGI_ABI= @SGI_ABI@
    CCSHARED= @CCSHARED@
    # LINKFORSHARED are the flags passed to the $(CC) command that links
    # the python executable -- this is only needed for a few systems
    LINKFORSHARED= @LINKFORSHARED@
    ARFLAGS= @ARFLAGS@
    # Extra C flags added for building the interpreter object files.
    CFLAGSFORSHARED=@CFLAGSFORSHARED@
    # C flags used for building the interpreter object files
    PY_STDMODULE_CFLAGS= $(PY_CFLAGS) $(PY_CFLAGS_NODIST) $(PY_CPPFLAGS) $(CFLAGSFORSHARED)
    PY_BUILTIN_MODULE_CFLAGS= $(PY_STDMODULE_CFLAGS) -DPy_BUILD_CORE_BUILTIN
    PY_CORE_CFLAGS= $(PY_STDMODULE_CFLAGS) -DPy_BUILD_CORE
    # Linker flags used for building the interpreter object files
    PY_CORE_LDFLAGS=$(PY_LDFLAGS) $(PY_LDFLAGS_NODIST)
    # Strict or non-strict aliasing flags used to compile dtoa.c, see above
    CFLAGS_ALIASING=@CFLAGS_ALIASING@
    ---

    I'm not sure which variables are "standard" and supposed to be overriden or not, because there is no documentation. The priority because these variables is not documented at all. For example:

    • What the priority between BASECFLAGS, CFLAGS, OPT and EXTRA_FLAGS?
    • Which flags are used to build Python core?
    • Which flags are used to build modules of the standard library?
    • Which flags are used to build third-party modules?

    Linker flags are not documented neither which caused a regression for a short time (issue ith BLDSHARED if I recall correctly).

    Examples of variables related to linker flags:

    ---
    # LDFLAGS_NODIST is used in the same manner as CFLAGS_NODIST.
    # Use it when a linker flag should _not_ be part of the distutils LDFLAGS
    # once Python is installed (bpo-35257)
    CONFIGURE_LDFLAGS_NODIST=@LDFLAGS_NODIST@
    CONFIGURE_LDFLAGS= @LDFLAGS@

    PY_LDFLAGS= $(CONFIGURE_LDFLAGS) $(LDFLAGS)
    PY_LDFLAGS_NODIST=$(CONFIGURE_LDFLAGS_NODIST) $(LDFLAGS_NODIST)
    PY_CORE_LDFLAGS=$(PY_LDFLAGS) $(PY_LDFLAGS_NODIST)

    LDSHARED= @LDSHARED@ $(PY_LDFLAGS)
    BLDSHARED= @BLDSHARED@ $(PY_CORE_LDFLAGS)
    ---

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Jul 30, 2019

    I agree that documenting the flags is quite important, I've had a hard time trying to figure out how to implement the LDFLAGS_NODIST, and the change still broke macos builds (luckily it was fixed swiftly).

    Nevertheless, this is still a bug which should be addressed.

    On another note, where would be the best place to start documenting those flags? Makefile, the python docs, somewhere else?

    @vstinner
    Copy link
    Member

    IMHO comments in Makefile.pre.in is a good start.

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Oct 2, 2019

    Dug a bit further here.

    The issue is that CFLAGS_NODIST will always come after normal CFLAGS (which are subsets of PY_CFLAGS and PY_CFLAGS_NODIST) [0][1].

    The EXTRA_CFLAGS variable is appended at the end of PY_CFLAGS [2], hence as reported here, whatever compiler flag comes after, embedded within PY_CFLAGS_NODIST, will override the previous flags if they contradict each other.

    So basically EXTRA_CFLAGS can be used only for flags that can't be overwritten by PY_CFLAGS_NODIST, which in my opinion, is not very useful in the context of just adding extra flags.

    Commit adding the variable to Python 2.5: 08cd598

    "EXTRA_CFLAGS has been introduced as an environment variable to hold compiler flags that change binary compatibility"

    Apparently it was added in order to avoid using the OPT variable for the various debug builds described in https://github.com/python/cpython/blob/master/Misc/SpecialBuilds.txt

    On another note this flag will get passed down to distutils, so if it was used for building the interpreter, C extensions compiled by users will also inherit it.

    Honestly I am not sure what the best solution would be here. If the various sub-debug special builds are still relevant and they stack, by doing for example $ make CFLAGS_NODIST="-DPy_TRACE_REFS" EXTRA_CFLAGS="-DPy_REF_DEBUG" then the issue can be closed, and the documentation can be more explicit that the EXTRA_CFLAGS is to be used only for the special builds.

    If the EXTRA_CFLAGS can also be used for adding our own flags, then the flag handling needs to change to take this into account.

    [0] https://github.com/python/cpython/blob/master/setup.py#L85
    [1] https://github.com/python/cpython/blob/master/Makefile.pre.in#L115
    [2] https://github.com/python/cpython/blob/master/Makefile.pre.in#L97

    @stratakis
    Copy link
    Mannequin

    stratakis mannequin commented Oct 2, 2019

    Also this is due to an expected behaviour from gcc. From the documentation:

    "If you use multiple -O options, with or without level numbers, the last such option is the one that is effective. "

    @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 3.9 only security fixes build The build process and cross-build type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    1 participant