SCons / scons

SCons - a software construction tool
http://scons.org
MIT License
2.09k stars 315 forks source link

Deprecated option for precompiled headers in Microsoft compilers #2463

Open bdbaddog opened 6 years ago

bdbaddog commented 6 years ago

This issue was originally created at: 2009-08-06 02:14:48. This issue was reported by: pommac2.

pommac2 said at 2009-08-06 02:14:48

The switch /Yd (used to put debugging information for the precompiled headers in every object file) was deprecated starting with Microsoft Visual C++ 2005 (according to https://msdn.microsoft.com/en-us/library/6bef4950(VS.80).aspx ). Update: dead link, but see https://learn.microsoft.com/en-us/cpp/build/reference/yd-place-debug-information-in-object-file?view=msvc-170

In Tools/msvc.py, PCHPDBFLAGS sets the /Yd option.

I think this should be removed completely for the later versions of the Microsoft compiler, as either /Zi or /Z7 will both have the desired effect.

/Zi causes debug information to be written to a program database (.pdb), and /Z7 causes it to be embedded in the .obj file (suitable for libraries).

gregnoel said at 2009-11-10 18:46:24

Bug party triage.

garyo said at 2012-09-01 10:01:49

Bumping all old issues targeted for past releases to 2.x.

mwichmann commented 3 years ago

It appears SCons knows about this deprecation and avoids using /Yd if it knows via MSVC_VERSION that it's a later compiler, but doesn't try the alternative suggested here, just leaves it blank:

    if env.get('MSVC_VERSION',False):
        maj, min = msvc_version_to_maj_min(env['MSVC_VERSION'])
        if maj < 8:
            env['PCHPDBFLAGS'] = SCons.Util.CLVar(['${(PDB and "/Yd") or ""}'])
        else:
            env['PCHPDBFLAGS'] = ''
    else:
        # Default if we can't determine which version of MSVC we're using
        env['PCHPDBFLAGS'] = SCons.Util.CLVar(['${(PDB and "/Yd") or ""}'])
bdbaddog commented 3 years ago

@dmoody256 @acmorrow - Any thoughts?

dmoody256 commented 3 years ago

Well if the user used Zi or Z7 somewhere else I think it still ends up in the command line for PCH:

env['PCHCOM'] = '$CXX /Fo${TARGETS[1]} $CXXFLAGS $CCFLAGS $CPPFLAGS $_CPPDEFFLAGS $_CPPINCFLAGS /c $SOURCES /Yc$PCHSTOP /Fp${TARGETS[0]} $CCPDBFLAGS $PCHPDBFLAGS'

If PDB is used, it will automatically add Z7 to the CCPDBFLAGS, and the Zi would be in the CXXFLAGS I suppose set from the user? I think it's working as intended.

Did the original bug report actually have an issue? or were they just reading the code and thought there was an improvement that could be made?

mwichmann commented 3 years ago

I think they had a problem, but I'm trying to sort if the problem is actually solved by a code change which never got the corresponding bug closed (so - just close), or whether something should be added in for the version >=8 case.

mwichmann commented 11 months ago

just ran across this again... @jcbrill any thoughts? (don't spend a lot of time on it, just curious)

jcbrill commented 11 months ago

How did it come up?

jcbrill commented 11 months ago

Do you happen to have the warning message issued (if any)?

Possibly off-topic:

Compiler option consistency This table lists compiler options that might trigger an inconsistency warning when using a precompiled header: ... /Zi Generate complete debugging information If this option is in effect when the precompiled header is created, subsequent compilations that use the precompilation can use that debugging information. If /Zi isn't in effect when the precompiled header is created, subsequent compilations that use the precompilation and the /Zi option trigger a warning. The debugging information is placed in the current object file, and local symbols defined in the precompiled header aren't available to the debugger.

mwichmann commented 11 months ago

Do you happen to have the warning message issued (if any)?

This predates me, I don't have anything but what appears here, although I apparently took a look briefly 3+ years ago and left the comment that appears earlier. I was searching for something different when the search hit on this one and I took a quick glance.

jcbrill commented 11 months ago

This issue is nearly 6 years old, was reported 14 years ago in version 1.1.0, and there are no apparent linked issues (i.e., no recent reports of the same behavior): seems reasonable to close.

mwichmann commented 11 months ago

If the current implementation Does The Right Thing, I'm more than happy to close.