Mailing-List: contact stdcxx-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: stdcxx-dev@incubator.apache.org
Received-SPF: pass (asf.osuosl.org: local policy)
Message-ID: <452ECA5E.3080706@roguewave.com>
Date: Thu, 12 Oct 2006 17:06:06 -0600
From: Andrew Black <ablack@roguewave.com>
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US;
 rv:1.8.0.7) Gecko/20060910 SeaMonkey/1.0.5
MIME-Version: 1.0
To: stdcxx-dev@incubator.apache.org
Subject: Re: [PATCH] stdcxx target warning counting
References: <452BB8CB.1050201@roguewave.com> <452D302C.2070303@roguewave.com>
 <452D6E71.9020207@roguewave.com> <452E9C4A.4020603@roguewave.com>
In-Reply-To: <452E9C4A.4020603@roguewave.com>
Content-Type: multipart/mixed;
 boundary="------------060700030104040406060103"

--------------060700030104040406060103
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit

Take 3 attached, altered some, slightly altered log at end.

Martin Sebor wrote:
> Andrew Black wrote:
>> Revised patch, attached.  I believe the same change log can be used.
>>
> [...]
>> One version of the patch did do a case insensitive comparison, moving 
>> the rw_strcasecmp (and rw_charcasecmp) functions from exec.cpp to 
>> util.cpp.  The reason I chose to do things this way was so that 
>> different warning parsing modes could be specified for the compile and 
>> link stages.  An example where this could be useful would be when 
>> building with GCC on Solaris, but linking with Sun's linker.
> 
> That's a good point. But I'm afraid I'm not sure I see how
> capitalization can help with which linker is used.

I was using the same capitalization trick that was used for setting 
hard/soft limits.  I've reworked the parse_warn_opts method, simplifying 
the logic and accepting only a single alias code (in lower case).  A 
future patch would likely be to accept case insensitive alias names.

[...]

> 
> [...]
>> This is a math overflow check.  If we assume ~5 assertions take 1 k of 
>> disk space in a result log (each assertion ~200 characters (bytes) 
>> long), the resulting file would be ~800,000,000 kb, or ~800 Gb.  A 
>> file this long is theoretically within the capabilities of a modern 
>> OS, but you are likely to run out of disk space prior to writing the 
>> entire log.
>>
>> This check was added for consistency with the assertion counting code, 
>> and it would be outside the scope of this patch to remove that check 
>> (and the ST_OVERFLOW item).
> 
> Maybe. It doesn't mean that we need to introduce unnecessary logic
> into new code just for consistency with existing code. I don't
> suppose you would introduce a bug into new code just because we
> had the same bug in a similar piece of existing code, would you? :)
> Incidentally, if you insist on handling this exceedingly unlikely
> case, it would be better to indicate the overflow in the column
> that overflowed (since there are more than one) and not in the
> status column.

It makes sense.  In this revised patch, I set the value of t_warn to -1 
(Max unsigned int) if we hit overflow.  It would be a simple patch 
(later) to alter the existing logic to behave the same way, remove the 
ST_OVERFLOW item, and display an overflow message when the count reaches 
this value.

--Andrew Black

Log:
     * makefile.rules (%.o: %(AS_EXT), %.o: %.cpp, %: %.o, %: %.cpp): 
Add $(TEEOPTS) to compile/link line so that output is routed to log files.
     * makefile.common (TEEOPTS): Always tee output to $@.log.
     * target.h (target_opts): Add c_warn and l_warn fields.
       (target_status): Split warn field into c_warn, l_warn, t_warn.
     * output.cpp (check_test): Count warnings, store in status->t_warn.
       (check_compat_test): Expand tail horizon, capture warnings into 
status->t_warn.
     * display.cpp (print_header_plain): Add WARN column, rename WALL 
column to REAL.
       (print_status_plain): Print the total number of warnings in the 
WARN column.
     * runall.cpp (count_warnings): Add static method to count the 
number of warnings in a .log file
       (check_target_ok): Always calculate object file name, call 
count_warnings for object and executable.
     * cmdopt.cpp (usage_text): Document new --warn switch
       (parse_warn_opts): Define new helper function to parse --warn switch.
       (eval_options): Used here in parsing and to initialize default 
c_warn and l_warn values.

--------------060700030104040406060103
Content-Type: text/x-patch;
 name="warnparse3.diff"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="warnparse3.diff"

Index: etc/config/makefile.common
===================================================================
--- etc/config/makefile.common	(revision 454401)
+++ etc/config/makefile.common	(working copy)
@@ -143,8 +143,10 @@
 
 # if LOGFILE is being created, tee command output into it
 # IMPORTANT: $(TEEOPTS) must be last on the command line
