Hi Jonathan & Peff,

On Wed, 1 May 2019, Jonathan Nieder wrote:

Jeff King wrote:

quoted

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