[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

Paul D. Smith
URL:
  <http://savannah.gnu.org/bugs/?48643>

                 Summary: Irrelevant targets can confuse make on which pattern
rule to select.
                 Project: make
            Submitted by: None
            Submitted on: Wed 27 Jul 2016 05:02:12 AM UTC
                Severity: 3 - Normal
              Item Group: Bug
                  Status: None
                 Privacy: Public
             Assigned to: None
             Open/Closed: Open
         Discussion Lock: Any
       Component Version: 4.2.1
        Operating System: POSIX-Based
           Fixed Release: None
           Triage Status: None

    _______________________________________________________

Details:

This is a based on
https://lists.gnu.org/archive/html/help-make/2016-01/msg00011.html which I
couldn't find an actual bug report for.

I'm not 100% certain that this is a bug; it's possible that we're missing
something subtle in the manual, but I really do think it's a bug.

I've attached a test case.  The bad behavior is triggered by the last line in
the Makefile; commenting it out causes things to work correctly.

The correct graph for make to choose is


default -> a.foo (%.foo, line 3) -> a.correct -> a.correct_src


But, if we append this line, then make makes the wrong decision:


misleading_target: a.mislead


The line should be irrelevant, because misleading_target isn't something we're
trying to build.  But, it causes make to instead choose this graph:


default -> a.foo (%.foo, line 6) -> a.mislead -> a.mislead_src


That is, it chose the wrong pattern rule for %.foo .



    _______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Wed 27 Jul 2016 05:02:12 AM UTC  Name: Makefile  Size: 543B   By: None

<http://savannah.gnu.org/bugs/download.php?file_id=38033>

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?48643>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/


_______________________________________________
Bug-make mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-make
Reply | Threaded
Open this post in threaded view
|

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

Paul D. Smith
Follow-up Comment #1, bug #48643 (project make):

According to what I wrote in
<https://lists.gnu.org/archive/html/help-make/2016-06/msg00003.html>, this
behaviour is present in 3.82, 4.1 and 4.2.1, but not 3.81.

    _______________________________________________________

Reply to this item at:

  <http://savannah.gnu.org/bugs/?48643>

_______________________________________________
  Message sent via/by Savannah
  http://savannah.gnu.org/


_______________________________________________
Bug-make mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-make
Reply | Threaded
Open this post in threaded view
|

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

Paul D. Smith
Follow-up Comment #2, bug #48643 (project make):

I think the problem might be in src/implicit.c.

From 3.81 code:


              /* The DEP->changed flag says that this dependency resides in a
                 nonexistent directory.  So we normally can skip looking for
                 the file.  However, if CHECK_LASTSLASH is set, then the
                 dependency file we are actually looking for is in a
different
                 directory (the one gotten by prepending FILENAME's
directory),
                 so it might actually exist.  */

              /* @@ dep->changed check is disabled. */
              if (((f = lookup_file (name)) != 0 && f->is_target)
                  /*|| ((!dep->changed || check_lastslash) && */
                  || file_exists_p (name))
                continue;


The corresponding check in 4.3:


                  if (lookup_file (d->name) != 0
                      /*|| ((!dep->changed || check_lastslash) && */
                      || file_exists_p (d->name))


There's no check for f->is_target on the result of lookup_file, so I presume
it is picking up the name as a dependency.  Perhaps it should read:


                  if (((f = lookup_file (d->name)) != 0 && f->is_target)
                      /*|| ((!dep->changed || check_lastslash) && */
                      || file_exists_p (d->name))


f is a [struct file *], not declared in 4.3.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?48643>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

Paul D. Smith
Follow-up Comment #3, bug #48643 (project make):

With the is_target test restored in implicit.c, "make check" fails:


features/patternrules ................................... Error running
/home/simpsons/Works/make/tests/../make (expected 0; got 512):
'/home/simpsons/Works/make/tests/../make' '-f'
'work/features/patternrules.mk.6'
FAILED (11/12 passed)


Seems to be in conflict with bug #17752 - the is_target test was removed to
resolve it, I gather.

I adapted a makefile from features/patternrules so that the intermediate can
have a specific suffix:


STEM = xyz
BIN = $(STEM)$(SFX)
COPY = $(STEM).cp
SRC = $(STEM).c
allbroken: $(COPY) $(BIN) ; @echo ok
$(SRC): ; @echo 'main(){}'
%.cp: %$(SFX) ; @echo $@ from $<
%$(SFX) : %.c ; @echo $@ from $<


Used with no builtin rules, if $(SFX) is non-empty, this works in 3.80, 3.81
and 4.3.  If $(SFX) is empty, only 3.81 fails.

So a %.sfx:%.c rule works, but %:%.c doesn't, because the target lacks a
suffix.  It seems this test eliminates the rule from the candidates:


          /* Rules that can match any filename and are not terminal
             are ignored if we're recursing, so that they cannot be
             intermediate files.  */
          if (recursions > 0 && target[1] == '\0' && !rule->terminal)
            continue;


This suggests that the test case in bug #17752 is a 'feature', as it tries to
use a rule "that can match any filename" to build an intermediate,
specifically, a builtin rule %:%.c.  The in_use flag prevents these repeating,
as in foo.c.c.c, but with umpteen builtin rules of this sort, every
arrangement of every subset of suffixes in these rules is tried, and that
takes a long time, which can result in some checks (variables/EXTRA_PREREQS)
being aborted.

I managed to introduce a new parameter to pattern_search, unsigned anydepth,
which is incremented on the recursive call only if such a rule is used.  The
test above then becomes:


          char anyrule = 0;
          if (target[1] == '\0' && !rule->terminal)
            {
              if (anydepth > 3)
                continue;
              anyrule = 1;
            }


anyrule is then stored in the tryrules entry, so it can be tested on
recursion:


                      if (pattern_search (int_file,
                                          0,
                                          depth + 1,
                                          recursions + 1,
                                          anydepth + !!tryrules[ri].anyrule))


This allows %:%.X rules to be applied at any depth, but limits the total
number on the stack.

For me, "make check" passes with the anydepth limit set to 0, 1, 2 or 3, but 4
took too long on the variables/EXTRA_PREREQS tests.

    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?48643>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

Paul D. Smith
Follow-up Comment #4, bug #48643 (project make):

Here's a patch that restores the is_target test, and counts the %: rules on
the stack, permitting at most one.  This should fix this bug, without bug
#17752 returning, as features/patternrules test 6 still passes, though perhaps
its explanation should be changed:


# TEST 6: Make sure that non-target files are still eligible to be created
# as part of implicit rule chaining.  Savannah bug #17752.


The problem was not that the intermediate file was not a target, but that it
matched a pattern with no suffix.


diff --git a/src/implicit.c b/src/implicit.c
index b281a17..4223d0f 100644
--- a/src/implicit.c
+++ b/src/implicit.c
@@ -24,7 +24,8 @@ this program.  If not, see <http://www.gnu.org/licenses/>.
*/
 #include "commands.h" /* set_file_variables */
 
 static int pattern_search (struct file *file, int archive,
-                           unsigned int depth, unsigned int recursions);
+                           unsigned int depth, unsigned int recursions,
+                           unsigned int anydepth);
 
 /* For a FILE which has no commands specified, try to figure out some
    from the implicit pattern rules.
@@ -42,7 +43,7 @@ try_implicit_rule (struct file *file, unsigned int depth)
      (the archive search omits the archive name), it is more specific and
      should come first.  */
 