-ifneq ($(LOGFILE),/dev/null)
-  TEEOPTS = 2>&1 | tee -a $(LOGFILE)
+ifeq ($(LOGFILE),/dev/null)
+  TEEOPTS = 2>&1 | tee $@.log
+else
+  TEEOPTS = 2>&1 | tee $@.log | tee -a $(LOGFILE)
 endif
 
 # determine the name of the results file against which to compute regressions
Index: etc/config/makefile.rules
===================================================================
--- etc/config/makefile.rules	(revision 454401)
+++ etc/config/makefile.rules	(working copy)
@@ -60,16 +60,16 @@
     ifneq ($(AS_EXT),".")
 
 %.o: %$(AS_EXT)
-	$(CXX) -c $(CPPFLAGS) $(CXXFLAGS) $<
+	$(CXX) -c $(CPPFLAGS) $(CXXFLAGS) $< $(TEEOPTS)
 
     endif   # ifneq ($(AS_EXT),".")
   endif   # ifneq ($(AS_EXT),)
 
 %.o: %.cpp
-	$(CXX) -c $(CPPFLAGS) $(CXXFLAGS) $<
+	$(CXX) -c $(CPPFLAGS) $(CXXFLAGS) $< $(TEEOPTS)
 
 %: %.o
-	$(LD) $< -o $@ $(LDFLAGS) $(LDLIBS)
+	$(LD) $< -o $@ $(LDFLAGS) $(LDLIBS) $(TEEOPTS)
 
 # disable compilation and linking in the same step
 # %: %.cpp
@@ -78,7 +78,7 @@
 
 # compile and link in one step to eliminate the space overhead of .o files
 %: %.cpp
-	$(CXX) $< -o $@ $(CPPFLAGS) $(CXXFLAGS) $(LDFLAGS) $(LDLIBS)
+	$(CXX) $< -o $@ $(CPPFLAGS) $(CXXFLAGS) $(LDFLAGS) $(LDLIBS) $(TEEOPTS)
 
 endif   # eq ($(NO_DOT_O),)
 
Index: util/cmdopt.cpp
===================================================================
--- util/cmdopt.cpp	(revision 454401)
+++ util/cmdopt.cpp	(working copy)
@@ -89,6 +89,7 @@
     "    --signal=sig Send itself the specified signal.\n"
     "    --ignore=sig Ignore the specified signal.\n"
     "    --ulimit=lim Set child process usage limits (see below).\n"
+    "    --warn=alias Set compiler log warning pattern (see below).\n"
     "\n"
     "  All short (single dash) options must be specified seperately.\n"
     "  If a short option takes a value, it may either be provided like\n"
@@ -121,6 +122,22 @@
     "  Note: Some operating systems lack support for some or all of the\n"
     "  ulimit modes.  If a system is unable to limit a given property, a\n"
     "  warning message will be produced.\n"
+    "\n"
+    "  --warn set the string used to parse compile and link logs.  Rather\n"
+    "  than specifying a search string, an alias code is provided,\n"
+    "  coresponding to the output of a compiler and linker.  Alias codes\n"
+    "  are case sensitive.\n"
+    "\n"
+    "  --warn modes:\n"
+    "    acc     HP aCC\n"
+    "    cxx     Compaq C++\n"
+    "    eccp    EDG eccp\n"
+    "    gcc     GNU gcc\n"
+    "    icc     Intel icc for Linux\n"
+    "    mipspro SGI MIPSpro\n"
+    "    sunpro  Sun C++\n"
+    "    vacpp   IBM VisualAge C++\n"
+    "    xlc     IBM XLC++\n"
 };
 
 #if !defined (_WIN32) && !defined (_WIN64)
@@ -365,7 +382,52 @@
     return 0;
 }
 
