proposal to add -Wheader-guard option

proposal to add -Wheader-guard option

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

Hi, I was wondering if it's a good idea to add -Wheader-guard option
that warns on mismatches between #ifndef and #define lines
in header guard, similar to -Wheader-guard in clang-3.4 ?
(http://llvm.org/releases/3.4/tools/clang/docs/ReleaseNotes.html)

I have implemented patch for -Wheader-guard (please find it attached).
Consider a file having the following format:
#ifndef cmacro (or #if !defined(cmacro) )
#define dmacro
// rest of the stuff
#endif

The warning is triggered if the edit distance
(http://en.wikipedia.org/wiki/Levenshtein_distance), between cmacro
and dmacro
is <= max (len(cmacro), len(dmacro)) / 2
If the edit distance is more than half, I assume that cmacro
and dmacro are "very different", and the intent
was probably not to define header guard (This is what clang does too).

Example:
#ifndef FOO_H
#define _FOO_H
#endif

foo.h:1:0: warning: FOO_H used as header guard followed by #define of
different macro [-Wheader-guard]
 #ifndef FOO_H
 ^
foo.h:2:0: note: FOO_H is defined here, did you mean _FOO_H ?
 #define _FOO_H
 ^

Warning is not triggered in the following cases:

1] The edit distance between #ifndef (or #!defined) macro
and #define macro is > half of maximum length between two macros

Example:
#ifndef FOO
#define BAR
#endif

2] #ifndef and #define are not on consecutive lines (blank lines/ comment lines
are not ignored).

3] dmacro gets undefined
Example:
#ifndef cmacro
#define dmacro
#undef dmacro
#endif

However the following warning gets generated during the build:
../../src/libcpp/directives.c: In function 'void _cpp_pop_buffer(cpp_reader*)':
../../src/libcpp/directives.c:
2720:59: warning: 'inc_type' may be used
uninitialized in this function [-Wmaybe-uninitialized]
       _cpp_pop_file_buffer (pfile, inc, to_free, inc_type);
                                                           ^
_cpp_pop_buffer(): http://pastebin.com/aLYLnXJa
I have defined inc_type only if inc is not null (ie buffer is file
buffer) in 1st if()
and used it (passed it to _cpp_pop_file_buffer() ), only if inc is not null
in 2nd if(). I guess this warning could be considered harmless ?
How should I should rewrite it to avoid the warning ?

Thanks and Regards,
Prathamesh

[-- Attachment #2: header-guard.patch --]
[-- Type: text/x-patch, Size: 11878 bytes --]

Index: gcc/c-family/c-common.c
===================================================================
--- gcc/c-family/c-common.c	(revision 207299)
+++ gcc/c-family/c-common.c	(working copy)
@@ -9558,6 +9558,7 @@ static const struct reason_option_codes_
   {CPP_W_WARNING_DIRECTIVE,		OPT_Wcpp},
   {CPP_W_LITERAL_SUFFIX,		OPT_Wliteral_suffix},
   {CPP_W_DATE_TIME,			OPT_Wdate_time},
+  {CPP_W_HEADER_GUARD,  OPT_Wheader_guard},
   {CPP_W_NONE,				0}
 };
 
