bugmake - Bugs: bug #44742, Double-dep with double-colon rule...

 
 

bug #44742: Double-dep with double-colon rule not built

Submitter:  Ed <mohawk>
Submitted:  Mon 06 Apr 2015 12:08:02 AM UTC
Votes: 100
 
Severity:  3 - Normal Item Group:  Bug
Status:  Fixed Privacy:  Public
Assigned to:  psmith Open/Closed:  Closed
Component Version:  4.1 Operating System:  POSIX-Based
Fixed Release:  4.2 Triage Status:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Sun 22 May 2016 07:24:42 PM UTC, comment #11: 

The patch is attached to the ticket separate from my
comment:

https://savannah.gnu.org/bugs/download.php?file_id=37212

For the first change, it makes no sense to me to propagate a
timestamp on a remade rule to an unremade rule.  Which the
f_mtime() function is doing (verified in debugger) when
called from remake.c:179 (of git tree 4c9e10f).  We could
try to ensure that f_time is never called when it is not
safe to propagate a timestamp to all the double colon rules,
but it is more robust to make f_mtime() itself do the check
so that f_mtime() can be used more freely.

For the second change, notice that the problematic timestamp
propagation was happening at remake.c:179 but, in the
non-parallel case, a loop forcing all the double colon
targets to be remade before that happens at remake.c:358.
But for the parallel case, that loop only queues jobs and
all of the rules will be considered later after the next
reap_children() after the timestamp mistake occurs.

The loop is referenced in the changelog:


1999-12-02  Paul D. Smith  <psmith@gnu.org>

       * remake.c (update_file): Move the considered test outside the
       double-colon loop, _but_ make sure we test the double_colon target
       not the "current" target.  If we stop early because one
       double-colon target is running, mark all the rest considered and
       try to start their prerequisites (so they're marked considered).
       Fix for PR/1476 suggested by Tim Magill <tim.magill@telops.gte.com>.


I don't know what bug 1476 was and don't know how to find it
on the internet, but I suspect it was to fix an issue
regarding an insufficient number of double-colon rules being
run due to the timestamp propagation in f_mtime.  I could be
wrong, but I'll believe this loop is unnecessary until
somebody explains to me why we need it.  I left it in only
to make my change more conservative in that it only impacts
parallel builds with double colons.

Disclosure: I do not understand why other, earlier, calls to
f_mtime() have not been an issue.

Now, supposing I'm wrong about something or that I
misunderstand something, it does not matter much, because it
is easy to prove that my change does not affect any of
make's behavior unless it is running parallel and it
includes double-colons.  A situation that is currently badly
broken and a situation, which, empirically, my change fixes.
So I feel justified in submitting the patch even with an
incomplete understanding.

Joe Crayne <joecrayne>
Sun 22 May 2016 04:44:05 AM UTC, comment #10: 

"Here is a one-line fix: double-colons-bail-bug44742.patch"

1) There is no file attached.
2) Could you please explain the thinking behind those 2 patches.

Thanks

Anonymous
Sat 21 May 2016 09:36:13 PM UTC, comment #9: 

Thanks for those patches, Joe.  Fixed for the next release of GNU make.

Paul D. Smith <psmith>
Group administrator
Fri 20 May 2016 10:07:34 AM UTC, comment #8: 

Turns out that the original issue for this bug was separate than the one I addressed in my first patch.

Here is a one-line fix: double-colons-bail-bug44742.patch

Both patches need to be applied for double colon rules to work properly in parallel builds.

Joe Crayne <joecrayne>
Fri 20 May 2016 06:23:12 AM UTC, comment #7: 

This patch (double-colons-parallel-bug44742.patch) corrects the attached test case (double_colon_parallel_test.diff).

I believe it fixes this bug.  Test and let me know.

