Hi Jonathan & Peff, On Wed, 1 May 2019, Jonathan Nieder wrote: > Jeff King wrote: > > > I wonder if this points to this patch touching the wrong level. These > > compiler flags are a thing that _some_ builds want (i.e., production > > builds where people care most about security and not about debugging), > > but not necessarily all. > > > > I'd have expected this to be tweakable by a Makefile knob (either a > > specific knob, or just the caller setting the right CFLAGS etc), and > > then for the builds of Git for Windows to turn those knobs when making a > > package to distribute. > > > > Our internal package builds at GitHub all have this in their config.mak > > (for Linux, of course): > > > > CFLAGS += -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=1 > > CFLAGS += -fstack-protector-strong > > > > CFLAGS += -fpie > > LDFLAGS += -z relro -z now > > LDFLAGS += -pie > > > > and I wouldn't be surprised if other binary distributors (like the > > Debian package) do something similar. > > Yes, the Debian package uses > > CFLAGS := -Wall \ > $(shell dpkg-buildflags --get CFLAGS) \ > $(shell dpkg-buildflags --get CPPFLAGS) > > and then passes CFLAGS='$(CFLAGS)' to "make". > > That means we're using > > -g -O2 -fstack-protector-strong -Wformat -Werror=format-security > -Wdate-time -D_FORTIFY_SOURCE=2 > > Dscho's suggestion for the Windows build sounds fine to me (if > checking for -Og, too). Maybe it would make sense to factor out a > makefile variable for this, that could be used for builds on other > platforms, too. That way, the autodetection can be in one place, and > there is a standard way to override it when the user wants something > else. Indeed, if I was to add a generic "are we building for production?" function, this would be incorrect. But this is not the case here, we are doing something very specific, Windows-only here, and for the sole reason to keep debuggability (for which the presence of the `-g` option indeed would not be a good indicator: in Git for Windows, we build `.pdb` files so that stackdumps can be more meaningful, but we do not want to have full debug information in those executables). In the long run, I think we need to become more explicit about this, by adding a "FOR_PRODUCTION" flag. It's really no good if we use implementation details such as CFLAGS to deduce intent. That's for another patch series, though, as it is pretty clear-cut here: If you build with optimization flags using Git for Windows' SDK, you cannot use gdb for single-stepping, likewise if you use ASLR, so we can totally piggyback the latter onto the former. Ciao, Dscho