Index: gcc/c-family/c-opts.c
===================================================================
--- gcc/c-family/c-opts.c	(revision 207299)
+++ gcc/c-family/c-opts.c	(working copy)
@@ -430,6 +430,10 @@ c_common_handle_option (size_t scode, co
       cpp_opts->cpp_warn_traditional = value;
       break;
 
+    case OPT_Wheader_guard:
+      cpp_opts->cpp_warn_header_guard = value;
+      break;
+
     case OPT_Wtrigraphs:
       cpp_opts->warn_trigraphs = value;
       break;
Index: gcc/c-family/c.opt
===================================================================
--- gcc/c-family/c.opt	(revision 207299)
+++ gcc/c-family/c.opt	(working copy)
@@ -736,6 +736,10 @@ Wtraditional
 C ObjC Var(warn_traditional) Warning
 Warn about features not present in traditional C
 
+Wheader-guard
+C ObjC C++ ObjC++ Warning
+Warn of header guard
+
 Wtraditional-conversion
 C ObjC Var(warn_traditional_conversion) Warning
 Warn of prototypes causing type conversions different from what would happen in the absence of prototype
Index: libcpp/directives.c
===================================================================
--- libcpp/directives.c	(revision 207299)
+++ libcpp/directives.c	(working copy)
@@ -565,6 +565,27 @@ lex_macro_node (cpp_reader *pfile, bool
   return NULL;
 }
 
+// return true if top of if_stack is cmacro
+static bool
+cmacro_ifs_top_p(cpp_reader *pfile)
+{
+  struct if_stack *ifs = pfile->buffer->if_stack;
+  return ifs && (ifs->next == NULL) && (ifs->mi_cmacro != NULL);
+}
+
+static linenum_type
+linediff (struct line_maps *maps, source_location loc1, source_location loc2)
+{
+  linenum_type temp;
+
+  if (loc1 < loc2)
+    temp = loc1, loc1 = loc2, loc2 = temp;
+
+  const struct line_map *m1 = linemap_lookup (maps, loc1);
+  const struct line_map *m2 = linemap_lookup (maps, loc2);
+  return SOURCE_LINE (m1, loc1) - SOURCE_LINE (m2, loc2);
+}
+
 /* Process a #define directive.  Most work is done in macro.c.  */
 static void
 do_define (cpp_reader *pfile)
@@ -586,6 +607,15 @@ do_define (cpp_reader *pfile)
 	  pfile->cb.define (pfile, pfile->directive_line, node);
 
       node->flags &= ~NODE_USED;
+      
+      // possibly #define following #ifndef in the include guard
+      if (pfile->buffer->dmacro == NULL && cmacro_ifs_top_p (pfile)
+          && linediff (pfile->line_table, pfile->directive_line, pfile->buffer->if_stack->line) == 1)
+      {
+        pfile->buffer->dmacro = node;
+        pfile->buffer->cmacro_loc = pfile->buffer->if_stack->line;
+        pfile->buffer->dmacro_loc = pfile->directive_line;
+      }
     }
 }
 
@@ -2527,6 +2557,104 @@ cpp_get_deps (cpp_reader *pfile)
   return pfile->deps;
 }
 
+static inline unsigned
+ed_min3 (unsigned a, unsigned b, unsigned c)
+{
+  return (a < b) ? (a < c) ? a : c
+                 : (b < c) ? b : c;
+}
+
+/*
+ * Levenshtein distance: http://en.wikipedia.org/wiki/Levenshtein_distance
+ * The algorithm is implemented using dynamic programming. 
+ * Instead of the matrix[s_len][t_len], we only need storage
+ * for one row with length = min(s_len, t_len), since we look only at one row and it's previous row
+ * at a time (and previous row's contents are replaced in place).
+ * hstr is string laid along the row of matrix and vstr is laid along column
+ * hstr is string with lesser length between s and t.
+ */
+
+static unsigned
+edit_dist (const unsigned char *s, const unsigned char *t,
+           unsigned s_len, unsigned t_len)
+{
+  unsigned *row; 
+  unsigned n_rows, n_cols, i, j, prev, temp, dist;
+  const unsigned char *hstr, *vstr;
+
+  if (s_len < t_len)
+  {
+    hstr = s; 
+    vstr = t;
+    n_cols = s_len + 1;
+    n_rows = t_len + 1; 
+  }
+  else
+  {
+    hstr = t;
+    vstr = s; 
+    n_cols = t_len + 1;
+    n_rows = s_len + 1;
+  }
+
+  row = (unsigned *) xmalloc (sizeof (*row) * n_cols);
+  for (j = 0; j < n_cols; j++)
+    row[j] = j;
+
+  for (i = 1; i < n_rows; i++)
+  {
+    row[0] = i;
+    for (j = 1, prev = i - 1; j < n_cols; j++)
+    {
+      temp = row[j];
+      row[j] = ed_min3 (row[j - 1] + 1, row[j] + 1, prev + (vstr[i-1] != hstr[j-1]));
+      prev = temp;
+    }
+  }
+
+  dist = row[n_cols - 1];
+  free (row);
+  return dist; 
+}
+
+
+static void
+warn_header_guard (cpp_reader *pfile)
+{
+  const cpp_hashnode *cmacro = pfile->mi_cmacro;
+  const cpp_hashnode *dmacro = pfile->buffer->dmacro;
+
+  /* 
+   * _cpp_file_cmacro_defined_p (), takes care that we are not warning again
+   * for the same file (for example if it's included twice)
+   * if cmacro is defined, or dmacro gets undefined return
+   */
+
+  if (_cpp_file_cmacro_defined_p (pfile->buffer->file) || cmacro == NULL || cmacro->type != NT_VOID
+     || dmacro == NULL || dmacro->type != NT_MACRO)
+    return;
+  
+  const unsigned char *cmacro_name = HT_STR (HT_NODE (&cmacro->ident));
+  unsigned cmacro_len = HT_LEN (HT_NODE (&cmacro->ident));
+
+  const unsigned char *dmacro_name = HT_STR (HT_NODE (&dmacro->ident));
+  unsigned dmacro_len = HT_LEN (HT_NODE (&dmacro->ident));
+
+  unsigned maxhalflen =  (cmacro_len > dmacro_len ? cmacro_len : dmacro_len) >> 1; 
+  unsigned ed = edit_dist (cmacro_name, dmacro_name, cmacro_len, dmacro_len);
+
+  if (ed <= maxhalflen)
+  {
+    cpp_warning_with_line (pfile, CPP_W_HEADER_GUARD, pfile->buffer->cmacro_loc, 0, 
+                           "%s used as header guard followed by #define of different macro",
+                           cmacro_name); 
+
+    cpp_error_with_line (pfile, CPP_DL_NOTE, pfile->buffer->dmacro_loc, 0,
+                         "%s is defined here, did you mean %s ?",
+                         cmacro_name, dmacro_name); 
+  }
+}
+
 /* Push a new buffer on the buffer stack.  Returns the new buffer; it
    doesn't fail.  It does not generate a file change call back; that
    is the responsibility of the caller.  */
