1. 24 Feb, 2019 1 commit
    • Ævar Arnfjörð Bjarmason's avatar
      Makefile: allow for combining DEVELOPER=1 and CFLAGS="..." · 6d5d4b4e
      Ævar Arnfjörð Bjarmason authored
      Ever since the DEVELOPER=1 facility introduced there's been no way to
      have custom CFLAGS (e.g. CFLAGS="-O0 -g -ggdb3") while still
      benefiting from the set of warnings and assertions DEVELOPER=1
      enables.
      
      This is because the semantics of variables in the Makefile are such
      that the user setting CFLAGS overrides anything we set, including what
      we're doing in config.mak.dev[1].
      
      So let's introduce a "DEVELOPER_CFLAGS" variable in config.mak.dev and
      add it to ALL_CFLAGS. Before this the ALL_CFLAGS variable
      would (basically, there's some nuance we won't go into) be set to:
      
          $(CPPFLAGS) [$(CFLAGS) *or* $(CFLAGS) in config.mak.dev] $(BASIC_CFLAGS) $(EXTRA_CPPFLAGS)
      
      But will now be:
      
          $(DEVELOPER_CFLAGS) $(CPPFLAGS) $(CFLAGS) $(BASIC_CFLAGS) $(EXTRA_CPPFLAGS)
      
      The reason for putting DEVELOPER_CFLAGS first is to allow for
      selectively overriding something DEVELOPER=1 brings in. On both GCC
      and Clang later settings override earlier ones. E.g. "-Wextra
      -Wno-extra" will enable no "extra" warnings, but not if those two
      arguments are reversed.
      
      Examples of things that weren't possible before, but are now:
      
          # Use -O0 instead of -O2 for less painful debuggng
          DEVELOPER=1 CFLAGS="-O0 -g"
          # DEVELOPER=1 plus -Wextra, but disable some of the warnings
          DEVELOPER=1 DEVOPTS="no-error extra-all" CFLAGS="-O0 -g -Wno-unused-parameter"
      
      The reason for the patches leading up to this one re-arranged the
      various *FLAGS assignments and includes is just for readability. The
      Makefile supports assignments out of order, e.g.:
      
          $ cat Makefile
          X = $(A) $(B) $(C)
          A = A
          B = B
          include c.mak
          all:
                  @echo $(X)
          $ cat c.mak
          C=C
          $ make
          A B C
      
      So we could have gotten away with the much smaller change of changing
      "CFLAGS" in config.mak.dev to "DEVELOPER_CFLAGS" and adding that to
      ALL_CFLAGS earlier in the Makefile "before" the config.mak.*
      includes. But I think it's more readable to use variables "after"
      they're included.
      
      1. https://www.gnu.org/software/make/manual/html_node/Overriding.htmlSigned-off-by: Ævar Arnfjörð Bjarmason's avatarÆvar Arnfjörð Bjarmason <avarab@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      6d5d4b4e
  2. 07 Jan, 2019 1 commit
    • Thomas Gummerer's avatar
      config.mak.dev: add -Wall, primarily for -Wformat, to help autoconf users · 6163f3f1
      Thomas Gummerer authored
      801fa63a ("config.mak.dev: add -Wformat-security", 2018-09-08)
      added the "-Wformat-security" to the flags set in config.mak.dev.
      In the gcc man page this is documented as:
      
               If -Wformat is specified, also warn about uses of format
               functions that represent possible security problems.  [...]
      
      The commit did however not add the "-Wformat" flag, but instead
      relied on the fact that "-Wall" is set in the Makefile by default
      and that "-Wformat" is part of "-Wall".
      
      Unfortunately, those who use config.mak.autogen generated with the
      autoconf to configure toolchain do *not* get "-Wall" in their CFLAGS
      and the added -Wformat-security had no effect.  Worse yet, newer
      versions of gcc (gcc 8.2.1 in this particular case) warn about the
      lack of "-Wformat" and thus compilation fails only with this option
      set.
      
      We could fix it by adding "-Wformat", but in general we do want all
      checks included in "-Wall", so let's add it to config.mak.dev to
      cover more cases.
      Signed-off-by: default avatarThomas Gummerer <t.gummerer@gmail.com>
      Helped-by: default avatarJeff King <peff@peff.net>
      Helped-by: default avatarJonathan Nieder <jrnieder@gmail.com>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      6163f3f1
  3. 19 Oct, 2018 1 commit
    • Jeff King's avatar
      config.mak.dev: enable -Wunused-function · 36da8931
      Jeff King authored
      We explicitly omitted -Wunused-function when we added
      -Wextra, but there is no need: the current code compiles
      cleanly with it. And it's worth having, since it can let you
      know when there are cascading effects from a cleanup (e.g.,
      deleting one function lets you delete its static helpers).
      
      There are cases where we may need an unused function to
      exist, but we can handle these easily:
      
        - macro-generated code like commit-slab; there we have the
          MAYBE_UNUSED annotation to silence the compiler
      
        - conditional compilation, where we may or may not need a
          static helper. These generally fall into one of two
          categories:
      
            - the call should not be conditional, but rather the
      	function body itself should be (and may just be a
      	no-op on one side of the #if). That keeps the
      	conditional pollution out of the main code.
      
            - call-chains of static helpers should all be in the
              same #if block, so they are all-or-nothing
      
          And if there's some case that doesn't cover, we still
          have MAYBE_UNUSED as a fallback.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      36da8931
  4. 11 Sep, 2018 1 commit
    • Jeff King's avatar
      config.mak.dev: add -Wformat-security · 801fa63a
      Jeff King authored
      We currently build cleanly with -Wformat-security, and it's
      a good idea to make sure we continue to do so (since calls
      that trigger the warning may be security vulnerabilities).
      
      Note that we cannot use the stronger -Wformat-nonliteral, as
      there are case where we are clever with passing around
      pointers to string literals. E.g., bisect_rev_setup() takes
      bad_format and good_format parameters. These ultimately come
      from literals, but they still trigger the warning.
      
      Some of these might be fixable (e.g., by passing flags from
      which we locally select a format), and might even be worth
      fixing (not because of security, but just because it's an
      easy mistake to pass the wrong format). But there are other
      cases which are likely quite hard to fix (we actually
      generate formats in a local buffer in some cases). So let's
      punt on that for now and start with -Wformat-security, which
      is supposed to catch the most important cases.
      Signed-off-by: default avatarJeff King <peff@peff.net>
      Signed-off-by: default avatarJunio C Hamano <gitster@pobox.com>
      801fa63a
  5. 25 Jul, 2018 1 commit
  6. 16 Apr, 2018 3 commits