Remove stale generated asm files for functions which are no longer syscalls.

[emaste]

Summary

Supposedly, this should help for -DNO_CLEAN builds after r318736.

[emaste]

@kib beat you to it :)

[emaste]

lib/libc/Makefile

180 ↗

(On Diff #28721)

Perhaps we ought to have a comment on why this is here, ideally with the date and associated rev. Something like # 20170523 Remove stale generated asm files for functions which are no longer syscalls after r318736

In part so that we have a marker for eventually removing this, after getting over the bump.

[kib]

Update diff with Ed' suggested comment.

Note that there is no reasonable date in sight where this workaround could be removed. Upgrades from 11 to 12, which are performed with -DNO_CLEAN source builds, need it. I do not discuss the feasibility of such builds on major upgrades.

[emaste]

In D10876#225322, @kib wrote:

Note that there is no reasonable date in sight where this workaround could be removed. Upgrades from 11 to 12, which are performed with -DNO_CLEAN source builds, need it. I do not discuss the feasibility of such builds on major upgrades.

Yes, I would not expect this for a user upgrade from 11 to 12; I would like to establish as a rule of thumb that we expect -DNO_CLEAN to work for developers (and Jenkins CI jobs) building -CURRENT regularly - e.g. last build up to a week or so ago. I'd like to facilitate quicker turnaround on CI builds, as well as bisecting and building.

lib/libc/Makefile

185 ↗

(On Diff #28722)

As a hack to get the build to work I had to also add:

@rm -f ${.OBJDIR}/getdents.*o ${.OBJDIR}/lstat.*o \
    ${.OBJDIR}/mknod.*o ${.OBJDIR}/stat.*o
@rm -f ${.OBJDIR}/.depend.getdents.* ${.OBJDIR}/.depend.lstat.* \
    ${.OBJDIR}/.depend.mknod.* ${.OBJDIR}/.depend.stat.*

which is somewhat self-defeating. Maybe @bdrewery has some insight?

[emaste]

lib/libc/Makefile

185 ↗

(On Diff #28722)

I guess it could be done with a rule that tests for the existence of getdents.[sS], and if it exists remove the .depend and object files, in addition to getdents.[sS]?

[bdrewery]

lib/libc/Makefile

185 ↗

(On Diff #28722)

Yeah I think it would be better to remove all of this.

[bdrewery]

lib/libc/Makefile

185 ↗

(On Diff #28722)

It might even be enough to remove the .depend files from what I remember of trying to fix this before.

[emaste]

lib/libc/Makefile

185 ↗

(On Diff #28722)

I tried the original patch as posted by @kib and it was not sufficient.

[emaste]

Test for one of the generated asm files, and only if it exists try to remove them as well as the .depend files.

[emaste]

from a build log after touch getdents.S in the objdir:

...
--- beforedepend ---
removing stale generated syscall asm files
--- getdents.pico ---
--- lstat.pico ---
--- mknod.pico ---
--- stat.pico ---
--- getdents.o ---
--- lstat.o ---
--- mknod.o ---
--- getdents.pico ---
cc -target x86_64-unknown-freebsd12.0 --sysroot=/usr/obj/usr/home/emaste/src/freebsd/tmp -B/usr/obj/usr/home/emaste/src/freebsd/tmp/usr/bin -fpic -DPIC -g -O2 -pipe   -I/usr/home/emaste/src/freebsd/lib/libc/include -I/usr/home/emaste/src/freebsd/include -I/usr/home/emaste/src/freebsd/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/usr/home/emaste/src/freebsd/contrib/gdtoa -I/usr/home/emaste/src/freebsd/contrib/libc-vis -DINET6 -I/usr/obj/usr/home/emaste/src/freebsd/lib/libc -I/usr/home/emaste/src/freebsd/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/usr/home/emaste/src/freebsd/lib/libmd -I/usr/home/emaste/src/freebsd/contrib/jemalloc/include -I/usr/home/emaste/src/freebsd/contrib/tzcode/stdtime -I/usr/home/emaste/src/freebsd/lib/libc/stdtime -I/usr/home/emaste/src/freebsd/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/usr/home/emaste/src/freebsd/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -MD  -MF.depend.getdents.pico -MTgetdents.pico -std=gnu99 -fstack-protector-strong -Wsy--- lstat.pico ---
...

[emaste]

Does not work as @bdrewery points out because the bad .S file will be cached from the .depend, before the .depend is deleted.

[emaste]

I modified the .depend file to refer to the ${.OBJDIR}/getdents.S and see

--- beforedepend ---
removing stale generated syscall asm files
--- getdents.pico ---
--- lstat.pico ---
--- mknod.pico ---
--- stat.pico ---
--- getdents.o ---
--- lstat.o ---
--- mknod.o ---
--- getdents.o ---
cc -target x86_64-unknown-freebsd12.0 --sysroot=/usr/obj/usr/home/emaste/src/freebsd/tmp -B/usr/obj/usr/home/emaste/src/freebsd/tmp/usr/bin -O2 -pipe   -I/usr/home/emaste/src/freebsd/lib/libc/include -I/usr/home/emaste/src/freebsd/include -I/usr/home/emaste/src/freebsd/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/usr/home/emaste/src/freebsd/contrib/gdtoa -I/usr/home/emaste/src/freebsd/contrib/libc-vis -DINET6 -I/usr/obj/usr/home/emaste/src/freebsd/lib/libc -I/usr/home/emaste/src/freebsd/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/usr/home/emaste/src/freebsd/lib/libmd -I/usr/home/emaste/src/freebsd/contrib/jemalloc/include -I/usr/home/emaste/src/freebsd/contrib/tzcode/stdtime -I/usr/home/emaste/src/freebsd/lib/libc/stdtime -I/usr/home/emaste/src/freebsd/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/usr/home/emaste/src/freebsd/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -MD  -MF.depend.getdents.o -MTgetdents.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter  -Qunused-arguments  -I/usr/home/emaste/src/freebsd/lib/libutil -I/usr/home/emaste/src/freebsd/lib/msun/amd64 -I/usr/home/emaste/src/freebsd/lib/msun/x86 -I/usr/home/emaste/src/freebsd/lib/msun/src   -c /usr/obj/usr/home/emaste/src/freebsd/lib/libc/getdents.S -o getdents.o
cc: error: no such file or directory: '/usr/obj/usr/home/emaste/src/freebsd/lib/libc/getdents.S'
cc: error: no input files
*** [getdents.o] Error code 1

[emaste]

clean asm files from _worldtmp instead

[emaste]

correct rm args

[cem]

I like the idea. Not super familiar with make syntax nor am I able to test this right now.

Maybe capitalize "Removing?" Otherwise, seems fine to me.

[emaste]

Thanks for the feedback. I will capitalize Removing based on the apparently common style in this file.

This final iteration seems to work, but I really want @bdrewery's acceptance before I'll commit.

[jhb]

If you are going to do this you should include pipe() fallout as well, and maybe open() if that was also removed? However, that means this can't just depend on getdents.S existing but should perhaps just be unconditional?

[emaste]

If you are going to do this you should include pipe() fallout as well, and maybe open() if that was also removed?

I can do that in a later change, because

However, that means this can't just depend on getdents.S existing but should perhaps just be unconditional?

I can't see any way to make that work correctly, because it will mean that we always remove the associated files and cause these objects to be rebuilt.

Maybe something like

.for old in getdents lstat mknod stat
.if exists(${OBJTREE}${.CURDIR}/lib/libc/${old}.s) || \
    exists(${OBJTREE}${.CURDIR}/lib/libc/${old}.S)
        cleanup+=${old}
.endif
.endfor

.for f in ${cleanup}
...

but I'd like to do that in a subsequent change.

[jhb]

Hmm, so the source switched, wasn't outright removed. The same is true of pipe.S as well FWIW. If you don't care about the 'echo' then you can just do the rm's directly inside the first .if, or maybe use a helper make variable to only echo the first time:

.for f in getdents lstat mknod stat pipe
  .if exists(...)
     .if !defined(STALE_WARNED)
       @echo ...
       STALE_WARNED=yes
     .endif
     rm ...
  .endif
.endfor

[emaste]

Include pipe and simplify with @jhb's suggestion.

For simplicity just echo each old syscall being removed. This will only happen once per build tree and only under NO_CLEAN, and there's some diagnostic value in having each case shown.

[emaste]

Upload correct diff this time

[bdrewery]

Makefile.inc1

722 ↗

(On Diff #28781)

I like this approach.
You only need to check for .depend.${f}.o/pico/po and then only need to remove the .depend files. There's no need to check for the .S files or remove them. I note why later...
The reason the .depend files need to be removed is that they are what are tainting the build due to the stale dependency rule being added in:

lstat.o: lstat.S

Otherwise it will likely error with 'dont know how to make lstat.S' or fail a compilation against that file.

Here's a test of that building directly in lib/libc. Then I removed the lstat.* files:

# rm /usr/obj/root/svn/base/lib/libc/lstat.*

Then I svn up.
In the .depend.lstat.o it had:

# less /usr/obj/root/svn/base/lib/libc/.depend.lstat.o
lstat.o: lstat.S /root/svn/base/lib/libc/include/compat.h \
  /root/svn/base/lib/libc/amd64/SYS.h /usr/include/sys/syscall.h \
  /usr/include/machine/asm.h /usr/include/sys/cdefs.h

So when I rebuilt with 'make lstat.o' it generated lstat.S and then used it to compile:

# make lstat.o
printf '#include "compat.h"\n' > lstat.S
printf '#include "SYS.h"\nRSYSCALL(lstat)\n' >> lstat.S
printf  '\t.section .note.GNU-stack,"",%%progbits\n' >>lstat.S
cc -O2 -pipe   -I/root/svn/base/lib/libc/include -I/root/svn/base/include -I/root/svn/base/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/root/svn/base/contrib/gdtoa -I/root/svn/base/contrib/libc-vis -DINET6 -I/usr/obj/root/svn/base/lib/libc -I/root/svn/base/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/root/svn/base/lib/libmd -I/root/svn/base/contrib/jemalloc/include -DMALLOC_PRODUCTION -I/root/svn/base/contrib/tzcode/stdtime -I/root/svn/base/lib/libc/stdtime -I/root/svn/base/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/root/svn/base/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -MD  -MF.depend.lstat.o -MTlstat.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter  -Qunused-arguments  -I/root/svn/base/lib/libutil -I/root/svn/base/lib/msun/amd64 -I/root/svn/base/lib/msun/x86 -I/root/svn/base/lib/msun/src   -c lstat.S -o lstat.o

This is bad, removing only the .o/S files still ended up building from the bad .S file.

This only uses lstat.c once the .depend.lstat.o file is removed:

# rm /usr/obj/root/svn/base/lib/libc/.depend.lstat.*
# make lstat.o
printf '#include "compat.h"\n' > lstat.S
printf '#include "SYS.h"\nRSYSCALL(lstat)\n' >> lstat.S
printf  '\t.section .note.GNU-stack,"",%%progbits\n' >>lstat.S
/usr/local/bin/ccache cc  -O2 -pipe   -I/root/svn/base/lib/libc/include -I/root/svn/base/include -I/root/svn/base/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/root/svn/base/contrib/gdtoa -I/root/svn/base/contrib/libc-vis -DINET6 -I/usr/obj/root/svn/base/lib/libc -I/root/svn/base/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/root/svn/base/lib/libmd -I/root/svn/base/contrib/jemalloc/include -DMALLOC_PRODUCTION -I/root/svn/base/contrib/tzcode/stdtime -I/root/svn/base/lib/libc/stdtime -I/root/svn/base/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/root/svn/base/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -MD  -MF.depend.lstat.o -MTlstat.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter  -Qunused-arguments  -I/root/svn/base/lib/libutil -I/root/svn/base/lib/msun/amd64 -I/root/svn/base/lib/msun/x86 -I/root/svn/base/lib/msun/src -c /root/svn/base/lib/libc/sys/lstat.c -o lstat.o

Also note that the lstat.S is still generated in the proper build case, so checking for its existence here is just going to cause these files to rebuild every time.

It is sufficient to just check for and remove the .depend files (did the same rebuild->upgrade thing here):

# rm /usr/obj/root/svn/base/lib/libc/.depend.lstat.*
# make lstat.o
/usr/local/bin/ccache cc  -O2 -pipe   -I/root/svn/base/lib/libc/include -I/root/svn/base/include -I/root/svn/base/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/root/svn/base/contrib/gdtoa -I/root/svn/base/contrib/libc-vis -DINET6 -I/usr/obj/root/svn/base/lib/libc -I/root/svn/base/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/root/svn/base/lib/libmd -I/root/svn/base/contrib/jemalloc/include -DMALLOC_PRODUCTION -I/root/svn/base/contrib/tzcode/stdtime -I/root/svn/base/lib/libc/stdtime -I/root/svn/base/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/root/svn/base/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -MD  -MF.depend.lstat.o -MTlstat.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter  -Qunused-arguments  -I/root/svn/base/lib/libutil -I/root/svn/base/lib/msun/amd64 -I/root/svn/base/lib/msun/x86 -I/root/svn/base/lib/msun/src -c /root/svn/base/lib/libc/sys/lstat.c -o lstat.o

It built the .c file properly here.

[emaste]

Makefile.inc1

722 ↗

(On Diff #28781)

You only need to check for .depend.${f}.o/pico/po and then only need to remove the .depend files. There's no need to check for the .S files or remove them. I note why later...

Checking for .depend.${f}.o means that we'll have to rebuild the dep for ${f}.o each time, no?

I agree that the .depend files have to be removed - and the current version of the patch does so in ${OBJTREE}${.CURDIR}/lib/libc/.depend.${f}.*.

Also note that the lstat.S is still generated in the proper build case, so checking for its existence here is just going to cause these files to rebuild every time.

After rS318736 none of getdents.S lstat.S mknod.S pipe.S stat.S should exist in the obj tree.

# rm /usr/obj/root/svn/base/lib/libc/.depend.lstat.*
# make lstat.o
printf '#include "compat.h"\n' > lstat.S
printf '#include "SYS.h"\nRSYSCALL(lstat)\n' >> lstat.S

This is a bug, it shouldn't be creating lstat.S for this. When I tried it in my work tree get lstat.o built from lstat.c as in your final example.

[bdrewery]

Makefile.inc1

722 ↗

(On Diff #28781)

This is a bug, it shouldn't be creating lstat.S for this. When I tried it in my work tree get lstat.o built from lstat.c as in your final example.

Ah I see, I had to update _sys/sys/syscall.mk_ as well for this to stop generating.

Checking for .depend.${f}.o means that we'll have to rebuild the dep for ${f}.o each time, no?

Ah yes true. Though my concern is that the current condition you have will not fix the problem if someone does 'make clean'.
In that case, the rule will still taint the build if an old .depend file exists and cause a failure.

I need to step away to eat, but a quick mockup of this shows it is a problem:

# make lstat.o
make: /usr/obj/root/svn/base/lib/libc/.depend.lstat.o, 20: ignoring stale .depend for lstat.S
cc -O2 -pipe   -I/root/svn/base/lib/libc/include -I/root/svn/base/include -I/root/svn/base/lib/libc/amd64 -DNLS  -D__DBINTERFACE_PRIVATE -I/root/svn/base/contrib/gdtoa -I/root/svn/base/contrib/libc-vis -DINET6 -I/usr/obj/root/svn/base/lib/libc -I/root/svn/base/lib/libc/resolv -D_ACL_PRIVATE -DPOSIX_MISTAKE -I/root/svn/base/lib/libmd -I/root/svn/base/contrib/jemalloc/include -DMALLOC_PRODUCTION -I/root/svn/base/contrib/tzcode/stdtime -I/root/svn/base/lib/libc/stdtime -I/root/svn/base/lib/libc/locale -DBROKEN_DES -DPORTMAP -DDES_BUILTIN -I/root/svn/base/lib/libc/rpc -DWANT_HYPERV -DYP -DNS_CACHING -DSYMBOL_VERSIONING -MD  -MF.depend.lstat.o -MTlstat.o -std=gnu99 -fstack-protector-strong -Wsystem-headers -Werror -Wall -Wno-format-y2k -Wno-uninitialized -Wno-pointer-sign -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-variable -Wno-tautological-compare -Wno-unused-value -Wno-parentheses-equality -Wno-unused-function -Wno-enum-conversion -Wno-unused-local-typedef -Wno-address-of-packed-member -Wno-switch -Wno-switch-enum -Wno-knr-promoted-parameter  -Qunused-arguments  -I/root/svn/base/lib/libutil -I/root/svn/base/lib/msun/amd64 -I/root/svn/base/lib/msun/x86 -I/root/svn/base/lib/msun/src   -c lstat.S -o lstat.o
cc: error: no such file or directory: 'lstat.S'
cc: error: no input files
*** Error code 1

Stop.
make: stopped in /root/svn/base/lib/libc

It fails until the .depend file is removed.

I agree that the .depend files have to be removed - and the current version of the patch does so in ${OBJTREE}${.CURDIR}/lib/libc/.depend.${f}.*.

I either missed that or was saying we shouldn't need to remove anything but those.

[emaste]

Makefile.inc1

722 ↗

(On Diff #28781)

will not fix the problem if someone does 'make clean'.

Yeah, but this is just a limitation of doing it from _worldtmp, no? This change as it exists, or checking for .depend, will have no effect on make clean. I suspect we should separately change make clean to remove all .depend files.

I either missed that or was saying we shouldn't need to remove anything but those.

Well, I think we do want to remove the generated .S files, else the stale copies will remain in the tree, and it seems like it's somewhat unspecified if we build from the .c or .S.

[bdrewery]

Let's just take this for now. I really need to fix this in the general case. It's a black mark on the FAST_DEPEND work for me.

Makefile.inc1

722 ↗

(On Diff #28781)

Yeah, but this is just a limitation of doing it from _worldtmp, no? This change as it exists, or checking for .depend, will have no effect on make clean. I suspect we should separately change make clean to remove all .depend files.

It is intended that make clean does not remove depend files, though admittedly it is less useful to keep them after all of the work I did to fix no-depend-files-yet builds.

[emaste]

Makefile.inc1

722 ↗

(On Diff #28781)

It is intended that make clean does not remove depend files, though admittedly it is less useful to keep them after all of the work I did to fix no-depend-files-yet builds.

Ah, ok. So perhaps what we could do is move this logic into a remove-stale-syscalls target in libc's Makefile, and then invoke it from both _worldtmp and libc's clean target. I will try to take a look at that approach.