(file #37211)

Joe Crayne <joecrayne>
Sun 09 Aug 2015 02:40:30 PM UTC, comment #6: 

It's a bug.  It should definitely be possible to use double-colon rules with parallel builds.

Paul D. Smith <psmith>
Group administrator
Sun 09 Aug 2015 02:35:28 PM UTC, comment #5: 

It definitely does ruin the build. My question is, is this by design or simply a long-standing bug in GNU make? It doesn't happen on BSD make, for instance.

Ed <mohawk>
Wed 05 Aug 2015 09:46:34 AM UTC, comment #4: 

Double-colon may not "play" nicely with parallel-execution.

The following example shows one situation, where parallel execution may skip over a dependency (i.e. NOT building it at all), because it was linked in via a double-colon.

As shown later, this occurs only for parallel execution, so you should choose EITHER one of them, parallel-execution OR double-colon.

Using BOTH (parallel-execution AND double-colon), may ruin your build.


root: all;
        echo root


# 'all' is a double-colon,
#     This is the FIRST target in the double-colon linked-list
all::
        echo all_first

# 'all' is a double-colon,
#     This is the SECOND target in the double-colon linked-list
all:: 3;
        echo all_second


# implicit-rule to match targets: '1', '2', and '3'
%:
        sleep 0.$*


Running


make -r 1 2 root


We get:


sleep 0.1
sleep 0.2
echo all_first
all_first
sleep 0.3
echo all_second
all_second
echo root
root


Where, running


make -r -j2 1 2 root


We get:


sleep 0.1
sleep 0.2
echo all_first
all_first
sleep 0.3
echo root
root



Or, to put simply, running


make -rs 1 2 root


We get:


all_first
all_second
root


Where, running


make -rs -j2 1 2 root


We get:


all_first
root


Anonymous
Mon 03 Aug 2015 01:46:14 AM UTC, comment #3: 

Thanks for taking a look! I tried patching my local GNU make 4.1 per your suggestion, and it hasn't helped. Here is a fuller set of steps to repro:

<code>
# initial setup
cpanm --dev --look PDL
cpanm ExtUtils::MakeMaker@7.05_19 # 7.05* has double-level
perl Makefile.PL
cd Basic/Core

# first time, and show failure with -j4 linkext
make -j4 linkext
ls ../../blib/arch/auto/PDL/Core/Core.so # not found

# keep old files to save time
cp Version.pm Version.pm.KEPT ; cp Types.pm Types.pm.KEPT

# show works with -j4 dynamic
rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; make -j4 dynamic ; ls ../../blib/arch/auto/PDL/Core/Core.so

# show works with -j1 linkext
rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; make -j1 linkext; ls ../../blib/arch/auto/PDL/Core/Core.so

# show fails again/still with -j4 linkext
rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; make -j4 linkext; ls ../../blib/arch/auto/PDL/Core/Core.so
</code>

This is a problem for EUMM since it's a bug that affects already-existing GNU make, so I'm going to have to find a way to make this not happen for current GNU make. However, it also seems like it would be worth finding and fixing this.

(file #34568)

Ed <mohawk>
Sun 02 Aug 2015 06:29:49 PM UTC, comment #2: 

Thanks for taking a look! I tried patching my local GNU make 4.1 per your suggestion, and it hasn't helped. Here is a fuller set of steps to repro:

<code>
# initial setup
cpanm --dev --look PDL
cpanm ExtUtils::MakeMaker@7.05_19 # 7.05* has double-level
perl Makefile.PL
cd Basic/Core

# first time, and show failure with -j4 linkext
make -j4 linkext
ls ../../blib/arch/auto/PDL/Core/Core.so # not found

# keep old files to save time
cp Version.pm Version.pm.KEPT ; cp Types.pm Types.pm.KEPT

# show works with -j4 dynamic
rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; make -j4 dynamic ; ls ../../blib/arch/auto/PDL/Core/Core.so

# show works with -j1 linkext
rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; make -j1 linkext; ls ../../blib/arch/auto/PDL/Core/Core.so

# show fails again/still with -j4 linkext
rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; make -j4 linkext; ls ../../blib/arch/auto/PDL/Core/Core.so
</code>

This is a problem for EUMM since it's a bug that affects already-existing GNU make, so I'm going to have to find a way to make this not happen for current GNU make. However, it also seems like it would be worth finding and fixing this.

Ed <mohawk>
Sat 11 Jul 2015 06:02:45 PM UTC, comment #1: 

Sorry but I can't understand your repro case, or at least I can't reproduce it.  The makefile you link here doesn't appear to be a stand-alone repro case; if I run it by itself I just get errors about missing Makefile.PL.

If I install cpanm then run the command in your "full repro by" line, it builds (after I install some other Perl prerequisites) but I don't get the error you see: running with/without -j4 it always successfully creates the Core.so file.

If I try the commands you list below, removing, running make clean, etc. (it's not clear whether the "Makefile" you're copying is the one you attached here or the one left over from "make clean", but neither seem to work for me) I don't have any .KEPT files and I get errors about not being able to locate PDL/Types.pm.

However, someone sent me a fix which may be related to this.  If you edit the file remake.c in the GNU make sources around line 351 and change:

            if (new > status)
              new = status;

to this:

            if (new > status)
              status = new;

(Note there is another similar if-statement earlier in the function which already has "status = new"; they should both be that way.)

See if that change helps; I cannot reproduce your problem so I cannot check it myself.

Paul D. Smith <psmith>
Group administrator
Mon 06 Apr 2015 12:08:02 AM UTC, original submission:  

If you do "make -j4 linkext", it will not link the *.so. If you do "make -j4 dynamic", or "make -j1 linkext", it will.

This is the whole of the "linkext" rule:

linkext :: dynamic
        $(NOECHO) $(NOOP)


I attach the fuller log, the (cut-down) Makefile, the "remake -x" output of each case.

Full repro by: cpanm --dev --look PDL; cd Basic/Core; make linkext

Log:

$ make -v
GNU Make 4.1
[...]
$ remake -v
GNU Make 4.1+dbg0.91
[...]
$ rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; remake -x linkext >output.linkext ; l ../../blib/arch/auto/PDL/Core/Core.so
Using new 64bit index support
Using new 64bit index support
ls: cannot access ../../blib/arch/auto/PDL/Core/Core.so: No such file or directory
$ rm -rf ../../blib ; make clean >/dev/null; mv Makefile.old Makefile ; cp Version.pm.KEPT Version.pm ; cp Types.pm.KEPT Types.pm ; remake -x dynamic >output.dynamic ; l ../../blib/arch/auto/PDL/Core/Core.so
Using new 64bit index support
Using new 64bit index support
-rwxr-xr-x. 1 user user 168335 Apr  6 00:42 ../../blib/arch/auto/PDL/Core/Core.so

Ed <mohawk>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #37212:  double-colons-bail-bug44742.patch added by joecrayne (383B - text/x-patch - apply this also)
file #37211:  double-colons-parallel-bug44742.patch added by joecrayne (1KiB - text/x-patch - proposed solution)
file #37203:  double_colon_parallel_test.diff added by joecrayne (784B - text/x-patch - Here is a failing test case for parallel build of double-colon.)
file #34568:  Makefile.works added by mohawk (29KiB - application/octet-stream - Makefile from above scenario that now works with updated EUMM.)
file #33553:  log.txt added by mohawk (1KiB - text/plain)
file #33554:  output.dynamic added by mohawk (24KiB - application/octet-stream)
file #33555:  Makefile added by mohawk (11KiB - application/octet-stream)
file #33556:  output.linkext added by mohawk (21KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by joecrayne (Updated the item)
  • -email is unavailable- added by psmith (Posted a comment)
  • -email is unavailable- added by mohawk (Voted in favor of this item)
  • -email is unavailable- added by mohawk (Submitted the item)
  •  

    There are 100 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 13 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-05-21 psmith StatusNone Fixed
        Assigned toNone psmith
        Open/ClosedOpen Closed
        Fixed ReleaseNone 4.2
    2016-05-20 joecrayne Attached File- Added double-colons-bail-bug44742.patch, #37212
    2016-05-20 joecrayne Attached File- Added double-colons-parallel-bug44742.patch, #37211
    2016-05-19 joecrayne Attached File- Added double_colon_parallel_test.diff, #37203
    2015-08-03 mohawk Attached File- Added Makefile.works, #34568
    2015-04-06 mohawk Carbon-Copy- Added mohawk
    2015-04-06 mohawk Attached File- Added log.txt, #33553
        Attached File- Added output.dynamic, #33554
        Attached File- Added Makefile, #33555
        Attached File- Added output.linkext, #33556

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code