+/**
+   Helper function to parse a warning value string
 
+   @param opts ulimit value string to pares
+   @see child_limits
+*/
+static bool
+parse_warn_opts (const char* arg, struct target_opts* defaults)
+{
+    static const struct {
+        const char* name;
+        const char* pat;
+    } warn_set [] = {
+        { "acc", "Warning " },
+/*
+        { "cds", "UNKNOWN"},
+        { "como", "UNKNOWN"},
+*/
+        { "cxx", "Warning:"},
+        { "eccp", "warning:"},
+        { "gcc", "warning:"},
+        { "icc", "warning #"},
+        { "mipspro", "CC: WARNING"},
+        { "sunpro", "Warning:"},
+        { "vacpp", ": (W) "},
+        { "xlc", ": (W) "}, /* xlc and vacpp are synonyms. */
+        { 0, 0 }
+    };
+
+    assert (0 != arg);
+    assert (0 != defaults);
+
+    for (size_t i = 0; warn_set [i].name; ++i) {
+        if (0 == strcmp (warn_set [i].name, arg)) {
+
+            /* Set both compiler and linker warning string. */
+            defaults->c_warn = warn_set [i].pat;
+            defaults->l_warn = warn_set [i].pat;
+
+            return 0;
+        }
+    }
+
+    return 1;
+}
+
 /**
     Helper function to produce 'Bad argument' error message.
     
@@ -423,11 +485,41 @@
     const char opt_signal[]   = "--signal";
     const char opt_sleep[]    = "--sleep";
     const char opt_ulimit[]   = "--ulimit";
+    const char opt_warn[]     = "--warn";
 
     int i;
 
     assert (0 != argv);
 
+    /* The chain of preprocesor logic below initializes the defaults->c_warn 
+       and defaults->l_warn values.
+    */
+#ifdef __GNUG__
+    parse_warn_opts ("Gcc", defaults);
+#elif defined (__HP_aCC)
+    parse_warn_opts ("Acc", defaults);
+#elif defined (__IBMCPP__)
+    parse_warn_opts ("Xlc", defaults);
+#elif defined (__SUNPRO_CC)
+    parse_warn_opts ("Sunpro", defaults);
+#elif defined (SNI)
+    parse_warn_opts ("Cds", defaults);
+#elif defined (__APOGEE__) /* EDG variant that doesn't define __EDG__. */
+    parse_warn_opts ("Como", defaults);
+
+/* The following are EDG variants, that define __EDG__ */
+#elif defined (__DECCXX)
+    parse_warn_opts ("Cxx", defaults);
+#elif defined (_SGI_COMPILER_VERSION)
+    parse_warn_opts ("Mipspro", defaults);
+#elif defined (__INTEL_COMPILER)
+    parse_warn_opts ("Icc", defaults);
+
+/* So we need to check for __EDG__ after we check for them. */
+#elif defined (__EDG__)
+    parse_warn_opts ("Eccp", defaults);
+#endif
+
     if (1 == argc || '-' != argv [1][0])
         return 1;
 
@@ -563,6 +655,16 @@
                     }
                 }
             }
+            else if (   sizeof opt_warn - 1 <= arglen
+                     && !memcmp (opt_warn, argv [i], sizeof opt_warn - 1)) {
+                optname = opt_warn;
+                optarg  = get_long_val (argv, &i, sizeof opt_warn - 1);
+                if (optarg && *optarg) {
+                    if (!parse_warn_opts (optarg, defaults)) {
+                        break;
+                    }
+                }
+            }
             /* fall through */
         }
         default:
Index: util/output.cpp
===================================================================
--- util/output.cpp	(revision 454401)
+++ util/output.cpp	(working copy)
@@ -109,7 +109,13 @@
                     return;
                 }
             }
-            /* else if (1 < r_lvl) warning*/
+            else if (1 < r_lvl) {
+                status->t_warn += r_active;
+                if (status->t_warn < r_active) {
+                    status->t_warn = (unsigned int)-1;
+                    return;
+                }
+            }
             fmt_ok = 1;
         }
     }
@@ -138,7 +144,7 @@
     assert (0 != data);
     assert (0 != status);
 
-    fseek (data, -64, SEEK_END); /* Seek near the end of the file */
+    fseek (data, -80, SEEK_END); /* Seek near the end of the file */
 
     for (tok = fgetc (data); fsm < 4 && !feof (data); tok = fgetc (data)) {
         switch (tok) {
@@ -160,13 +166,13 @@
             fsm = 0;
         }
     }