@@ -2559,6 +2687,7 @@ _cpp_pop_buffer (cpp_reader *pfile)
   struct _cpp_file *inc = buffer->file;
   struct if_stack *ifs;
   const unsigned char *to_free;
+  enum include_type inc_type;
 
   /* Walk back up the conditional stack till we reach its level at
      entry to this file, issuing error messages.  */
@@ -2566,6 +2695,13 @@ _cpp_pop_buffer (cpp_reader *pfile)
     cpp_error_with_line (pfile, CPP_DL_ERROR, ifs->line, 0,
 			 "unterminated #%s", dtable[ifs->type].name);
 
+  if (inc)
+  {
+    inc_type = buffer->inc_type;
+    if (CPP_WHEADER_GUARD (pfile) && pfile->mi_valid)
+      warn_header_guard (pfile);
+  }
+
   /* In case of a missing #endif.  */
   pfile->state.skipping = 0;
 
@@ -2581,7 +2717,7 @@ _cpp_pop_buffer (cpp_reader *pfile)
 
   if (inc)
     {
-      _cpp_pop_file_buffer (pfile, inc, to_free);
+      _cpp_pop_file_buffer (pfile, inc, to_free, inc_type);
 
       _cpp_do_file_change (pfile, LC_LEAVE, 0, 0, 0);
     }