-  if (pattern_search (file, 0, depth, 0))
+  if (pattern_search (file, 0, depth, 0, 0))
     return 1;
 
 #ifndef NO_ARCHIVES
@@ -52,7 +53,7 @@ try_implicit_rule (struct file *file, unsigned int depth)
     {
       DBF (DB_IMPLICIT,
            _("Looking for archive-member implicit rule for '%s'.\n"));
-      if (pattern_search (file, 1, depth, 0))
+      if (pattern_search (file, 1, depth, 0, 0))
         return 1;
     }
 #endif
@@ -173,6 +174,9 @@ struct tryrule
 
     /* Nonzero if the LASTSLASH logic was used in matching this rule. */
     char checked_lastslash;
+
+    /* True if the rule would match anything. */
+    char anyrule;
   };
 
 int
@@ -200,7 +204,8 @@ stemlen_compare (const void *v1, const void *v2)
 
 static int
 pattern_search (struct file *file, int archive,
-                unsigned int depth, unsigned int recursions)
+                unsigned int depth, unsigned int recursions,
+                unsigned int anydepth)
 {
   /* Filename we are searching for a rule for.  */
   const char *filename = archive ? strchr (file->name, '(') : file->name;
@@ -317,12 +322,20 @@ pattern_search (struct file *file, int archive,
           const char *target = rule->targets[ti];
           const char *suffix = rule->suffixes[ti];
           char check_lastslash;
+          char anyrule = 0;
 
           /* Rules that can match any filename and are not terminal
-             are ignored if we're recursing, so that they cannot be
-             intermediate files.  */
-          if (recursions > 0 && target[1] == '\0' && !rule->terminal)
-            continue;
+             must not be allowed to nest very deeply.  Record whether
+             we have such a rule, so that if a further level of
+             recursion is required as a result of matching this rule,
+             we can increment the number of such rules we're nested
+             in.  */
+          if (target[1] == '\0' && !rule->terminal)
+            {
+              if (anydepth > 0)
+                continue;
+              anyrule = 1;
+            }
 
           if (rule->lens[ti] > namelen)
             /* It can't possibly match.  */
@@ -395,6 +408,7 @@ pattern_search (struct file *file, int archive,
              target in MATCHES.  If several targets of the same rule match,
              that rule will be in TRYRULES more than once.  */
           tryrules[nrules].rule = rule;
+          tryrules[nrules].anyrule = anyrule;
           tryrules[nrules].matches = ti;
           tryrules[nrules].stemlen = stemlen + (check_lastslash ? pathlen :
0);
           tryrules[nrules].order = nrules;
@@ -704,6 +718,7 @@ pattern_search (struct file *file, int archive,
               /* Go through the nameseq and handle each as a prereq name.
*/
               for (d = dl; d != 0; d = d->next)
                 {
+                  struct file *f;
                   struct dep *expl_d;
                   int is_rule = d->name == dep_name (dep);
 
@@ -753,7 +768,7 @@ pattern_search (struct file *file, int archive,
                      FILENAME's directory), so it might actually exist.  */
 
                   /* @@ dep->changed check is disabled. */
-                  if (lookup_file (d->name) != 0
+                  if (((f = lookup_file (d->name)) != 0 && f->is_target)
                       /*|| ((!dep->changed || check_lastslash) && */
                       || file_exists_p (d->name))
                     {
@@ -794,7 +809,8 @@ pattern_search (struct file *file, int archive,
                       if (pattern_search (int_file,
                                           0,
                                           depth + 1,
-                                          recursions + 1))
+                                          recursions + 1,
+                                          anydepth +
!!tryrules[ri].anyrule))
                         {
                           pat->pattern = int_file->name;
                           int_file->name = d->name;



    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?48643>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/


Reply | Threaded
Open this post in threaded view
|

[bug #48643] Irrelevant targets can confuse make on which pattern rule to select.

Paul D. Smith
Additional Item Attachment, bug #48643 (project make):

File name: fix-17752-48643.patch          Size:4 KB
    <https://file.savannah.gnu.org/file/fix-17752-48643.patch?file_id=50851>



    _______________________________________________________

Reply to this item at:

  <https://savannah.gnu.org/bugs/?48643>

_______________________________________________
  Message sent via Savannah
  https://savannah.gnu.org/