-
-    if (!feof (data)) { /* leading "## A" eaten above */
-        read = fscanf (data, "ssertions = %u\n## FailedAssertions = %u",
-                        &status->assert, &status->failed);
+    if (!feof (data)) { /* leading "## W" eaten above */
+        read = fscanf (data, "arnings = %u\n## Assertions = %u\n"
+                       "## FailedAssertions = %u",
+                       &status->t_warn, &status->assert, &status->failed);
     }
 
-    if (2 != read) {
+    if (3 != read) {
         status->status = ST_FORMAT;
     }
 }
Index: util/display.cpp
===================================================================
--- util/display.cpp	(revision 454401)
+++ util/display.cpp	(working copy)
@@ -40,8 +40,8 @@
 */
 static void print_header_plain ()
 {
-    puts ("NAME                      STATUS ASSERTS FAILED PERCNT    USER     "
-          "SYS    WALL");
+    puts ("NAME                      STATUS WARN ASSERTS FAILED PERCNT    "
+          "USER     SYS    REAL");
 }
 
 /**
@@ -76,6 +76,8 @@
     else 
         printf ("     0");
 
+    printf (" %4u", status->c_warn + status->l_warn + status->t_warn);
+
     /* Print assetions, if any registered */
     if (status->assert) {
         printf (" %7u %6u %5d%%", status->assert, status->failed, 
Index: util/target.h
===================================================================
--- util/target.h	(revision 454401)
+++ util/target.h	(working copy)
@@ -98,6 +98,8 @@
     char** argv; /**< Target argv array. */
     unsigned timeout; /**< Allowed time for process execution. */
     const char* data_dir; /**< Root dir for reference input/output files. */
+    const char* c_warn; /**< Warning flag string when compiling. */
+    const char* l_warn; /**< Warning flag string when linking. */
     int compat; /**< Test suite compatability mode switch. */
     rw_rlimit* core;
     rw_rlimit* cpu;
@@ -141,7 +143,9 @@
     const rw_timeval* user; /**< Elapsed user time spent in execution. */
     const rw_timeval* sys; /**< Elapsed system time spent in execution. */
     const rw_timeval* wall; /**< Wall clock time spent in execution. */
-    unsigned warn; /**< Number of (test) warnings. */
+    unsigned c_warn; /**< Number of compile warnings. */
+    unsigned l_warn; /**< Number of link warnings. */
+    unsigned t_warn; /**< Number of (test) warnings. */
     unsigned assert; /**< Number of (test) assertions. */
     unsigned failed; /**< Number of failed (test) assertions. */
 };
Index: util/runall.cpp
===================================================================
--- util/runall.cpp	(revision 454401)
+++ util/runall.cpp	(working copy)
@@ -28,6 +28,7 @@
 #include <errno.h>      /* for errno */
 #include <stdlib.h>     /* for exit(), free() */
 #include <string.h>     /* for memcpy(), ... */
+#include <stdio.h>      /* for FILE, fopen(), ... */
 
 #include <ctype.h>      /* for isspace */
 #include <sys/types.h>
@@ -193,11 +194,82 @@
 }
 
 /**
+   Arbitrary constant controling static read buffer size.
+
+   @see count_warnings
+*/
+#define READ_BUF_LEN 64
+
+/**
+   Compiler/linker output parser.
+
+   This method tries to open the compiler/linker log file, based on the name
+   of a the target file that was generated by the compiler/linker.
+   Upon opening the file, it tries to count the number of warnings present.
+
+   @param target name of target to parse the log for
+   @param counter pointer to the counter to count warnings in.
+   @param match string to match when detecting a warning.
+*/
+static
+void count_warnings (const char* const target, unsigned* counter, 
+                     const char* const match)
+{
+    size_t path_len;
+    char* tmp_name;
+    FILE* data;
+
+    assert (0 != target);
+    assert (0 != counter);
+
+    if (0 == match)
+        return; /* Don't have a match string, so don't tally warnings */
+
+    path_len = strlen (target);
+    tmp_name = (char*)RW_MALLOC (path_len + 5);
+    memcpy (tmp_name, target, path_len + 1);
+    strcat (tmp_name,".log");
+
+    data = fopen (tmp_name, "r");
+    if (data) {
+        size_t pos = 0, limit = strlen (match);
+        while (!feof (data)) {
+            char buf [READ_BUF_LEN];
+            size_t nread;   /* bytes read */
+
+            /* First, read a block from the file into the buffer */
+            nread = fread (buf, 1, sizeof buf, data);
+            if (ferror (data)) {
+                warn ("Error reading %s: %s\n", tmp_name, strerror (errno));
+                break;
+            }
+
+            /* Then, look for the search string within it. */
+            for (size_t i = 0; i < nread; ++i) {
+                if (buf [i] != match [pos])
+                    pos = 0;
+                else if (++pos == limit) {
+                    pos = 0;
+                    ++*counter;
+                }
+            }
+        }
+
+        fclose (data);
+    }
+    else if (ENOENT != errno)
+        warn ("Error opening %s: %s\n", tmp_name, strerror (errno));
+
+    free (tmp_name);
+}
+
+/**
    Preflight check to ensure that target is something that should be run.
 
    This method checks to see if target exists and is theoretically executable.
    If a problem is detected, the condition is recorded in the status structure
-   and 0 is returned.
+   and 0 is returned.  This method also attempts to determine the number of 
+   compile and link warnings that occurred, using count_warnings.
 
    A special case is the situation where the target name ends in .o, 
    indicating that the target only needed to compile, and doesn't need to
@@ -212,31 +284,63 @@
 {
     struct stat file_info;
     int exists = 1;
+    size_t path_len;
+    char* tmp_name;
 
     assert (0 != target);
     assert (0 != status);
 
+    path_len = strlen (target);
+
+#ifndef _WIN32
+    /* Otherwise, check for the .o file on non-windows systems */
+    tmp_name = (char*)RW_MALLOC (path_len + 3);
+    memcpy (tmp_name, target, path_len + 1);
+    strcat (tmp_name,".o");
+#else
+    /* Or the target\target.obj file on windows systems*/
+    {
+        const char* const target_name = get_target ();
+        size_t target_len = strlen (target_name);
+        size_t tmp_len = path_len + target_len - 2;
+        /* - 2 comes from removing 4 characters (extra .exe) and adding 2 
+           characters (\ directory seperator and trailing null) */
+        tmp_name = (char*)RW_MALLOC (tmp_len);
+        memcpy (tmp_name, target, path_len - 4);
+        tmp_name [path_len - 4] = default_path_sep;
+        memcpy (tmp_name + path_len - 3, target_name, target_len);
+        tmp_name [tmp_len - 4] = 'o';
+        tmp_name [tmp_len - 3] = 'b';
+        tmp_name [tmp_len - 2] = 'j';
+        tmp_name [tmp_len - 2] = '\0';
+    }
+#endif   /* _WIN32 */
+
+    count_warnings (target, &status->l_warn, "warning:");
+    count_warnings (tmp_name, &status->c_warn, "warning:");
+
     if (0 > stat (target, &file_info)) {
         if (ENOENT != errno) {
             warn ("Error stating %s: %s\n", target, strerror (errno));
             status->status = ST_SYSTEM_ERROR;
+            free (tmp_name);
             return 0;
         }
         file_info.st_mode = 0; /* force mode on non-existant file to 0 */
         exists = 0; /* note that it doesn't exist */
     }
+
     if (0 == (file_info.st_mode & (S_IXUSR | S_IXGRP | S_IXOTH))) {
         /* This is roughly equivlent to the -x bash operator.
            It checks if the file can be run, /not/ if we can run it
         */
-        const size_t path_len = strlen (target);
-        char* tmp_name;
 
 #if 0 /* Disable .o target check as unused */
         /* If target is a .o file, check if it exists */
         if ('.' == target [path_len-1] && 'o' == target [path_len]) {
             if (!exists)
                 status->status = ST_COMPILE;
+            free (tmp_name);
             return 0;
         }
 #endif
@@ -244,34 +348,10 @@
         /* If the target exists, it doesn't have valid permissions */
         if (exists) {
             status->status = ST_EXECUTE_FLAG;
+            free (tmp_name);
             return 0;
         }
 
-#if !defined (_WIN32) && !defined (_WIN64)
-        /* Otherwise, check for the .o file on non-windows systems */
-        tmp_name = (char*)RW_MALLOC (path_len + 3);
-        memcpy (tmp_name, target, path_len + 1);
-        strcat (tmp_name,".o");
-#else
-        /* Or the target\target.obj file on windows systems*/
-        {
-            const char* const target_name = get_target ();
-            size_t target_len = strlen (target_name);
-            size_t tmp_len = path_len + target_len - 2;
-                /* - 2 comes from removing 4 characters (extra .exe) and 
-                   adding 2 characters (\ directory seperator and trailing 
-                   null) */
-            tmp_name = (char*)RW_MALLOC (tmp_len);
-            memcpy (tmp_name, target, path_len - 4);
-            tmp_name [path_len - 4] = default_path_sep;
-            memcpy (tmp_name + path_len - 3, target_name, target_len);
-            tmp_name [tmp_len - 4] = 'o';
-            tmp_name [tmp_len - 3] = 'b';
-            tmp_name [tmp_len - 2] = 'j';
-            tmp_name [tmp_len - 2] = '\0';
-        }
-#endif   /* _WIN{32,64} */
-
         if (0 > stat (tmp_name, &file_info)) {
             if (ENOENT != errno) {
                 warn ("Error stating %s: %s\n", tmp_name, strerror (errno));
@@ -287,6 +367,7 @@
         free (tmp_name);
         return 0;
     }
+    free (tmp_name);
     return 1;
 }
 

--------------060700030104040406060103--