[PATCH 2/2] [SV 54161] Fix second expansion of $*

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

[PATCH 2/2] [SV 54161] Fix second expansion of $*

Jouke Witteveen
Before, this was only expanded to $(*F) in prerequisites.
---
 src/implicit.c | 49 +++++++++++++++++++++++++++++++------------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/src/implicit.c b/src/implicit.c
index 7bdc8ba..a887e92 100644
--- a/src/implicit.c
+++ b/src/implicit.c
@@ -220,8 +220,9 @@ pattern_search (struct file *file, int archive,
   struct patdeps *deplist = xmalloc (max_deps * sizeof (struct patdeps));
   struct patdeps *pat = deplist;
 
-  /* Names of possible dependencies are constructed in this buffer.  */
-  char *depname = alloca (namelen + max_pattern_dep_length);
+  /* Names of possible dependencies are constructed in this buffer.
+     We replace a % by $(*F) for second expansion, increasing the length.  */
+  char *depname = alloca (namelen + max_pattern_dep_length + 4);
 
   /* The start and length of the stem of FILENAME for the current rule.  */
   const char *stem = 0;
@@ -479,9 +480,10 @@ pattern_search (struct file *file, int archive,
                 }
             }
 
-          if (stemlen > GET_PATH_MAX)
+          if (stemlen + (check_lastslash ? pathlen : 0) > GET_PATH_MAX)
             {
-              DBS (DB_IMPLICIT, (_("Stem too long: '%.*s'.\n"),
+              DBS (DB_IMPLICIT, (_("Stem too long: '%s%.*s'.\n"),
+                                 check_lastslash ? pathdir : "",
                                  (int) stemlen, stem));
               continue;
             }
@@ -489,8 +491,19 @@ pattern_search (struct file *file, int archive,
           DBS (DB_IMPLICIT, (_("Trying pattern rule with stem '%.*s'.\n"),
                              (int) stemlen, stem));
 
-          strncpy (stem_str, stem, stemlen);
-          stem_str[stemlen] = '\0';
+          if (!check_lastslash)
+            {
+              memcpy (stem_str, stem, stemlen);
+              stem_str[stemlen] = '\0';
+            }
+          else
+            {
+              /* We want to prepend the directory from
+                 the original FILENAME onto the stem.  */
+              memcpy (stem_str, filename, pathlen);
+              memcpy (stem_str + pathlen, stem, stemlen);
+              stem_str[pathlen + stemlen] = '\0';
+            }
 
           /* If there are no prerequisites, then this rule matches.  */
           if (rule->deps == 0)
@@ -541,7 +554,7 @@ pattern_search (struct file *file, int archive,
                         }
                       memcpy (o, nptr, p - nptr);
                       o += p - nptr;
-                      memcpy (o, stem_str, stemlen);
+                      memcpy (o, stem, stemlen);
                       o += stemlen;
                       strcpy (o, p + 1);
                     }
@@ -586,15 +599,15 @@ pattern_search (struct file *file, int archive,
                       continue;
                     }
 
-                  /* If the dependency name has %, substitute the stem.  If we
-                     just replace % with the stem value then later, when we do
-                     the 2nd expansion, we will re-expand this stem value
-                     again.  This is not good if you have certain characters
-                     in your stem (like $).
+                  /* If the dependency name has %, substitute the filename of
+                     the stem.  If we just replace % with the stem value then
+                     later, when we do the 2nd expansion, we will re-expand
+                     this stem value again.  This is not good if you have
+                     certain characters in your stem (like $).
 
-                     Instead, we will replace % with $* and allow the second
-                     expansion to take care of it for us.  This way (since $*
-                     is a simple variable) there won't be additional
+                     Instead, we will replace % with $(*F) and allow the second
+                     expansion to take care of it for us.  This way (since
+                     $(*F) is a simple variable) there won't be additional
                      re-expansion of the stem.  */
 
                   p = lindex (nptr, nptr + len, '%');
@@ -607,9 +620,9 @@ pattern_search (struct file *file, int archive,
                     {
                       size_t i = p - nptr;
                       memcpy (depname, nptr, i);
-                      memcpy (depname + i, "$*", 2);
-                      memcpy (depname + i + 2, p + 1, len - i - 1);
-                      depname[len + 2 - 1] = '\0';
+                      memcpy (depname + i, "$(*F)", 5);
+                      memcpy (depname + i + 5, p + 1, len - i - 1);
+                      depname[len + 5 - 1] = '\0';
 
                       if (check_lastslash)
                         add_dir = 1;
--
2.23.0


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

Re: [PATCH 2/2] [SV 54161] Fix second expansion of $*

Paul Smith-20
On Sat, 2019-10-12 at 13:11 +0200, Jouke Witteveen wrote:
> Before, this was only expanded to $(*F) in prerequisites.

Sorry but I need more information than this; I can't understand this
change.

The bug in Savannah, as I understand it, is that directory prefixes
which should be present are missing during prerequisite expansion.

How does switching from $* to $(*F) (which is explicitly a file only)
help solve this problem?

Cheers!


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

Re: [PATCH 2/2] [SV 54161] Fix second expansion of $*

Jouke Witteveen
On Sat, Oct 12, 2019 at 2:50 PM Paul Smith <[hidden email]> wrote:

>
> On Sat, 2019-10-12 at 13:11 +0200, Jouke Witteveen wrote:
> > Before, this was only expanded to $(*F) in prerequisites.
>
> Sorry but I need more information than this; I can't understand this
> change.
>
> The bug in Savannah, as I understand it, is that directory prefixes
> which should be present are missing during prerequisite expansion.
>
> How does switching from $* to $(*F) (which is explicitly a file only)
> help solve this problem?
>
> Cheers!
>

Haha, yeah, this seems counterintuitive, doesn't it!
The true cause of the issue is not immediately visible in the patch:
stem_str is used as a temporary value of file->stem and this value
gets assigned to $* in the second expansion. To be consistent with the
value of $* in the recipe, we should thus prepend the path to stem_str
in some cases. This is what my patch does.

However, as a sort of 'hack', the current code replaces % by $* on the
first expansion, relying on the second expansion to do the right
thing. This does not work after fixing $*, as directories have a
special treatment in implicit rules (they are prepended to
prerequisites separately). The reason this currently works, is because
$* is set incorrectly. By changing it to $(*F), everything is fine
again also with the fixed $* :-).

I hope this clarifies what the patch does.

Regards,
- Jouke

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

Re: [PATCH 2/2] [SV 54161] Fix second expansion of $*

Jouke Witteveen
On Sat, Oct 12, 2019 at 3:08 PM Jouke Witteveen <[hidden email]> wrote:

>
> On Sat, Oct 12, 2019 at 2:50 PM Paul Smith <[hidden email]> wrote:
> >
> > On Sat, 2019-10-12 at 13:11 +0200, Jouke Witteveen wrote:
> > > Before, this was only expanded to $(*F) in prerequisites.
> >
> > Sorry but I need more information than this; I can't understand this
> > change.
> >
> > The bug in Savannah, as I understand it, is that directory prefixes
> > which should be present are missing during prerequisite expansion.
> >
> > How does switching from $* to $(*F) (which is explicitly a file only)
> > help solve this problem?
> >
> > Cheers!
> >
>
> Haha, yeah, this seems counterintuitive, doesn't it!
> The true cause of the issue is not immediately visible in the patch:
> stem_str is used as a temporary value of file->stem and this value
> gets assigned to $* in the second expansion. To be consistent with the
> value of $* in the recipe, we should thus prepend the path to stem_str
> in some cases. This is what my patch does.
>
> However, as a sort of 'hack', the current code replaces % by $* on the
> first expansion, relying on the second expansion to do the right
> thing. This does not work after fixing $*, as directories have a
> special treatment in implicit rules (they are prepended to
> prerequisites separately). The reason this currently works, is because
> $* is set incorrectly. By changing it to $(*F), everything is fine
> again also with the fixed $* :-).

I realized this morning that my testing was too focused on one
particular scenario and that whether % is replaced by $* or by $(*F)
should depend on whether check_lastslash is set or not (i.e. whether
add_dir is 1 or 0). I'll send a revised patch soon.

- Jouke

_______________________________________________
Bug-make mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/bug-make