Index: libcpp/files.c
===================================================================
--- libcpp/files.c	(revision 207299)
+++ libcpp/files.c	(working copy)
@@ -198,6 +198,12 @@ static int pchf_save_compare (const void
 static int pchf_compare (const void *d_p, const void *e_p);
 static bool check_file_against_entries (cpp_reader *, _cpp_file *, bool);
 
+bool
+_cpp_file_cmacro_defined_p (_cpp_file *file)
+{
+  return file->cmacro != NULL;
+}
+
 /* Given a filename in FILE->PATH, with the empty string interpreted
    as <stdin>, open it.
 
@@ -859,6 +865,14 @@ should_stack_file (cpp_reader *pfile, _c
   return f == NULL;
 }
 
+/* Initialize controlling macro state */
+static inline void
+mi_init (cpp_reader *pfile)
+{
+  pfile->mi_valid = true;
+  pfile->mi_cmacro = 0;
+}
+
 /* Place the file referenced by FILE into a new buffer on the buffer
    stack if possible.  IMPORT is true if this stacking attempt is
    because of a #import directive.  Returns true if a buffer is
@@ -895,10 +909,10 @@ _cpp_stack_file (cpp_reader *pfile, _cpp
   buffer->file = file;
   buffer->sysp = sysp;
   buffer->to_free = file->buffer_start;
+  buffer->dmacro = 0;
 
   /* Initialize controlling macro state.  */
-  pfile->mi_valid = true;
-  pfile->mi_cmacro = 0;
+  mi_init (pfile); 
 
   /* Generate the call back.  */
   _cpp_do_file_change (pfile, LC_ENTER, file->path, 1, sysp);
@@ -1012,7 +1026,8 @@ _cpp_stack_include (cpp_reader *pfile, c
     /* _cpp_stack_file didn't stack the file, so let's rollback the
        compensation dance we performed above.  */
     pfile->line_table->highest_location++;
-
+  else
+    pfile->buffer->inc_type = type;
   return stacked;
 }
 
@@ -1445,7 +1460,7 @@ cpp_push_default_include (cpp_reader *pf
    input stack.  */
 void
 _cpp_pop_file_buffer (cpp_reader *pfile, _cpp_file *file,
-		      const unsigned char *to_free)
+		      const unsigned char *to_free, enum include_type inc_type)
 {
   /* Record the inclusion-preventing macro, which could be NULL
      meaning no controlling macro.  */
@@ -1453,6 +1468,10 @@ _cpp_pop_file_buffer (cpp_reader *pfile,
     file->cmacro = pfile->mi_cmacro;
 
   /* Invalidate control macros in the #including file.  */
+
+  if (inc_type == IT_DEFAULT || inc_type == IT_CMDLINE)
+    mi_init (pfile); 
+  else
   pfile->mi_valid = false;
 
   if (to_free)
Index: libcpp/include/cpplib.h
===================================================================
--- libcpp/include/cpplib.h	(revision 207299)
+++ libcpp/include/cpplib.h	(working copy)
@@ -354,6 +354,9 @@ struct cpp_options
      traditional C.  */
   unsigned char cpp_warn_traditional;
 
+  /* Nonzero means warn about header guard */
+  unsigned char cpp_warn_header_guard;
+
   /* Nonzero means warn about long long numeric constants.  */
   unsigned char cpp_warn_long_long;
 
@@ -933,7 +936,8 @@ enum {
   CPP_W_INVALID_PCH,
   CPP_W_WARNING_DIRECTIVE,
   CPP_W_LITERAL_SUFFIX,
-  CPP_W_DATE_TIME
+  CPP_W_DATE_TIME,
+  CPP_W_HEADER_GUARD
 };
 
 /* Output a diagnostic of some kind.  */
Index: libcpp/internal.h
===================================================================
--- libcpp/internal.h	(revision 207299)
+++ libcpp/internal.h	(working copy)
@@ -348,6 +348,11 @@ struct cpp_buffer
      needs to be extern "C" protected in C++, and zero otherwise.  */
   unsigned char sysp;
 
+  enum include_type inc_type;
+  const cpp_hashnode *dmacro;
+  source_location cmacro_loc;
+  source_location dmacro_loc;
+
   /* The directory of the this buffer's file.  Its NAME member is not
      allocated, so we don't need to worry about freeing it.  */
   struct cpp_dir dir;
@@ -597,6 +602,7 @@ cpp_in_system_header (cpp_reader *pfile)
 }
 #define CPP_PEDANTIC(PF) CPP_OPTION (PF, cpp_pedantic)
 #define CPP_WTRADITIONAL(PF) CPP_OPTION (PF, cpp_warn_traditional)
+#define CPP_WHEADER_GUARD(PF) CPP_OPTION (PF, cpp_warn_header_guard)
 
 static inline int cpp_in_primary_file (cpp_reader *);
 static inline int
@@ -640,11 +646,12 @@ extern void _cpp_report_missing_guards (
 extern void _cpp_init_files (cpp_reader *);
 extern void _cpp_cleanup_files (cpp_reader *);
 extern void _cpp_pop_file_buffer (cpp_reader *, struct _cpp_file *,
-				  const unsigned char *);
+				  const unsigned char *, enum include_type);
 extern bool _cpp_save_file_entries (cpp_reader *pfile, FILE *f);
 extern bool _cpp_read_file_entries (cpp_reader *, FILE *);
 extern const char *_cpp_get_file_name (_cpp_file *);
 extern struct stat *_cpp_get_file_stat (_cpp_file *);
+extern bool _cpp_file_cmacro_defined_p (_cpp_file *); 
 
 /* In expr.c */
 extern bool _cpp_parse_expr (cpp_reader *, bool);

Re: proposal to add -Wheader-guard option

[Prathamesh Kulkarni]

Ping ?

On Fri, Jan 31, 2014 at 12:47 AM, Prathamesh Kulkarni
<bilbotheelffriend@gmail.com> wrote:
> Hi, I was wondering if it's a good idea to add -Wheader-guard option
> that warns on mismatches between #ifndef and #define lines
> in header guard, similar to -Wheader-guard in clang-3.4 ?
> (http://llvm.org/releases/3.4/tools/clang/docs/ReleaseNotes.html)
>
> I have implemented patch for -Wheader-guard (please find it attached).
> Consider a file having the following format:
> #ifndef cmacro (or #if !defined(cmacro) )
> #define dmacro
> // rest of the stuff
> #endif
>
> The warning is triggered if the edit distance
> (http://en.wikipedia.org/wiki/Levenshtein_distance), between cmacro
> and dmacro
> is <= max (len(cmacro), len(dmacro)) / 2
> If the edit distance is more than half, I assume that cmacro
> and dmacro are "very different", and the intent
> was probably not to define header guard (This is what clang does too).
>
> Example:
> #ifndef FOO_H
> #define _FOO_H
> #endif
>
> foo.h:1:0: warning: FOO_H used as header guard followed by #define of
> different macro [-Wheader-guard]
>  #ifndef FOO_H
>  ^
> foo.h:2:0: note: FOO_H is defined here, did you mean _FOO_H ?
>  #define _FOO_H
>  ^
>
> Warning is not triggered in the following cases:
>
> 1] The edit distance between #ifndef (or #!defined) macro
> and #define macro is > half of maximum length between two macros
>
> Example:
> #ifndef FOO
> #define BAR
> #endif
>
> 2] #ifndef and #define are not on consecutive lines (blank lines/ comment lines
> are not ignored).
>
> 3] dmacro gets undefined
> Example:
> #ifndef cmacro
> #define dmacro
> #undef dmacro
> #endif
>
> However the following warning gets generated during the build:
> ../../src/libcpp/directives.c: In function 'void _cpp_pop_buffer(cpp_reader*)':
> ../../src/libcpp/directives.c:
> 2720:59: warning: 'inc_type' may be used
> uninitialized in this function [-Wmaybe-uninitialized]
>        _cpp_pop_file_buffer (pfile, inc, to_free, inc_type);
>                                                            ^
> _cpp_pop_buffer(): http://pastebin.com/aLYLnXJa
> I have defined inc_type only if inc is not null (ie buffer is file
> buffer) in 1st if()
> and used it (passed it to _cpp_pop_file_buffer() ), only if inc is not null
> in 2nd if(). I guess this warning could be considered harmless ?
> How should I should rewrite it to avoid the warning ?
>
> Thanks and Regards,
> Prathamesh

Re: proposal to add -Wheader-guard option

[Markus Trippelsdorf]

On 2014.02.04 at 12:36 +0530, Prathamesh Kulkarni wrote:
> Ping ?

Patches should be posted to the gcc-patches@gcc.gnu.org list.
Please follow up there.

-- 
Markus

Re: proposal to add -Wheader-guard option

[Prathamesh Kulkarni]

On Tue, Feb 4, 2014 at 12:45 PM, Markus Trippelsdorf
<markus@trippelsdorf.de> wrote:
> On 2014.02.04 at 12:36 +0530, Prathamesh Kulkarni wrote:
>> Ping ?
>
> Patches should be posted to the gcc-patches@gcc.gnu.org list.
> Please follow up there.
Redirected to gcc-patches@gcc.gnu.org list. I apologize for posting here.
>
> --
> Markus