Re: svn commit: r364761 - in head: share/mk sys/conf sys/modules/cloudabi32 sys/modules/cloudabi64 sys/modules/linux sys/modules/linux64

[Alexander Richardson]

On Sun, 13 Dec 2020 at 19:22, Ryan Libby  wrote:
>
> On Tue, Aug 25, 2020 at 6:30 AM Alex Richardson  
> wrote:
> >
> > Author: arichardson
> > Date: Tue Aug 25 13:30:03 2020
> > New Revision: 364761
> > URL: https://svnweb.freebsd.org/changeset/base/364761
> >
> > Log:
> >   Pass -fuse-ld=/path/to/ld if ${LD} != "ld"
> >
> >   This is needed so that setting LD/XLD is not ignored when linking with $CC
> >   instead of directly using $LD. Currently only clang accepts an absolute
> >   path for -fuse-ld= (Clang 12+ will add a new --ld-path flag), so we now
> >   warn when building with GCC and $LD != "ld" since that might result in the
> >   wrong linker being used.
> >
> >   We have been setting XLD=/path/to/cheri/ld.lld in CheriBSD for a long 
> > time and
> >   used a similar version of this patch to avoid linking with /usr/bin/ld.
> >   This change is also required when building FreeBSD on an Ubuntu with 
> > Clang:
> >   In that case we set XCC=/usr/lib/llvm-10/bin/clang and since
> >   /usr/lib/llvm-10/bin/ does not contain a "ld" binary the build fails with
> >   `clang: error: unable to execute command: Executable "ld" doesn't exist!`
> >   unless we pass -fuse-ld=/usr/lib/llvm-10/bin/ld.lld.
> >
> >   This change passes -fuse-ld instead of copying ${XLD} to WOLRDTMP/bin/ld
> >   since then we would have to ensure that this file does not exist while
> >   building the bootstrap tools. The cross-linker might not be compatible 
> > with
> >   the host linker (e.g. when building on macos: host-linker= Mach-O 
> > /usr/bin/ld,
> >   cross-linker=LLVM ld.lld).
> >
> >   Reviewed By:  brooks, emaste
> >   Differential Revision: https://reviews.freebsd.org/D26055
> >
> > Modified:
> >   head/share/mk/bsd.sys.mk
> >   head/sys/conf/kern.mk
> >   head/sys/conf/kern.post.mk
> >   head/sys/modules/cloudabi32/Makefile
> >   head/sys/modules/cloudabi64/Makefile
> >   head/sys/modules/linux/Makefile
> >   head/sys/modules/linux64/Makefile
> >
> > Modified: head/share/mk/bsd.sys.mk
> > ==
> > --- head/share/mk/bsd.sys.mkTue Aug 25 13:29:57 2020(r364760)
> > +++ head/share/mk/bsd.sys.mkTue Aug 25 13:30:03 2020(r364761)
> > @@ -284,6 +284,19 @@ CFLAGS+=   ERROR-tried-to-rebuild-during-make-install
> >  .endif
> >  .endif
> >
> > +# Please keep this if in sync with kern.mk
> > +.if ${LD} != "ld" && (${CC:[1]:H} != ${LD:[1]:H} || ${LD:[1]:T} != "ld")
> > +# Add -fuse-ld=${LD} if $LD is in a different directory or not called "ld".
> > +# Note: Clang 12+ will prefer --ld-path= over -fuse-ld=.
> > +.if ${COMPILER_TYPE} == "clang"
> > +LDFLAGS+=  -fuse-ld=${LD:[1]}
> > +.else
> > +# GCC does not support an absolute path for -fuse-ld so we just print this
> > +# warning instead and let the user add the required symlinks.
> > +.warning LD (${LD}) is not the default linker for ${CC} but -fuse-ld= is 
> > not supported
>
> FYI: This causes a huge amount of wrong and irrelevant warnings in the
> build log for gcc cross compilers with XLD set, such as the toolchain
> from pkg install amd64-xtoolchain-gcc.  That causes XLD to be set (via
> CROSS_BINUTILS_PREFIX in Makefile.inc1) to a path which is indeed the
> default linker for the cross compiler.  The warnings are harmless, but
> annoying.  Sorry, I don't have a suggestion for a better method.
>
> You can see an example in the build log from a gcc build in CI here
> (although note that the build currently fails for unrelated reasons):
> https://ci.freebsd.org/job/FreeBSD-head-amd64-gcc6_build/3131
>
Hi Ryan,

I think there are (at least) 3 options to fix/silence this warning:

1) The easiest option would be to turn off this warning for GCC builds
and hope that the user-supplied LD matches the one that GCC will
invoke.

2) Another option could be to ask GCC for what it thinks the default
linker is and use that in the toolchain file, and update the check to
use `${CC} --print-prog-name ld` instead of (${CC:[1]:H} !=
${LD:[1]:H} || ${LD:[1]:T} != "ld")
$ /usr/local/bin/mips-unknown-freebsd12.1-gcc6 --print-prog-name ld
/usr/local/bin/mips-unknown-freebsd12.1-ld
$ /usr/local/bin/mips-unknown-freebsd12.1-gcc6 --print-prog-name ld.bfd
/usr/local/lib/gcc/mips-unknown-freebsd12.1/6.5.0/../../../../mips-unknown-freebsd12.1/bin/ld.bfd

2a) We could also use ${CC} -xc /dev/null -o /dev/null -Wl,--version`
to detect linker features in bsd.linker.mk and check if that matches
the value returned by ${LD} --version.

3) Change the xtoolchain installed a gcc-N hardlink in
/usr/local/-unknown-freebsd/bin and use that as ${CC}.
It would match then the ld directory and the existing check could
work.

I'd personally opt for 1 since I don't build with GCC and don't have
time right now to implement the other options.
Let me know what you think.

Thanks,
Alex

> > +.endif
> > +.endif
> > +
> >  # Tell bmake not to mistake standard targets for things to be searched for
> >  # or expect to ever be 

Re: svn commit: r368609 - in head/sys: kern sys

[Alexander Richardson]

On Sun, 13 Dec 2020 at 18:06, Mateusz Guzik  wrote:
>
> Author: mjg
> Date: Sun Dec 13 18:06:24 2020
> New Revision: 368609
> URL: https://svnweb.freebsd.org/changeset/base/368609
>
> Log:
>   fd: fix fdrop prediction when closing a fd
>
>   Most of the time this is the last reference, contrary to typical fdrop use.
>
> Modified:
>   head/sys/kern/kern_descrip.c
>   head/sys/sys/file.h
>
> Modified: head/sys/kern/kern_descrip.c
> ==
> --- head/sys/kern/kern_descrip.cSun Dec 13 16:26:37 2020
> (r368608)
> +++ head/sys/kern/kern_descrip.cSun Dec 13 18:06:24 2020
> (r368609)
> @@ -2766,7 +2766,7 @@ closef(struct file *fp, struct thread *td)
> FILEDESC_XUNLOCK(fdp);
> }
> }
> -   return (fdrop(fp, td));
> +   return (fdrop_close(fp, td));
>  }
>
>  /*
>
> Modified: head/sys/sys/file.h
> ==
> --- head/sys/sys/file.h Sun Dec 13 16:26:37 2020(r368608)
> +++ head/sys/sys/file.h Sun Dec 13 18:06:24 2020(r368609)
> @@ -299,6 +299,17 @@ fhold(struct file *fp)
> _error; \
>  })
>
> +#definefdrop_close(fp, td) ({  \
> +   struct file *_fp;   \
> +   int _error; \
> +   \
> +   _error = 0; \
> +   _fp = (fp); \
> +   if (__predict_true(refcount_release(&_fp->f_count)))\
> +   _error = _fdrop(_fp, td);   \
> +   _error; \
> +})
> +
>  static __inline fo_rdwr_t  fo_read;
>  static __inline fo_rdwr_t  fo_write;
>  static __inline fo_truncate_t  fo_truncate;

Wouldn't this be more readable as a static __inline function (or if
you are concerned about it not being inlined, __always_inline)? Also
means you can drop the temporary _fp variable that's there to avoid
side-effects in macro args.

Regards,
Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r368329 - head/stand/kshim

[Alexander Richardson]

On Fri, 4 Dec 2020 at 16:06, Hans Petter Selasky  wrote:
>
> On 12/4/20 4:59 PM, Alexander Richardson wrote:
> > On Fri, 4 Dec 2020 at 14:51, Hans Petter Selasky  
> > wrote:
> >>
> >> Author: hselasky
> >> Date: Fri Dec  4 14:50:55 2020
> >> New Revision: 368329
> >> URL: https://svnweb.freebsd.org/changeset/base/368329
> >>
> >> Log:
> >>Fix definition of int64_t and uint64_t when long is 64-bit. This gets 
> >> the kernel
> >>shim code in line with the rest of the kernel, sys/x86/include/_types.h.
> >>
> >>MFC after:1 week
> >>Sponsored by: Mellanox Technologies // NVIDIA Networking
> >>
> >> Modified:
> >>head/stand/kshim/bsd_kernel.h
> >>
> >> Modified: head/stand/kshim/bsd_kernel.h
> >> ==
> >> --- head/stand/kshim/bsd_kernel.h   Fri Dec  4 14:09:12 2020
> >> (r368328)
> >> +++ head/stand/kshim/bsd_kernel.h   Fri Dec  4 14:50:55 2020
> >> (r368329)
> >> @@ -208,9 +208,17 @@ typedef unsigned int uint32_t;
> >>   #define_INT32_T_DECLARED
> >>   typedef signed int int32_t;
> >>   #define_UINT64_T_DECLARED
> >> +#ifndef __LP64__
> >>   typedef unsigned long long uint64_t;
> >> +#else
> >> +typedef unsigned long uint64_t;
> >> +#endif
> >>   #define_INT16_T_DECLARED
> >> +#ifndef __LP64__
> >>   typedef signed long long int64_t;
> >> +#else
> >> +typedef signed long int64_t;
> >> +#endif
> >>
> >>   typedef uint16_t uid_t;
> >>   typedef uint16_t gid_t;
> >
> > Since we no longer support ancient compilers, could we simplify this
> > and just use
> > typedef __UINT64_TYPE__ uint64_t;
> > typedef __INT64_TYPE__ int64_t;
> > ?
> >
> > This will work across all architectures and ABIs, and appears to work
> > starting with GCC 4.5.3 and Clang 3.5:
> > https://godbolt.org/z/TWavfb
>
> Hi Alexander,
>
> I'm not sure how that definition will work together with existing code,
> mixing uint64_t, unsigned long, and unsigned long long. Will this cause
> more compiler warnings? This also will affect user-space and ports.
>
> Maybe Konstantin, CC'ed, has some input on this?
>
> --HPS
>

I haven't looked at GCC's source code, but clang tries to make
__UINT64_TYPE__ expand to what the target expects: it picks unsigned
long/unsigned long long depending on the target OS and architecture.
E.g. if I look at clang/Basic/Targets/AArch64.cpp, I can see that
clang uses unsigned long for uint64_t everywhere except
OpenBSD/Windows/Darwin where it uses unsigned long long.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r368329 - head/stand/kshim

[Alexander Richardson]

On Fri, 4 Dec 2020 at 14:51, Hans Petter Selasky  wrote:
>
> Author: hselasky
> Date: Fri Dec  4 14:50:55 2020
> New Revision: 368329
> URL: https://svnweb.freebsd.org/changeset/base/368329
>
> Log:
>   Fix definition of int64_t and uint64_t when long is 64-bit. This gets the 
> kernel
>   shim code in line with the rest of the kernel, sys/x86/include/_types.h.
>
>   MFC after:1 week
>   Sponsored by: Mellanox Technologies // NVIDIA Networking
>
> Modified:
>   head/stand/kshim/bsd_kernel.h
>
> Modified: head/stand/kshim/bsd_kernel.h
> ==
> --- head/stand/kshim/bsd_kernel.h   Fri Dec  4 14:09:12 2020
> (r368328)
> +++ head/stand/kshim/bsd_kernel.h   Fri Dec  4 14:50:55 2020
> (r368329)
> @@ -208,9 +208,17 @@ typedef unsigned int uint32_t;
>  #define_INT32_T_DECLARED
>  typedef signed int int32_t;
>  #define_UINT64_T_DECLARED
> +#ifndef __LP64__
>  typedef unsigned long long uint64_t;
> +#else
> +typedef unsigned long uint64_t;
> +#endif
>  #define_INT16_T_DECLARED
> +#ifndef __LP64__
>  typedef signed long long int64_t;
> +#else
> +typedef signed long int64_t;
> +#endif
>
>  typedef uint16_t uid_t;
>  typedef uint16_t gid_t;

Since we no longer support ancient compilers, could we simplify this
and just use
typedef __UINT64_TYPE__ uint64_t;
typedef __INT64_TYPE__ int64_t;
?

This will work across all architectures and ABIs, and appears to work
starting with GCC 4.5.3 and Clang 3.5:
https://godbolt.org/z/TWavfb

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366993 - head/sys/net

[Alexander Richardson]

On Sat, 24 Oct 2020 at 13:52, Alexey Dokuchaev  wrote:
>
> On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
> > New Revision: 366993
> > URL: https://svnweb.freebsd.org/changeset/base/366993
> >
> > Log:
> >   Run code through "clang-format -style=file" with some additional fixes.
> >   No functional change.
> >
> > ...
> > @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
> >
> >  #ifdef INET6
> >  static inline void
> > -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
> > -const uint8_t *broadcast, uint8_t *buf)
> > +infiniband_ipv6_multicast_map(
> > +const struct in6_addr *addr, const uint8_t *broadcast, uint8_t *buf)
> >  {
>
> This is not how we format these in FreeBSD, please revert.  It was correct
> before and no "fix" is need here.
>
> ./danfe

Unfortunately this is a limitation of the current clang-format
version. I've submitted a patch upstream as
https://reviews.llvm.org/D90246 and the config file change is
https://reviews.freebsd.org/D26978.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366993 - head/sys/net

[Alexander Richardson]

On Sat, 24 Oct 2020, 16:27 Warner Losh,  wrote:

>
>
> On Sat, Oct 24, 2020, 7:38 AM Alexander V. Chernikov 
> wrote:
>
>> 24.10.2020, 14:08, "Hans Petter Selasky" :
>> > On 2020-10-24 14:52, Alexey Dokuchaev wrote:
>> >>  On Sat, Oct 24, 2020 at 10:23:22AM +, Hans Petter Selasky wrote:
>> >>>  New Revision: 366993
>> >>>  URL: https://svnweb.freebsd.org/changeset/base/366993
>> >>>
>> >>>  Log:
>> >>> Run code through "clang-format -style=file" with some additional
>> fixes.
>> >>> No functional change.
>> >>>
>> >>>  ...
>> >>>  @@ -99,8 +97,8 @@ infiniband_ipv4_multicast_map(uint32_t addr,
>> >>>
>> >>>#ifdef INET6
>> >>>static inline void
>> >>>  -infiniband_ipv6_multicast_map(const struct in6_addr *addr,
>> >>>  - const uint8_t *broadcast, uint8_t *buf)
>> >>>  +infiniband_ipv6_multicast_map(
>> >>>  + const struct in6_addr *addr, const uint8_t *broadcast, uint8_t
>> *buf)
>> >>>{
>> >>
>> >>  This is not how we format these in FreeBSD, please revert. It was
>> correct
>> >>  before and no "fix" is need here.
>> Given we already have nice .clang-format, that does most of the job,
>> maybe it's worth considering looking into tweaking it further to fix this
>> part?
>> It would be nice if we could finally offload all formatting issues to the
>> tool and focus on the actual code :-)
>>
>
> It would be nice if it produced one of the style(9) acceptable formats
> without disrupting things already acceptable.  That's been the big problem
> with the tweaks to date... some things are fixed, others break. It's
> getting a lot closer, though
>


I've upstreamed a few fixes, but haven't got to the line
wrapping/continuation indentation stuff yet. That part of clang format is
not particularly easy to modify without breaking other stuff and I'm also
rather short on time right now, so probably won't get to it any time soon.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366697 - head/usr.bin/xinstall

[Alexander Richardson]

On Thu, 15 Oct 2020 at 06:59, Enji Cooper  wrote:
>
>
> > On Oct 14, 2020, at 5:28 AM, Alex Richardson  
> > wrote:
> >
> > Author: arichardson
> > Date: Wed Oct 14 12:28:41 2020
> > New Revision: 366697
> > URL: https://svnweb.freebsd.org/changeset/base/366697
> >
> > Log:
> >  install(1): Avoid unncessary fstatfs() calls and use mmap() based on size
> >
> >  According to git blame the trymmap() function was added in 1996 to skip
> >  mmap() calls for NFS file systems. However, nowadays mmap() should be
> >  perfectly safe even on NFS. Importantly, onl ufs and cd9660 file systems
> >  were whitelisted so we don't use mmap() on ZFS. It also prevents the use
> >  of mmap() when bootstrapping from macOS/Linux since on those systems the
> >  trymmap() function was always returning zero due to the missing MFSNAMELEN
> >  define.
> >
> >  This change keeps the trymmap() function but changes it to check whether
> >  using mmap() can reduce the number of system calls that are required.
> >  Using mmap() only reduces the number of system calls if we need multiple 
> > read()
> >  syscalls, i.e. if the file size is > MAXBSIZE. However, mmap() is more 
> > expensive
> >  than read() so this sets the threshold at 4 fewer syscalls. Additionally, 
> > for
> >  larger file size mmap() can significantly increase the number of page 
> > faults,
> >  so avoid it in that case.
> >
> >  It's unclear whether using mmap() is ever faster than a read with an 
> > appropriate
> >  buffer size, but this change at least removes two unnecessary system calls
> >  for every file that is installed.
> >
> >  Reviewed By: markj
> >  Differential Revision: https://reviews.freebsd.org/D26041
>
> * Has this change been tested out with source filesystems other than 
> UFS/ZFS? Not all filesystems support mmap(2).

I've used ext4 and afps, but it doesn't matter since there's a fallback.

> * trymmap(..) seems to be less about computing whether or not the 
> filesystem should use mmap(2) after this change, but how it should use 
> mmap(2). Seems like a misnamed function call now.
There was always fallback code in case mmap fails:
https://github.com/freebsd/freebsd/blob/8349de39d23fc152c3782ee3b9eeab3df4610944/usr.bin/xinstall/xinstall.c#L1253
and 
https://github.com/freebsd/freebsd/blob/8349de39d23fc152c3782ee3b9eeab3df4610944/usr.bin/xinstall/xinstall.c#L1253
so it is really "should I try to use mmap()".

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366697 - head/usr.bin/xinstall

[Alexander Richardson]

On Wed, 14 Oct 2020 at 14:29, Mateusz Guzik  wrote:
>
> This should use copy_file_range (also available on Linux).
>

I agree. I even mentioned this in https://reviews.freebsd.org/D26041#589287.
This change avoids the two unnecessary syscalls, but I agree that
longer-term install should share the copy_file_range code with cp.
The only thing that copy_file_range won't speed up is the check
whether source and target are already identical.

Alex

> On 10/14/20, Alex Richardson  wrote:
> > Author: arichardson
> > Date: Wed Oct 14 12:28:41 2020
> > New Revision: 366697
> > URL: https://svnweb.freebsd.org/changeset/base/366697
> >
> > Log:
> >   install(1): Avoid unncessary fstatfs() calls and use mmap() based on size
> >
> >   According to git blame the trymmap() function was added in 1996 to skip
> >   mmap() calls for NFS file systems. However, nowadays mmap() should be
> >   perfectly safe even on NFS. Importantly, onl ufs and cd9660 file systems
> >   were whitelisted so we don't use mmap() on ZFS. It also prevents the use
> >   of mmap() when bootstrapping from macOS/Linux since on those systems the
> >   trymmap() function was always returning zero due to the missing
> > MFSNAMELEN
> >   define.
> >
> >   This change keeps the trymmap() function but changes it to check whether
> >   using mmap() can reduce the number of system calls that are required.
> >   Using mmap() only reduces the number of system calls if we need multiple
> > read()
> >   syscalls, i.e. if the file size is > MAXBSIZE. However, mmap() is more
> > expensive
> >   than read() so this sets the threshold at 4 fewer syscalls. Additionally,
> > for
> >   larger file size mmap() can significantly increase the number of page
> > faults,
> >   so avoid it in that case.
> >
> >   It's unclear whether using mmap() is ever faster than a read with an
> > appropriate
> >   buffer size, but this change at least removes two unnecessary system
> > calls
> >   for every file that is installed.
> >
> >   Reviewed By:markj
> >   Differential Revision: https://reviews.freebsd.org/D26041
> >
> > Modified:
> >   head/usr.bin/xinstall/xinstall.c
> >
> > Modified: head/usr.bin/xinstall/xinstall.c
> > ==
> > --- head/usr.bin/xinstall/xinstall.c  Wed Oct 14 10:12:39 2020
> > (r366696)
> > +++ head/usr.bin/xinstall/xinstall.c  Wed Oct 14 12:28:41 2020
> > (r366697)
> > @@ -148,7 +148,7 @@ static void   metadata_log(const char *, const char 
> > *, s
> >   const char *, const char *, off_t);
> >  static int   parseid(const char *, id_t *);
> >  static int   strip(const char *, int, const char *, char **);
> > -static int   trymmap(int);
> > +static int   trymmap(size_t);
> >  static void  usage(void);
> >
> >  int
> > @@ -1087,7 +1087,7 @@ compare(int from_fd, const char *from_name __unused,
> > s
> >   if (do_digest)
> >   digest_init();
> >   done_compare = 0;
> > - if (trymmap(from_fd) && trymmap(to_fd)) {
> > + if (trymmap(from_len) && trymmap(to_len)) {
> >   p = mmap(NULL, from_len, PROT_READ, MAP_SHARED,
> >   from_fd, (off_t)0);
> >   if (p == MAP_FAILED)
> > @@ -1248,13 +1248,8 @@ copy(int from_fd, const char *from_name, int to_fd,
> > co
> >
> >   digest_init();
> >
> > - /*
> > -  * Mmap and write if less than 8M (the limit is so we don't totally
> > -  * trash memory on big files.  This is really a minor hack, but it
> > -  * wins some CPU back.
> > -  */
> >   done_copy = 0;
> > - if (size <= 8 * 1048576 && trymmap(from_fd) &&
> > + if (trymmap((size_t)size) &&
> >   (p = mmap(NULL, (size_t)size, PROT_READ, MAP_SHARED,
> >   from_fd, (off_t)0)) != MAP_FAILED) {
> >   nw = write(to_fd, p, size);
> > @@ -1523,20 +1518,23 @@ usage(void)
> >   *   return true (1) if mmap should be tried, false (0) if not.
> >   */
> >  static int
> > -trymmap(int fd)
> > +trymmap(size_t filesize)
> >  {
> > -/*
> > - * The ifdef is for bootstrapping - f_fstypename doesn't exist in
> > - * pre-Lite2-merge systems.
> > - */
> > -#ifdef MFSNAMELEN
> > - struct statfs stfs;
> > -
> > - if (fstatfs(fd, ) != 0)
> > - return (0);
> > - if (strcmp(stfs.f_fstypename, "ufs") == 0 ||
> > - strcmp(stfs.f_fstypename, "cd9660") == 0)
> > - return (1);
> > -#endif
> > - return (0);
> > + /*
> > +  * This function existed to skip mmap() for NFS file systems whereas
> > +  * nowadays mmap() should be perfectly safe. Nevertheless, using 
> > mmap()
> > +  * only reduces the number of system calls if we need multiple read()
> > +  * syscalls, i.e. if the file size is > MAXBSIZE. However, mmap() is
> > +  * more expensive than read() so set the threshold at 4 fewer 
> > syscalls.
> > +  * 

Re: svn commit: r366634 - head/contrib/nvi/common

[Alexander Richardson]

On Mon, 12 Oct 2020 at 14:55, Jessica Clarke  wrote:
>
> On 12 Oct 2020, at 11:42, Alex Richardson  wrote:
> > --- head/contrib/nvi/common/common.h  Mon Oct 12 10:42:19 2020
> > (r366633)
> > +++ head/contrib/nvi/common/common.h  Mon Oct 12 10:42:24 2020
> > (r366634)
> > @@ -14,7 +14,7 @@
> > #ifdef __linux__
> > #include "/usr/include/db1/db.h"  /* Only include db1. */
> > #else
> > -#include "/usr/include/db.h" /* Only include db1. */
> > +#include   /* Only include db1. */
> > #endif
>
> Can this not be expressed more nicely as the following?
>
> /* Only include db1 */
> #if __has_include()
> #include 
> #else
> #include 
> #endif
>
> Jess
>

Yes it could, but I'd prefer it if upstream fixes it instead so we can
drop this diff. See also https://github.com/lichray/nvi2/issues/69.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r366169 - head/sys/mips/include

[Alexander Richardson]

On Fri, 25 Sep 2020, 20:04 Justin Hibbits,  wrote:

> Author: jhibbits
> Date: Fri Sep 25 19:04:03 2020
> New Revision: 366169
> URL: https://svnweb.freebsd.org/changeset/base/366169
>
> Log:
>   mips: Fix compat32 library builds from r366162
>
>   Re-add the a_ptr and a_fcn fields to Elf32_Auxinfo.
>
>   MFC after:1 week
>   Sponsored by: Juniper Networks, Inc.
>
> Modified:
>   head/sys/mips/include/elf.h
>
> Modified: head/sys/mips/include/elf.h
>
> ==
> --- head/sys/mips/include/elf.h Fri Sep 25 19:02:49 2020(r366168)
> +++ head/sys/mips/include/elf.h Fri Sep 25 19:04:03 2020(r366169)
> @@ -105,6 +105,10 @@ typedef struct {   /* Auxiliary vector entry on
> initial
> int a_type; /* Entry type. */
> union {
> int a_val;  /* Integer value. */
> +#ifndef __mips_n64
> +   void*a_ptr; /* Address. */
> +   void(*a_fcn)(void); /* Function pointer (not used). */
> +#endif
> } a_un;
>  } Elf32_Auxinfo;


Not sure what the current minimal compiler versions are, but maybe this
should be #if __SIZEOF_POINTER__ == 4 instead of checking the ABI? This
would break CHERI-MIPS kernels since we don't define __mips_n64 for the
pure-capability ABI (128-bit pointers). However, we don't really do compat
32 right now so it probably doesn't matter much.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r365876 - in head/lib/libarchive: . tests

[Alexander Richardson]

On Fri, 18 Sep 2020 at 14:12, Kyle Evans  wrote:
>
> On Fri, Sep 18, 2020 at 6:22 AM Alex Richardson  
> wrote:
> >
> > Author: arichardson
> > Date: Fri Sep 18 11:22:34 2020
> > New Revision: 365876
> > URL: https://svnweb.freebsd.org/changeset/base/365876
> >
> > Log:
> >   libarchive: fix mismatch between library and test configuration
> >
> >   I was investigating libarchive test failures on CheriBSD and it turns out
> >   we get a reproducible SIGBUS for test_archive_m5, etc. Debugging this 
> > shows
> >   that libarchive and the tests disagree when it comes to the definition of
> >   archive_md5_ctx: libarchive assumes it's the OpenSSL type whereas the test
> >   use the libmd type. The latter is not necessarily aligned enough to store
> >   a pointer (16 bytes for CHERI RISC-V), so we were crashing when storing
> >   EVP_MD_CTX* to an 8-byte-aligned archive_md5_ctx.
> >
> >   To avoid problems like this in the future, factor out the common compiler
> >   flags into a Makefile.inc and include that from the tests Makefile.
> >
> >   Reviewed By:  lwhsu
> >   Differential Revision: https://reviews.freebsd.org/D26469
> >
> > Added:
> >   head/lib/libarchive/Makefile.inc   (contents, props changed)
> > Modified:
> >   head/lib/libarchive/Makefile
> >   head/lib/libarchive/tests/Makefile
> >
> > [.. snip ..]
> > Modified: head/lib/libarchive/tests/Makefile
> > ==
> > --- head/lib/libarchive/tests/Makefile  Fri Sep 18 11:04:16 2020
> > (r365875)
> > +++ head/lib/libarchive/tests/Makefile  Fri Sep 18 11:22:34 2020
> > (r365876)
> > @@ -1,4 +1,5 @@
> >  # $FreeBSD$
> > +.include 
> >
> >  PACKAGE=   tests
> >
> > @@ -15,7 +16,7 @@ PROGS+=   libarchive_test
> >  CFLAGS+= -I${.CURDIR} -I${.CURDIR:H} -I${.OBJDIR}
> >  CFLAGS+= -I${_LIBARCHIVEDIR}/libarchive -I${_LIBARCHIVEDIR}/libarchive/test
> >  CFLAGS+= -I${_LIBARCHIVEDIR}/test_utils
> > -CFLAGS+= -DHAVE_LIBLZMA=1 -DHAVE_LZMA_H=1
> > +.include "../Makefile.inc"
> >
> >  # Uncomment to link against dmalloc
> >  #LDADD+= -L/usr/local/lib -ldmalloc
>
> Is the explicit .include here necessary (e.g. for ordering)? I note
> that inclusion of the parent directory's Makefile.inc is already
> guaranteed by bsd.init.mk, so this duplicates all the CFLAGS
> additions.
>
Thanks, I didn't notice that it was already being included. Fixed in r365882

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r365836 - head/share/mk

[Alexander Richardson]

On Thu, 17 Sep 2020 at 18:05, Rodney W. Grimes
 wrote:
>
> > On Thu, Sep 17, 2020 at 9:39 AM Steffen Nurpmeso  wrote:
> >
> > > Alex Richardson wrote in
> > >  <202009171507.08hf7qns080...@repo.freebsd.org>:
> > >  |Author: arichardson
> > >  |Date: Thu Sep 17 15:07:25 2020
> > >  |New Revision: 365836
> > >  |URL: https://svnweb.freebsd.org/changeset/base/365836
> > >  |
> > >  |Log:
> > >  |  Stop using lorder and ranlib when building libraries
> > >  |
> > >  |  Use of ranlib or lorder is no longer necessary with current linkers
> > >  |  (probably anything newer than ~1990) and ar's ability to create an
> > > object
> > >  |  index and symbol table in the archive.
> > >  |  Currently the build system uses lorder+tsort to sort the .o files in
> > >  |  dependency order so that a single-pass linker can use them. However,
> > >  |  we can use the -s flag to ar to add an index to the .a file which 
> > > makes
> > >  |  lorder unnecessary.
> > >  |  Running ar -s is equivalent to running ranlib afterwards, so we can
> > > also
> > >  |  skip the ranlib invocation.
> > >
> > > That ranlib thing yes (for long indeed), but i have vague memories
> > > that the tsort/lorder ordering was also meant to keep the things
> > > which heavily interdepend nearby each other.  (Luckily Linux
> > > always had at least tsort available.)
> > > This no longer matters for all the platforms FreeBSD supports?
> > >
> >
> > tsort has no notion of how dependent the modules are, just an order that
> > allows a single pass through the .a file (otherwise you'd need to list the
> > .a file multiple times on the command line absent ranlib). That's the
> > original purpose of tsort. tsort, lsort, and ranlib all arrived in 7th
> > edition unix on a PDP-11, where size was more important than proximity to
> > locations (modulo overlays, which this doesn't affect at all).
> >
> > There were some issues of long vs short jumps on earlier architectures that
> > this helped (since you could only jump 16MB, for example). However, there
> > were workarounds for this issue on those platforms too. And if you have a
> > program that this does make a difference, then you can still use
> > tsort/lorder. They are still in the system.
> >
> > I doubt you could measure a difference here today. I doubt, honestly, that
> > anybody will notice at all.
>
> The x86 archicture has relative jmps of differning lengths, even in long mode
> there is support for rel8 and rel32.

However, unless you have linker relaxations (e.g. RISC-V) the compiler
has to emit the large size anyway since the linker won't replace a
32-bit immediate with an 8-bit one since that changes all later
offsets.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r364190 - head/tools/build

[Alexander Richardson]

On Thu, 13 Aug 2020 at 17:28, Jessica Clarke  wrote:
>
> On 13 Aug 2020, at 17:22, Rodney W. Grimes  wrote:
> >
> >> Author: arichardson
> >> Date: Thu Aug 13 14:14:46 2020
> >> New Revision: 364190
> >> URL: https://svnweb.freebsd.org/changeset/base/364190
> >>
> >> Log:
> >>  Add pwd to the list of tools that are linked to $WORLDTMP/legacy
> >
> > Since "sh" is already in this list, and our "sh" has a builtin pwd
> > that does the correct thing with pwd -P this should not be needed.
> >
> > Or are we contininue to use the host "sh" for far too long?
> >
> > For me from ancient days of hand bootstrapping BSD sources onto
> > another system sh(1) and make(1) are the first 2 tools to get
> > working.
>
> The issue is that r364174 used `env pwd -P` rather than just `pwd -P`. With
> that fixed, this should be revertible; even if the bootstrap sh isn't being
> used at this point, I don't know of any contemporary sh-compatible shell that
> doesn't implement pwd as a builtin (but surely we are using the bootstrap sh 
> by
> this point otherwise BUILD_WITH_STRICT_TMPPATH would have complained about 
> sh).
>
> Jess

I'll change it to use pwd instead of env pwd shortly and also revert
the change that added pwd to the linked tools since using the shell
builtin with -P is fine.
We are not bootstrapping sh yet, but instead copying it from /usr/bin
since it is not (yet) possible to bootstrap the base system sh on
macOS

Alex

>
> >>  After r364166 and r364174, crunchgen needs a pwd binary in $PATH instead
> >>  of using a hardcoded absolute path. This commit is needed for
> >>  BUILD_WITH_STRICT_TMPPATH builds (currently not on by default).
> >>
> >> Modified:
> >>  head/tools/build/Makefile
> >>
> >> Modified: head/tools/build/Makefile
> >> ==
> >> --- head/tools/build/MakefileThu Aug 13 13:59:31 2020
> >> (r364189)
> >> +++ head/tools/build/MakefileThu Aug 13 14:14:46 2020
> >> (r364190)
> >> @@ -113,8 +113,8 @@ SYSINCS+=${SRCTOP}/sys/sys/font.h
> >> # Linux/MacOS since we only use flags that are supported by all of them.
> >> _host_tools_to_symlink=  basename bzip2 bunzip2 chmod chown cmp comm 
> >> cp date dd \
> >>  dirname echo env false find fmt gzip gunzip head hostname id ln ls \
> >> -mkdir mv nice patch rm realpath sh sleep stat tee touch tr true uname 
> >> \
> >> -uniq wc which
> >> +mkdir mv nice patch pwd rm realpath sh sleep stat tee touch tr true \
> >> +uname uniq wc which
> >>
> >> # We also need a symlink to the absolute path to the make binary used for
> >> # the toplevel makefile. This is not necessarily the same as `which make`
> >>
> >
> > --
> > Rod Grimes 
> > rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r363992 - head/usr.sbin/pwd_mkdb

[Alexander Richardson]

On Sat, 8 Aug 2020 at 17:27, Ian Lepore  wrote:
>
> On Sat, 2020-08-08 at 11:08 +0100, Alexander Richardson wrote:
> > On Sat, 8 Aug 2020 at 02:19, Mateusz Guzik  wrote:
> > >
> > > This broke i386 builds:
> > >
> > > /usr/src/usr.bin/chpass/field.c:175:15: error: incompatible pointer
> > > types passing
> > >   '_bootstrap_time_t *' (aka 'unsigned long long *') to
> > > parameter
> > > of type 'time_t *'
> > >   (aka 'int *') [-Werror,-Wincompatible-pointer-types]
> > > if (!atot(p, >pw_change))
> > >  ^~
> > > /usr/src/usr.bin/chpass/chpass.h:67:27: note: passing argument to
> > > parameter here
> > > int  atot(char *, time_t *);
> > >   ^
> > > /usr/src/usr.bin/chpass/field.c:185:15: error: incompatible pointer
> > > types passing
> > >   '_bootstrap_time_t *' (aka 'unsigned long long *') to
> > > parameter
> > > of type 'time_t *'
> > >   (aka 'int *') [-Werror,-Wincompatible-pointer-types]
> > > if (!atot(p, >pw_expire))
> > >  ^~
> > > /usr/src/usr.bin/chpass/chpass.h:67:27: note: passing argument to
> > > parameter here
> > > int  atot(char *, time_t *);
> > >   ^
> >
> > Sorry, fixed in r364049.
> >
>
> It may be fixed in terms of compiling, but how about at runtime?
> _bootstrap_time_t is still typedef'd as 64-bit, but on i386 a time_t is
> a 32-bit type.
>
> -- Ian
>

The structure is only used as a temporary buffer before serializing
the values to a fixed format.
That format truncates all integers to 32-bit so changing it to 64-bits
on i386 it does not change the format.
Arguably, pwd_mkdb should be using 64-bit time_t values everywhere,
but this commit is intended to allow bootstrapping and not to
introduce a future-proof binary format.

Alex

> > > On 8/6/20, Alex Richardson  wrote:
> > > > Author: arichardson
> > > > Date: Thu Aug  6 20:46:13 2020
> > > > New Revision: 363992
> > > > URL: https://svnweb.freebsd.org/changeset/base/363992
> > > >
> > > > Log:
> > > >   Allow bootstrapping pwd_mkdb on Linux/macOS
> > > >
> > > >   We need to provide a struct passwd that is compatible with the
> > > > target
> > > >   system and this is not the case when cross-building from
> > > > macOS/Linux.
> > > >   It should also be a problem when bootstrapping for an i386
> > > > target from a
> > > >   FreeBSD amd64 host since time_t does not match across those
> > > > systems.
> > > >   However, pwd_mkdb always truncates integer values to 32-bit so
> > > > this
> > > >   difference does not result in different databases.
> > > >
> > > >   Reviewed By:brooks
> > > >   Differential Revision: https://reviews.freebsd.org/D25931
> > > >
> > > > Added:
> > > >   head/usr.sbin/pwd_mkdb/pwd.h   (contents, props changed)
> > > > Modified:
> > > >   head/usr.sbin/pwd_mkdb/Makefile
> > > >
> > > > Modified: head/usr.sbin/pwd_mkdb/Makefile
> > > > =
> > > > =
> > > > --- head/usr.sbin/pwd_mkdb/Makefile   Thu Aug  6 20:44:40
> > > > 2020(r363991)
> > > > +++ head/usr.sbin/pwd_mkdb/Makefile   Thu Aug  6 20:46:13
> > > > 2020(r363992)
> > > > @@ -9,5 +9,8 @@ MAN=  pwd_mkdb.8
> > > >  SRCS=pw_scan.c pwd_mkdb.c
> > > >
> > > >  CFLAGS+= -I${SRCTOP}/lib/libc/gen# for pw_scan.h
> > > > +.if defined(BOOTSTRAPPING)
> > > > +CFLAGS+=-I${.CURDIR}
> > > > +.endif
> > > >
> > > >  .include 
> > > >
> > > > Added: head/usr.sbin/pwd_mkdb/pwd.h
> > > > =
> > > > =
> > > > --- /dev/null 00:00:00 1970   (empty, because file is newly
> > > > added)
> > > > +++ head/usr.sbin/pwd_mkdb/pwd.h  Thu Aug  6 20:46:13
> > > > 2020(r363992)
> > > > @@ -0,0 +1,66 @@
> > > > +/*-
> > > > + * SPDX-License-Identifier: BSD-2-Clause
> > > > + *
> > > > + * Copyright 2018-2020 Alex Richardson 
> > > > 

Re: svn commit: r363992 - head/usr.sbin/pwd_mkdb

[Alexander Richardson]

On Sat, 8 Aug 2020 at 02:19, Mateusz Guzik  wrote:
>
> This broke i386 builds:
>
> /usr/src/usr.bin/chpass/field.c:175:15: error: incompatible pointer
> types passing
>   '_bootstrap_time_t *' (aka 'unsigned long long *') to parameter
> of type 'time_t *'
>   (aka 'int *') [-Werror,-Wincompatible-pointer-types]
> if (!atot(p, >pw_change))
>  ^~
> /usr/src/usr.bin/chpass/chpass.h:67:27: note: passing argument to parameter 
> here
> int  atot(char *, time_t *);
>   ^
> /usr/src/usr.bin/chpass/field.c:185:15: error: incompatible pointer
> types passing
>   '_bootstrap_time_t *' (aka 'unsigned long long *') to parameter
> of type 'time_t *'
>   (aka 'int *') [-Werror,-Wincompatible-pointer-types]
> if (!atot(p, >pw_expire))
>  ^~
> /usr/src/usr.bin/chpass/chpass.h:67:27: note: passing argument to parameter 
> here
> int  atot(char *, time_t *);
>   ^

Sorry, fixed in r364049.

> On 8/6/20, Alex Richardson  wrote:
> > Author: arichardson
> > Date: Thu Aug  6 20:46:13 2020
> > New Revision: 363992
> > URL: https://svnweb.freebsd.org/changeset/base/363992
> >
> > Log:
> >   Allow bootstrapping pwd_mkdb on Linux/macOS
> >
> >   We need to provide a struct passwd that is compatible with the target
> >   system and this is not the case when cross-building from macOS/Linux.
> >   It should also be a problem when bootstrapping for an i386 target from a
> >   FreeBSD amd64 host since time_t does not match across those systems.
> >   However, pwd_mkdb always truncates integer values to 32-bit so this
> >   difference does not result in different databases.
> >
> >   Reviewed By:brooks
> >   Differential Revision: https://reviews.freebsd.org/D25931
> >
> > Added:
> >   head/usr.sbin/pwd_mkdb/pwd.h   (contents, props changed)
> > Modified:
> >   head/usr.sbin/pwd_mkdb/Makefile
> >
> > Modified: head/usr.sbin/pwd_mkdb/Makefile
> > ==
> > --- head/usr.sbin/pwd_mkdb/Makefile   Thu Aug  6 20:44:40 2020
> > (r363991)
> > +++ head/usr.sbin/pwd_mkdb/Makefile   Thu Aug  6 20:46:13 2020
> > (r363992)
> > @@ -9,5 +9,8 @@ MAN=  pwd_mkdb.8
> >  SRCS=pw_scan.c pwd_mkdb.c
> >
> >  CFLAGS+= -I${SRCTOP}/lib/libc/gen# for pw_scan.h
> > +.if defined(BOOTSTRAPPING)
> > +CFLAGS+=-I${.CURDIR}
> > +.endif
> >
> >  .include 
> >
> > Added: head/usr.sbin/pwd_mkdb/pwd.h
> > ==
> > --- /dev/null 00:00:00 1970   (empty, because file is newly added)
> > +++ head/usr.sbin/pwd_mkdb/pwd.h  Thu Aug  6 20:46:13 2020
> > (r363992)
> > @@ -0,0 +1,66 @@
> > +/*-
> > + * SPDX-License-Identifier: BSD-2-Clause
> > + *
> > + * Copyright 2018-2020 Alex Richardson 
> > + *
> > + * This software was developed by SRI International and the University of
> > + * Cambridge Computer Laboratory (Department of Computer Science and
> > + * Technology) under DARPA contract HR0011-18-C-0016 ("ECATS"), as part of
> > the
> > + * DARPA SSITH research programme.
> > + *
> > + * This software was developed by SRI International and the University of
> > + * Cambridge Computer Laboratory under DARPA/AFRL contract
> > (FA8750-10-C-0237)
> > + * ("CTSRD"), as part of the DARPA CRASH research programme.
> > + *
> > + * Redistribution and use in source and binary forms, with or without
> > + * modification, are permitted provided that the following conditions
> > + * are met:
> > + * 1. Redistributions of source code must retain the above copyright
> > + *notice, this list of conditions and the following disclaimer.
> > + * 2. Redistributions in binary form must reproduce the above copyright
> > + *notice, this list of conditions and the following disclaimer in the
> > + *documentation and/or other materials provided with the distribution.
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND
> > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> > + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
> > PURPOSE
> > + * ARE DISCLAIMED.  IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE
> > + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> > CONSEQUENTIAL
> > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS
> > + * OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> > + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> > STRICT
> > + * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY
> > WAY
> > + * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> > + * SUCH DAMAGE.
> > + *
> > + * $FreeBSD$
> > + */
> > +
> > +/*
> > + * When building pwd_mkdb we need to use target systems definition of
> > + * struct 

Re: svn commit: r364023 - head/usr.bin/grep

[Alexander Richardson]

On Fri, 7 Aug 2020 at 17:06, Kyle Evans  wrote:
>
> On Fri, Aug 7, 2020 at 11:04 AM Alex Richardson  
> wrote:
> >
> > Author: arichardson
> > Date: Fri Aug  7 16:04:01 2020
> > New Revision: 364023
> > URL: https://svnweb.freebsd.org/changeset/base/364023
> >
> > Log:
> >   Always install usr.bin/grep as grep when bootstrapping
> >
> >   We have to bootstrap grep when cross-building from macOS/Linux.
> >
>
> What's going on here that it isn't bootstrapping gnugrep over in
> gnu/usr.bin/grep as grep? My gut instinct is that we're missing
> something else and this isn't quite the right place to have addressed
> this.
>

The logic in Makefile.inc1 builds usr.bin/grep as the bootstrap grep
when BOOTSTRAP_ALL_TOOLS is set (e.g. when building on macOS/Linux).
If you think this should be gnu/usr.bin/grep instead I can revert this
change and update Makefile.inc1.
We have been using usr.bin/grep for bootstrap in CheriBSD for a long
time now and it works fine so I'm not sure whether we need to use GNU
grep for bootstrapping?

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r362217 - head/stand/common

[Alexander Richardson]

On Tue, 16 Jun 2020 at 18:09, Cy Schubert  wrote:
>
> In message <55903c38d363aef2a6f6d0075dd4526b86d51258.ca...@freebsd.org>,
> Ian Le
> pore writes:
> > On Tue, 2020-06-16 at 07:05 +, Toomas Soome wrote:
> > > Author: tsoome
> > > Date: Tue Jun 16 07:05:03 2020
> > > New Revision: 362217
> > > URL: https://svnweb.freebsd.org/changeset/base/362217
> > >
> > > Log:
> > >   loader: variable i is unused without MBR/GPT support built in
> > >
> > >   Because i is only used as index in for loop, declare it in for
> > > statement.
> > >
> >
> > As much as I prefer doing it this way, style(9) doesn't allow for
> > variable declarations inside a for() statement (or even inside a local
> > block, which is just too 1980s for me, but it is still our standard).
>
> Doesn't this use stack for a shorter period of time or does the compiler
> optimize this, making this change moot?
>
> The tradeoff is a few extra bytes of stack for a longer period of time vs a
> few extra instructions incrementing and decrementing the stack pointer.
>
>
At least for LLVM it makes no difference where you declare the
variable: it will always be represented by an alloca instruction at
the start of the IR function:
https://godbolt.org/z/NFG3hN
There will also not be any changes to the stack pointer since the
stack frame size is fixed no matter where the variables are declared
(unless you use variable-length arrays or __builtin_alloca()).

LLVM will also merge variables that have non-overlapping lifetimes to
reuse the same stack slot if possible.
I'm almost certain GCC performs the same optimizations (as will any
other compiler written in the last 20 years).

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r360073 - head/tests/sys/kqueue/libkqueue

[Alexander Richardson]

On Sat, 18 Apr 2020 at 20:13, Conrad Meyer  wrote:
>
> Hi Alex,
>
> We usually do not use "extern" for function declarations in headers.
>
> Best,
> Conrad

Hi Conrad,

I decided to follow the style that was used in the header already, but
I agree those externs should probably be dropped.
I can commit a follow-up change to remove them on Monday.

Alex

>
> On Sat, Apr 18, 2020 at 5:55 AM Alex Richardson  
> wrote:
> >
> > Author: arichardson
> > Date: Sat Apr 18 12:54:47 2020
> > New Revision: 360073
> > URL: https://svnweb.freebsd.org/changeset/base/360073
> >
> > Log:
> >   Fix various warnings in tests/sys/kqueue and bump WARNS
> >
> >   Reviewed By:  kevans
> >   Differential Revision: https://reviews.freebsd.org/D24296
> >
> > Modified:
> >   head/tests/sys/kqueue/libkqueue/Makefile
> >   head/tests/sys/kqueue/libkqueue/common.h
> >   head/tests/sys/kqueue/libkqueue/main.c
> >   head/tests/sys/kqueue/libkqueue/proc.c
> >   head/tests/sys/kqueue/libkqueue/read.c
> >   head/tests/sys/kqueue/libkqueue/signal.c
> >   head/tests/sys/kqueue/libkqueue/timer.c
> >   head/tests/sys/kqueue/libkqueue/user.c
> >   head/tests/sys/kqueue/libkqueue/vnode.c
> >
> > Modified: head/tests/sys/kqueue/libkqueue/Makefile
> > ==
> > --- head/tests/sys/kqueue/libkqueue/MakefileSat Apr 18 12:54:40 2020
> > (r360072)
> > +++ head/tests/sys/kqueue/libkqueue/MakefileSat Apr 18 12:54:47 2020
> > (r360073)
> > @@ -16,6 +16,5 @@ SRCS.kqtest=  \
> > proc.c  \
> > signal.c\
> > user.c
> > -WARNS?=2
> >
> >  .include 
> >
> > Modified: head/tests/sys/kqueue/libkqueue/common.h
> > ==
> > --- head/tests/sys/kqueue/libkqueue/common.hSat Apr 18 12:54:40 2020
> > (r360072)
> > +++ head/tests/sys/kqueue/libkqueue/common.hSat Apr 18 12:54:47 2020
> > (r360073)
> > @@ -40,7 +40,6 @@
> >
> >  #include 
> >
> > -extern char *cur_test_id;
> >  extern int vnode_fd;
> >  extern int kqfd;
> >
> > @@ -76,5 +75,14 @@ extern void test_no_kevents_quietly(void);
> >
> >  extern void test_begin(const char *);
> >  extern void success(void);
> > +
> > +extern void test_evfilt_read(void);
> > +extern void test_evfilt_signal(void);
> > +extern void test_evfilt_vnode(void);
> > +extern void test_evfilt_timer(void);
> > +extern void test_evfilt_proc(void);
> > +#if HAVE_EVFILT_USER
> > +extern void test_evfilt_user(void);
> > +#endif
> >
> >  #endif  /* _COMMON_H */
> >
> > Modified: head/tests/sys/kqueue/libkqueue/main.c
> > ==
> > --- head/tests/sys/kqueue/libkqueue/main.c  Sat Apr 18 12:54:40 2020
> > (r360072)
> > +++ head/tests/sys/kqueue/libkqueue/main.c  Sat Apr 18 12:54:47 2020
> > (r360073)
> > @@ -21,21 +21,12 @@
> >  #include "config.h"
> >  #include "common.h"
> >
> > -int testnum = 1;
> > -char *cur_test_id = NULL;
> >  int kqfd;
> > +static char *cur_test_id = NULL;
> > +static int testnum = 1;
> >
> > -extern void test_evfilt_read();
> > -extern void test_evfilt_signal();
> > -extern void test_evfilt_vnode();
> > -extern void test_evfilt_timer();
> > -extern void test_evfilt_proc();
> > -#if HAVE_EVFILT_USER
> > -extern void test_evfilt_user();
> > -#endif
> > -
> >  /* Checks if any events are pending, which is an error. */
> > -void
> > +void
> >  test_no_kevents(void)
> >  {
> >  int nfds;
> > @@ -58,7 +49,7 @@ test_no_kevents(void)
> >  /* Checks if any events are pending, which is an error. Do not print
> >   * out anything unless events are found.
> >  */
> > -void
> > +void
> >  test_no_kevents_quietly(void)
> >  {
> >  int nfds;
> > @@ -79,7 +70,7 @@ test_no_kevents_quietly(void)
> >
> >  /* Retrieve a single kevent */
> >  struct kevent *
> > -kevent_get(int kqfd)
> > +kevent_get(int fd)
> >  {
> >  int nfds;
> >  struct kevent *kev;
> > @@ -87,7 +78,7 @@ kevent_get(int kqfd)
> >  if ((kev = calloc(1, sizeof(*kev))) == NULL)
> >  err(1, "out of memory");
> >
> > -nfds = kevent(kqfd, NULL, 0, kev, 1, NULL);
> > +nfds = kevent(fd, NULL, 0, kev, 1, NULL);
> >  if (nfds < 1)
> >  err(1, "kevent(2)");
> >
> > @@ -96,7 +87,7 @@ kevent_get(int kqfd)
> >
> >  /* Retrieve a single kevent, specifying a maximum time to wait for it. */
> >  struct kevent *
> > -kevent_get_timeout(int kqfd, int seconds)
> > +kevent_get_timeout(int fd, int seconds)
> >  {
> >  int nfds;
> >  struct kevent *kev;
> > @@ -105,7 +96,7 @@ kevent_get_timeout(int kqfd, int seconds)
> >  if ((kev = calloc(1, sizeof(*kev))) == NULL)
> >  err(1, "out of memory");
> >
> > -nfds = kevent(kqfd, NULL, 0, kev, 1, );
> > +nfds = kevent(fd, NULL, 0, kev, 1, );
> >  if (nfds < 0) {
> >  err(1, "kevent(2)");
> >  } else if (nfds == 0) {
> > @@ 

Re: svn commit: r339636 - in head: . share/mk

[Alexander Richardson]

Hi Markiyan,


This looks exactly like the SYSTEM_COMPILER/SYSTEM_LINKER build failure
that should have been fixed by r340167.
Does passing BUILD_WITH_STRICT_TMPPATH=0 on the make command line fix the
build error? If so it seems like I missed one more SYSTEM_COMPILER case.

Could you send me your buildworld command+environment variables so I can
see what's going wrong?

Thanks,
Alex


On Tue, 6 Nov 2018 at 17:12 Markiyan Kushnir 
wrote:

> Alexander,
>
> Cannot tell exactly what change it was, might be one of your recent
> changes to bsd.compiler.mk or Makefile.inc1? ...
>
> Now running "make buildworld" I can proceed with "3.1: recording build
> metadata" only having USING_SYSTEM_COMPILER=yes USING_SYSTEM_LINKER=yes set
> in my environment. Otherwise I'm getting this:
>
> sh: cc: not found
> make[2]: "/work/src.svn/share/mk/bsd.compiler.mk" line 176: Unable to
> determine compiler type for CC=cc -target x86_64-unknown-freebsd13.0
> --sysroot=//usr/obj/work/src.svn/amd64.amd64/tmp
> -B//usr/obj/work/src.svn/amd64.amd64/tmp/usr/bin.  Consider setting
> COMPILER_TYPE.
> *** Error code 1
>
> Stop.
> make[1]: stopped in /work/src.svn
> *** Error code 1
>
> Stop.
> make: stopped in /work/src.svn
> Failed to build world
>
> I'm at rev. 340189. Could you please have a look?
>
> --
> Markiyan
>
> вт, 6 лист. 2018 о 01:11 Alexander Richardson 
> пише:
>
>> On Mon, 5 Nov 2018 at 23:00, Bryan Drewery  wrote:
>> >
>> > On 10/22/2018 11:31 PM, Alex Richardson wrote:
>> > > Author: arichardson
>> > > Date: Tue Oct 23 06:31:25 2018
>> > > New Revision: 339636
>> > > URL: https://svnweb.freebsd.org/changeset/base/339636
>> > >
>> > > Log:
>> > >   Only compute the X_COMPILER_*/X_LINKER_* variables when needed
>> > >
>> > >   When building CheriBSD we have to set XLD/XCC/XCFLAGS on the
>> command line.
>> > >   This triggers the $XCC != $CC case in bsd.compiler.mk (and the
>> same for LD
>> > >   in bsd.linker.mk) which causes it to call ${XCC} --version and
>> > >   ${XLD} --version (plus various awk+sed+echo calls) in every
>> subdirectory.
>> > >   For incremental builds and stages that only walk the source tree
>> this is
>> > >   often the majority of the time spent in that directory.
>> > >
>> > ...
>> >
>> > >   By only computing the value of the X_COMPILER_*/X_LINKER_*
>> variables if
>> > >   _WANT_TOOLCHAIN_CROSS_VARS is set we can reduce the number of cc/ld
>> calls
>> > >   to once per build stage instead of once per recursive make.
>> >
>> > This sounds wrong. bsd.compiler.mk *already* handles that kind of thing
>> > by exporting its computations. Adding a second hack for a similar
>> > problem isn't the right solution.
>> >
>>
>> I added debug .info statements for every time bsd.compiler.mk was
>> running cc --version and it was always running it for $XCC during make
>> buildworld.
>> As far as I can tell this happens because $CC and $XCC no longer match.
>> I believe the problem is that the toplevel makefile runs the cross
>> stages with CC="${XCC} ${XCFLAGS}" so if you have XCFLAGS set in your
>> environment $CC will never be equal to $XCC.
>> This also means there is not cached variable for $XCC since it was
>> computed for "${XCC} ${XCFLAGS}" rather than $XCC .
>>
>> This patch massively reduced the incremental build time for CheriBSD
>> but if you can suggest I better fix that would be great.
>>
>> Alex
>>
> ___
>> svn-src-...@freebsd.org mailing list
>> https://lists.freebsd.org/mailman/listinfo/svn-src-all
>> To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
>>
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r340157 - in head: . tools/build tools/tools/nanobsd/embedded

[Alexander Richardson]

On Tue, 6 Nov 2018 at 07:10, Michal Meloun  wrote:
>
>
>
> On 05.11.2018 20:51, Alex Richardson wrote:
> > Author: arichardson
> > Date: Mon Nov  5 19:51:10 2018
> > New Revision: 340157
> > URL: https://svnweb.freebsd.org/changeset/base/340157
> >
> > Log:
> >   Allow building world without inheriting $PATH
> >
> >   Inheriting $PATH during the build phase can cause the build to fail when
> >   compiling on a different system due to missing build tools or incompatible
> >   versions somewhere in $PATH. This has cause build failures for us before
> >   due to the jenkins slaves still running FreeBSD 10.
> >   Listing the tools we depend on explicitly instead of just using whatever
> >   happens to be in $PATH allows us to check that we don't accidentally add a
> >   new build dependency.
> >
> >   All tools that do no need to be bootstrapped will now be symlinked to
> >   ${WORLDTMP}/legacy/bin and during the build phase $PATH will only contain
> >   ${WORLDTMP}. There is also a new variable "BOOTSTRAP_ALL_TOOLS" which can
> >   be set to force compiling almost all bootstrap tools instead of symlinking
> >   them. This will not bootstrap tools such as cp,mv, etc. since they may be
> >   used during the build and for those we should really only be using POSIX
> >   compatible options.
> >
> >   Furthermore, this change is required in order to be able to build on
> >   non-FreeBSD hosts. While the same binaries may exist on Linux/MacOS they
> >   often accept different flags or produce incompatible output.
> >
> >   Approved By:brooks (mentor)
> >   Differential Revision: https://reviews.freebsd.org/D16815
> >
> > Modified:
> >   head/Makefile
> >   head/Makefile.inc1
> >   head/tools/build/Makefile
> >   head/tools/tools/nanobsd/embedded/common
> >
> > Modified: head/Makefile
> > ==
> > --- head/Makefile Mon Nov  5 19:25:57 2018(r340156)
> > +++ head/Makefile Mon Nov  5 19:51:10 2018(r340157)
> > @@ -610,10 +610,13 @@ _need_lld_${target}_${target_arch} != \
> >  # XXX: Passing HOST_OBJTOP into the PATH would allow skipping legacy,
> >  #  bootstrap-tools, and cross-tools.  Need to ensure each tool actually
> >  #  supports all TARGETS though.
> > +# For now we only pass UNIVERSE_TOOLCHAIN_PATH which will be added at the 
> > end
> > +# of STRICTTMPPATH to ensure that the target-specific binaries come first.
> >  MAKE_PARAMS_${target}+= \
> >   XCC="${HOST_OBJTOP}/tmp/usr/bin/cc" \
> >   XCXX="${HOST_OBJTOP}/tmp/usr/bin/c++" \
> > - XCPP="${HOST_OBJTOP}/tmp/usr/bin/cpp"
> > + XCPP="${HOST_OBJTOP}/tmp/usr/bin/cpp" \
> > + UNIVERSE_TOOLCHAIN_PATH=${HOST_OBJTOP}/tmp/usr/bin
> >  .endif
> >  .if defined(_need_lld_${target}_${target_arch}) && \
> >  ${_need_lld_${target}_${target_arch}} == "yes"
> >
> > Modified: head/Makefile.inc1
> > ==
> > --- head/Makefile.inc1Mon Nov  5 19:25:57 2018(r340156)
> > +++ head/Makefile.inc1Mon Nov  5 19:51:10 2018(r340157)
> > @@ -580,8 +580,21 @@ BUILD_ARCH!= uname -p
> >  WORLDTMP?=   ${OBJTOP}/tmp
> >  BPATH=   
> > ${CCACHE_WRAPPER_PATH_PFX}${WORLDTMP}/legacy/usr/sbin:${WORLDTMP}/legacy/usr/bin:${WORLDTMP}/legacy/bin
> >  XPATH=   ${WORLDTMP}/usr/sbin:${WORLDTMP}/usr/bin
> > -STRICTTMPPATH=   ${BPATH}:${XPATH}
> > +
> > +# When building we want to find the cross tools before the host tools in 
> > ${BPATH}.
> > +# We also need to add UNIVERSE_TOOLCHAIN_PATH so that we can find the 
> > shared
> > +# toolchain files (clang, lld, etc.) during make universe/tinderbox
> > +STRICTTMPPATH=   ${XPATH}:${BPATH}:${UNIVERSE_TOOLCHAIN_PATH}
> > +# We should not be using tools from /usr/bin accidentally since this could 
> > cause
> > +# the build to break on other systems that don't have that tool. For now we
> > +# still allow using the old behaviour (inheriting $PATH) if
> > +# BUILD_WITH_STRICT_TMPPATH is set to 0 but this will eventually be 
> > removed.
> > +BUILD_WITH_STRICT_TMPPATH?=1
> > +.if ${BUILD_WITH_STRICT_TMPPATH} != 0
> > +TMPPATH= ${STRICTTMPPATH}
> > +.else
> >  TMPPATH= ${STRICTTMPPATH}:${PATH}
> > +.endif
> >
> >  #
> >  # Avoid running mktemp(1) unless actually needed.
> > @@ -589,8 +602,16 @@ TMPPATH= ${STRICTTMPPATH}:${PATH}
> >  # when in the middle of installing over this system.
> >  #
> >  .if make(distributeworld) || make(installworld) || make(stageworld)
> > -INSTALLTMP!= mktemp -d -u -t install
> > +.if ${BUILD_WITH_STRICT_TMPPATH} != 0
> > +MKTEMP=${WORLDTMP}/legacy/usr/bin/mktemp
> > +.if !exists(${MKTEMP})
> > +.error "mktemp binary doesn't exist in expected location: ${MKTEMP}"
> >  .endif
> > +.else
> > +MKTEMP=mktemp
> > +.endif
> > +INSTALLTMP!= ${MKTEMP} -d -u -t install
> > +.endif
> >
> >  .if make(stagekernel) || make(distributekernel)
> >  TAGS+= 

Re: svn commit: r340157 - in head: . tools/build tools/tools/nanobsd/embedded

[Alexander Richardson]

On Mon, 5 Nov 2018, 23:32 Bryan Drewery  On 11/5/2018 11:51 AM, Alex Richardson wrote:
> > Author: arichardson
> > Date: Mon Nov  5 19:51:10 2018
> > New Revision: 340157
> > URL: https://svnweb.freebsd.org/changeset/base/340157
> >
> > Log:
> >   Allow building world without inheriting $PATH
> >
>
> This change has a summary that doesn't seem to match its change (to
> build host tools for cross-os builds).
> Does $PATH go into the build still or not? SYSTEM_COMPILER relies on
> $PATH inheriting into the build.


Bootstrapping of host tools does not change unless BOOTSTRAP_ALL_TOOLS is
set on the make command line (this will be the default for cross-os builds
but that still requires more changes). This patch only ensures that the
required tools are symlinked into worldtmp.

$PATH should only be inherited for the bootstrap-tools phase. I thought I
had tested all variations of universe builds but it looks like I forgot to
test on a SYSTEM_COMPILER machine so didn't notice it is needed there. This
should be fixed in r340167.
$PATH will now be inherited when using the system compiler or linker but
otherwise the strict path will be used.

In case I missed more cases: the new strict path behaviour can be disabled
by setting BUILD_WITH_STRICT_TMPPATH=0.

Sorry about any breakage.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r338268 - head

[Alexander Richardson]

On Mon, 5 Nov 2018 at 23:03, Bryan Drewery  wrote:
>
> On 8/23/2018 11:19 AM, Alex Richardson wrote:
> > Author: arichardson
> > Date: Thu Aug 23 18:19:10 2018
> > New Revision: 338268
> > URL: https://svnweb.freebsd.org/changeset/base/338268
> >
> > Log:
> >   Fix non-FreeBSD host lib32 build for TARGET=amd64
> >
> >   When building on non-FreeBSD systems we need to pass an explicit target
> >   triple to clang otherwise it will attempt to build with the host triple.
> >   This also has advantages when building on a FreeBSD host: we now tell
> >   clang that we are targeting at least FreeBSD 12.0 instead of an older
> >   version so it can enable newer features.
> >
> >   Reviewed By:brooks (mentor)
> >   Approved By:jhb (mentor)
> >   Differential Revision: https://reviews.freebsd.org/D16842
> >
> > Modified:
> >   head/Makefile.libcompat
> >
> > Modified: head/Makefile.libcompat
> > ==
> > --- head/Makefile.libcompat   Thu Aug 23 18:19:01 2018(r338267)
> > +++ head/Makefile.libcompat   Thu Aug 23 18:19:10 2018(r338268)
> > @@ -14,6 +14,11 @@ LIB32CPUFLAGS= -march=i686 -mmmx -msse -msse2
> >  .else
> >  LIB32CPUFLAGS=   -march=${TARGET_CPUTYPE}
> >  .endif
> > +.if ${WANT_COMPILER_TYPE} == gcc || \
> > +(defined(X_COMPILER_TYPE) && ${X_COMPILER_TYPE} == gcc)
> > +.else
> > +LIB32CPUFLAGS+=  -target x86_64-unknown-freebsd12.0
>
> This can be ${TARGET_TRIPLE}, no?
> It also seems like it should be in the LIBSOFT flags too; via
> LIBCOMPATCFLAGS.

Yes, that would be better. I didn't see that we already had a variable
with the target triple when I wrote this patch.
I can fix this tomorrow morning.

Alex
>
> > +.endif
> >  LIB32CPUFLAGS+=  -m32
> >  LIB32WMAKEENV=   MACHINE=i386 MACHINE_ARCH=i386 \
> >   MACHINE_CPU="i686 mmx sse sse2"
> >
>
>
> --
> Regards,
> Bryan Drewery
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r339636 - in head: . share/mk

[Alexander Richardson]

On Mon, 5 Nov 2018 at 23:00, Bryan Drewery  wrote:
>
> On 10/22/2018 11:31 PM, Alex Richardson wrote:
> > Author: arichardson
> > Date: Tue Oct 23 06:31:25 2018
> > New Revision: 339636
> > URL: https://svnweb.freebsd.org/changeset/base/339636
> >
> > Log:
> >   Only compute the X_COMPILER_*/X_LINKER_* variables when needed
> >
> >   When building CheriBSD we have to set XLD/XCC/XCFLAGS on the command line.
> >   This triggers the $XCC != $CC case in bsd.compiler.mk (and the same for LD
> >   in bsd.linker.mk) which causes it to call ${XCC} --version and
> >   ${XLD} --version (plus various awk+sed+echo calls) in every subdirectory.
> >   For incremental builds and stages that only walk the source tree this is
> >   often the majority of the time spent in that directory.
> >
> ...
>
> >   By only computing the value of the X_COMPILER_*/X_LINKER_* variables if
> >   _WANT_TOOLCHAIN_CROSS_VARS is set we can reduce the number of cc/ld calls
> >   to once per build stage instead of once per recursive make.
>
> This sounds wrong. bsd.compiler.mk *already* handles that kind of thing
> by exporting its computations. Adding a second hack for a similar
> problem isn't the right solution.
>

I added debug .info statements for every time bsd.compiler.mk was
running cc --version and it was always running it for $XCC during make
buildworld.
As far as I can tell this happens because $CC and $XCC no longer match.
I believe the problem is that the toplevel makefile runs the cross
stages with CC="${XCC} ${XCFLAGS}" so if you have XCFLAGS set in your
environment $CC will never be equal to $XCC.
This also means there is not cached variable for $XCC since it was
computed for "${XCC} ${XCFLAGS}" rather than $XCC .

This patch massively reduced the incremental build time for CheriBSD
but if you can suggest I better fix that would be great.

Alex
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r339876 - head/libexec/rtld-elf

[Alexander Richardson]

On Wed, 31 Oct 2018 at 18:38, Mark Millard  wrote:
>
> On 2018-Oct-30, at 3:23 PM, Alexander Richardson  
> wrote:
>
> > . . .
> > Before this change obj->textsize was always set as the end of
> > PT_LOAD[0]. Now it will contain everything up to the end of the last
> > PT_LOAD with execute permissions. In the binary you dumped this is
> > PT_LOAD[0] both before and after the patch so there is no change in
> > behaviour. The .got and .plt we not included in textsize before this
> > patch either. Therefore the only way I can see this patch breaking
> > anything is if the PHDRS of one of the loaded libraries causes a
> > change in behaviour.
>
> A fair night's sleep helps.
>
> Not only did I read something into your description that was
> not there, I looked at powerpc64 when it turns out that
> only 32-bit powerpc shows the problem. (I asked.)
>
> I'm also told reverting just those changes fixes 32-bit
> powerpc context.
>
> So I'm using an older 32-bit powerpc context to look at:
>
> https://artifact.ci.freebsd.org/snapshot/head/r339870/powerpc/powerpc/base*.txz
>
> materials now. -r339870 is the first 32-bit powerpc build
> available there after the changes were made.
>
> So in a /339870/ area from expanding the .txz's
> inside it I get:
>
> # elfdump -p ./bin/ls | less
>
> program header:
>
> entry: 0
> p_type: PT_PHDR
> p_offset: 52
> p_vaddr: 0x1800034
> p_paddr: 0x1800034
> p_filesz: 224
> p_memsz: 224
> p_flags: PF_X|PF_R
> p_align: 4
>
> entry: 1
> p_type: PT_INTERP
> p_offset: 276
> p_vaddr: 0x1800114
> p_paddr: 0x1800114
> p_filesz: 21
> p_memsz: 21
> p_flags: PF_R
> p_align: 1
>
> entry: 2
> p_type: PT_LOAD
> p_offset: 0
> p_vaddr: 0x180
> p_paddr: 0x180
> p_filesz: 34112
> p_memsz: 34112
> p_flags: PF_X|PF_R
> p_align: 65536
>
> entry: 3
> p_type: PT_LOAD
> p_offset: 34112
> p_vaddr: 0x1818540
> p_paddr: 0x1818540
> p_filesz: 316
> p_memsz: 1752
> p_flags: PF_X|PF_W|PF_R
> p_align: 65536
>
> entry: 4
> p_type: PT_DYNAMIC
> p_offset: 34132
> p_vaddr: 0x1818554
> p_paddr: 0x1818554
> p_filesz: 216
> p_memsz: 216
> p_flags: PF_W|PF_R
> p_align: 4
>
> entry: 5
> p_type: PT_NOTE
> p_offset: 300
> p_vaddr: 0x180012c
> p_paddr: 0x180012c
> p_filesz: 48
> p_memsz: 48
> p_flags: PF_R
> p_align: 4
>
> entry: 6
> p_type: PT_LOAD
> p_offset: 0
> p_vaddr: 0
> p_paddr: 0
> p_filesz: 0
> p_memsz: 0
> p_flags: PF_W|PF_R
> p_align: 4
>
> I note some things unique to this compared
> to powerpc64 and to what the old code would
> do for what is unique:
>
> There are 2 PT_LOADS with PF_X and there are a
> bunch of pages between that are not covered by
> any entry. This is from p_align being 65536 from
> what I can tell.
>
> entry 2:
> 0x180+34112==0x1808540 for the ending of the read-only PF_X area.
> entry 3:
> 0x1818540 for the beginning of the writable PF_X area.
> 0x001 differences: 65536 Bytes.
>
> What is the handling of the page range that is
> not described in any entry? Is
> __syncicache(obj->mapbase, obj->textsize)
> spanning the hole valid? Previously the
> hole was not spanned as I understand.
>
>
> Libraries also have the hole between the
> PT_LOAD with PF_X ranges. Using /lib/libc.so
> as an example:
>
> # elfdump -p ./lib/libc.so.7 | less
>
> program header:
>
> entry: 0
> p_type: PT_LOAD
> p_offset: 0
> p_vaddr: 0
> p_paddr: 0
> p_filesz: 1746472
> p_memsz: 1746472
> p_flags: PF_X|PF_R
> p_align: 65536
>
> entry: 1
> p_type: PT_LOAD
> p_offset: 1746480
> p_vaddr: 0x1ba630
> p_paddr: 0x1ba630
> p_filesz: 44188
> p_memsz: 201536
> p_flags: PF_X|PF_W|PF_R
> p_align: 65536
>
> entry: 2
> p_type: PT_DYNAMIC
> p_offset: 1762600
> p_vaddr: 0x1be528
> p_paddr: 0x1be528
> p_filesz: 192
> p_memsz: 192
> p_flags: PF_W|PF_R
> p_align: 4
>
> entry: 3
> p_type: PT_TLS
> p_offset: 1746480
> p_vaddr: 0x1ba

Re: svn commit: r339876 - head/libexec/rtld-elf

[Alexander Richardson]

On Tue, 30 Oct 2018 at 22:16, Mark Millard  wrote:
>
> On 2018-Oct-30, at 2:40 PM, Alexander Richardson  
> wrote:
>
> > On Tue, 30 Oct 2018 at 21:32, Mark Millard  
> > wrote:
> >>
> >>
> >>
> >> On 2018-Oct-30, at 2:23 PM, Alexander Richardson  >> freebsd.org> wrote:
> >>
> >>> On Tue, 30 Oct 2018 at 18:19, Mark Millard  
> >>> wrote:
> >>>>
> >>>> Alexander Richardson arichardson at freebsd.org wrote on
> >>>> Tue Oct 30 15:33:00 UTC 2018 :
> >>>>
> >>>>> On Tue, 30 Oct 2018 at 10:17, Michael Tuexen
> >>>>>  wrote:
> >>>>>>
> >>>>>>> On 29. Oct 2018, at 22:08, Alex Richardson  >>>>>>> FreeBSD.org> wrote:
> >>>>>>>
> >>>>>>> Author: arichardson
> >>>>>>> Date: Mon Oct 29 21:08:02 2018
> >>>>>>> New Revision: 339876
> >>>>>>> URL: https://svnweb.freebsd.org/changeset/base/339876
> >>>>>>>
> >>>>>>> Log:
> >>>>>>> rtld: set obj->textsize correctly
> >>>>>>>
> >>>>>>> With lld-generated binaries the first PT_LOAD will usually be a 
> >>>>>>> read-only
> >>>>>>> segment unless you pass --no-rosegment. For those binaries the 
> >>>>>>> textsize is
> >>>>>>> determined by the next PT_LOAD. To allow both LLD and bfd 2.17 
> >>>>>>> binaries to
> >>>>>>> be parsed correctly use the end of the last PT_LOAD that is marked as
> >>>>>>> executable instead.
> >>>>>>>
> >>>>>>> I noticed that the value was wrong while adding some debug prints for 
> >>>>>>> some rtld
> >>>>>>> changes for CHERI binaries. `obj->textsize` only seems to be used by 
> >>>>>>> PPC so the
> >>>>>>> effect is untested. However, the value before was definitely wrong 
> >>>>>>> and the new
> >>>>>>> result matches the phdrs.
> >>>>>> I build kernel and world with a revision later than this on a PPC. 
> >>>>>> Buildword
> >>>>>> ends up with a world where almost all binaries are segfaulting 
> >>>>>> Especially gdb
> >>>>>> (but svn, ls or so all segfault).
> >>>>>>
> >>>>>> Best regards
> >>>>>> Michael
> >>>>>
> >>>>> This is rather surprising since if anything the range of the icache
> >>>>> flush should increase rather than decrease after this change.
> >>>>>
> >>>>> I can only see this causing a behaviour change if we actually need to
> >>>>> flush more than just the executable segments.
> >>>>> Is it possible that some binary/library contains a non-executable
> >>>>> segment as the first PT_LOAD?
> >>>>> Or is there some linker script that adds custom PHDRS?
> >>>>
> >>>> The following is based on using devel/powerpc64-xtoolchain-gcc
> >>>> to buildworld buildkernel on/for powerpc64. (I experiment with
> >>>> using fairly modern tools to target powerpc64 and powerpc.)
> >>>> The build context is head -r339076 based, both for what
> >>>> did the build and for what it was building.
> >>>>
> >>>> I report from both elfdump and objdump output
> >>>> because each seems to have some oddities in what
> >>>> it outputs.
> >>>>
> >>>> I start with elfdump (which leaves sh_flags blank
> >>>> and shows a section header with sh_name empty
> >>>> that objdump does not list at all):
> >>>>
> >>>> # elfdump -pc /bin/ls | less
> >>>>
> >>>> . . .
> >>>>
> >>>> As for objdump on the same file (section
> >>>> one less than elfdump listed, no empty sh_name
> >>>> section listed):
> >>>>
> >>>> # objdump -ph /bin/ls | less
> >>>>
> >>>> /bin/ls: file format elf64-powerpc-freebsd
> >>>>
> >>>> Program Header:
> >>>>   

Re: svn commit: r339876 - head/libexec/rtld-elf

[Alexander Richardson]

On Tue, 30 Oct 2018 at 21:32, Mark Millard  wrote:
>
>
>
> On 2018-Oct-30, at 2:23 PM, Alexander Richardson  
> wrote:
>
> > On Tue, 30 Oct 2018 at 18:19, Mark Millard  
> > wrote:
> >>
> >> Alexander Richardson arichardson at freebsd.org wrote on
> >> Tue Oct 30 15:33:00 UTC 2018 :
> >>
> >>> On Tue, 30 Oct 2018 at 10:17, Michael Tuexen
> >>>  wrote:
> >>>>
> >>>>> On 29. Oct 2018, at 22:08, Alex Richardson  
> >>>>> wrote:
> >>>>>
> >>>>> Author: arichardson
> >>>>> Date: Mon Oct 29 21:08:02 2018
> >>>>> New Revision: 339876
> >>>>> URL: https://svnweb.freebsd.org/changeset/base/339876
> >>>>>
> >>>>> Log:
> >>>>> rtld: set obj->textsize correctly
> >>>>>
> >>>>> With lld-generated binaries the first PT_LOAD will usually be a 
> >>>>> read-only
> >>>>> segment unless you pass --no-rosegment. For those binaries the textsize 
> >>>>> is
> >>>>> determined by the next PT_LOAD. To allow both LLD and bfd 2.17 binaries 
> >>>>> to
> >>>>> be parsed correctly use the end of the last PT_LOAD that is marked as
> >>>>> executable instead.
> >>>>>
> >>>>> I noticed that the value was wrong while adding some debug prints for 
> >>>>> some rtld
> >>>>> changes for CHERI binaries. `obj->textsize` only seems to be used by 
> >>>>> PPC so the
> >>>>> effect is untested. However, the value before was definitely wrong and 
> >>>>> the new
> >>>>> result matches the phdrs.
> >>>> I build kernel and world with a revision later than this on a PPC. 
> >>>> Buildword
> >>>> ends up with a world where almost all binaries are segfaulting 
> >>>> Especially gdb
> >>>> (but svn, ls or so all segfault).
> >>>>
> >>>> Best regards
> >>>> Michael
> >>>
> >>> This is rather surprising since if anything the range of the icache
> >>> flush should increase rather than decrease after this change.
> >>>
> >>> I can only see this causing a behaviour change if we actually need to
> >>> flush more than just the executable segments.
> >>> Is it possible that some binary/library contains a non-executable
> >>> segment as the first PT_LOAD?
> >>> Or is there some linker script that adds custom PHDRS?
> >>
> >> The following is based on using devel/powerpc64-xtoolchain-gcc
> >> to buildworld buildkernel on/for powerpc64. (I experiment with
> >> using fairly modern tools to target powerpc64 and powerpc.)
> >> The build context is head -r339076 based, both for what
> >> did the build and for what it was building.
> >>
> >> I report from both elfdump and objdump output
> >> because each seems to have some oddities in what
> >> it outputs.
> >>
> >> I start with elfdump (which leaves sh_flags blank
> >> and shows a section header with sh_name empty
> >> that objdump does not list at all):
> >>
> >> # elfdump -pc /bin/ls | less
> >>
> >> . . .
> >>
> >> As for objdump on the same file (section
> >> one less than elfdump listed, no empty sh_name
> >> section listed):
> >>
> >> # objdump -ph /bin/ls | less
> >>
> >> /bin/ls: file format elf64-powerpc-freebsd
> >>
> >> Program Header:
> >>PHDR off0x0040 vaddr 0x1040 paddr 
> >> 0x1040 align 2**3
> >> filesz 0x0188 memsz 0x0188 flags r--
> >>  INTERP off0x01c8 vaddr 0x11c8 paddr 
> >> 0x11c8 align 2**0
> >> filesz 0x0015 memsz 0x0015 flags r--
> >>LOAD off0x vaddr 0x1000 paddr 
> >> 0x1000 align 2**16
> >> filesz 0x910c memsz 0x910c flags r-x
> >>LOAD off0x9110 vaddr 0x10019110 paddr 
> >> 0x10019110 align 2**16
> >> filesz 0x0ee0 memsz 0x10e8 flags rw-
> >> DYNAMIC off0x9138 vaddr 0x1001913

Re: svn commit: r339876 - head/libexec/rtld-elf

[Alexander Richardson]

On Tue, 30 Oct 2018 at 18:19, Mark Millard  wrote:
>
> Alexander Richardson arichardson at freebsd.org wrote on
> Tue Oct 30 15:33:00 UTC 2018 :
>
> > On Tue, 30 Oct 2018 at 10:17, Michael Tuexen
> >  wrote:
> > >
> > > > On 29. Oct 2018, at 22:08, Alex Richardson  
> > > > wrote:
> > > >
> > > > Author: arichardson
> > > > Date: Mon Oct 29 21:08:02 2018
> > > > New Revision: 339876
> > > > URL: https://svnweb.freebsd.org/changeset/base/339876
> > > >
> > > > Log:
> > > >  rtld: set obj->textsize correctly
> > > >
> > > >  With lld-generated binaries the first PT_LOAD will usually be a 
> > > > read-only
> > > >  segment unless you pass --no-rosegment. For those binaries the 
> > > > textsize is
> > > >  determined by the next PT_LOAD. To allow both LLD and bfd 2.17 
> > > > binaries to
> > > >  be parsed correctly use the end of the last PT_LOAD that is marked as
> > > >  executable instead.
> > > >
> > > >  I noticed that the value was wrong while adding some debug prints for 
> > > > some rtld
> > > >  changes for CHERI binaries. `obj->textsize` only seems to be used by 
> > > > PPC so the
> > > >  effect is untested. However, the value before was definitely wrong and 
> > > > the new
> > > >  result matches the phdrs.
> > > I build kernel and world with a revision later than this on a PPC. 
> > > Buildword
> > > ends up with a world where almost all binaries are segfaulting 
> > > Especially gdb
> > > (but svn, ls or so all segfault).
> > >
> > > Best regards
> > > Michael
> >
> > This is rather surprising since if anything the range of the icache
> > flush should increase rather than decrease after this change.
> >
> > I can only see this causing a behaviour change if we actually need to
> > flush more than just the executable segments.
> > Is it possible that some binary/library contains a non-executable
> > segment as the first PT_LOAD?
> > Or is there some linker script that adds custom PHDRS?
>
> The following is based on using devel/powerpc64-xtoolchain-gcc
> to buildworld buildkernel on/for powerpc64. (I experiment with
> using fairly modern tools to target powerpc64 and powerpc.)
> The build context is head -r339076 based, both for what
> did the build and for what it was building.
>
> I report from both elfdump and objdump output
> because each seems to have some oddities in what
> it outputs.
>
> I start with elfdump (which leaves sh_flags blank
> and shows a section header with sh_name empty
> that objdump does not list at all):
>
> # elfdump -pc /bin/ls | less
>
> program header:
>
> entry: 0
> p_type: PT_PHDR
> p_offset: 64
> p_vaddr: 0x1040
> p_paddr: 0x1040
> p_filesz: 392
> p_memsz: 392
> p_flags: PF_R
> p_align: 8
>
> entry: 1
> p_type: PT_INTERP
> p_offset: 456
> p_vaddr: 0x11c8
> p_paddr: 0x11c8
> p_filesz: 21
> p_memsz: 21
> p_flags: PF_R
> p_align: 1
>
> entry: 2
> p_type: PT_LOAD
> p_offset: 0
> p_vaddr: 0x1000
> p_paddr: 0x1000
> p_filesz: 37132
> p_memsz: 37132
> p_flags: PF_X|PF_R
> p_align: 65536
>
> entry: 3
> p_type: PT_LOAD
> p_offset: 37136
> p_vaddr: 0x10019110
> p_paddr: 0x10019110
> p_filesz: 3808
> p_memsz: 4328
> p_flags: PF_W|PF_R
> p_align: 65536
>
> entry: 4
> p_type: PT_DYNAMIC
> p_offset: 37176
> p_vaddr: 0x10019138
> p_paddr: 0x10019138
> p_filesz: 448
> p_memsz: 448
> p_flags: PF_W|PF_R
> p_align: 8
>
> entry: 5
> p_type: PT_NOTE
> p_offset: 480
> p_vaddr: 0x11e0
> p_paddr: 0x11e0
> p_filesz: 48
> p_memsz: 48
> p_flags: PF_R
> p_align: 4
>
> entry: 6
> p_type: PT_LOAD
> p_offset: 0
> p_vaddr: 0
> p_paddr: 0
> p_filesz: 0
> p_memsz: 0
> p_flags: PF_W|PF_R
> p_align: 16
>
> section header:
>
> entry: 0
> sh_name:
> sh_type: SHT_NULL
> sh_flags:
> sh_addr: 0
> sh_offset: 0
>   

Re: svn commit: r339876 - head/libexec/rtld-elf

[Alexander Richardson]

On Tue, 30 Oct 2018 at 10:17, Michael Tuexen
 wrote:
>
> > On 29. Oct 2018, at 22:08, Alex Richardson  wrote:
> >
> > Author: arichardson
> > Date: Mon Oct 29 21:08:02 2018
> > New Revision: 339876
> > URL: https://svnweb.freebsd.org/changeset/base/339876
> >
> > Log:
> >  rtld: set obj->textsize correctly
> >
> >  With lld-generated binaries the first PT_LOAD will usually be a read-only
> >  segment unless you pass --no-rosegment. For those binaries the textsize is
> >  determined by the next PT_LOAD. To allow both LLD and bfd 2.17 binaries to
> >  be parsed correctly use the end of the last PT_LOAD that is marked as
> >  executable instead.
> >
> >  I noticed that the value was wrong while adding some debug prints for some 
> > rtld
> >  changes for CHERI binaries. `obj->textsize` only seems to be used by PPC 
> > so the
> >  effect is untested. However, the value before was definitely wrong and the 
> > new
> >  result matches the phdrs.
> I build kernel and world with a revision later than this on a PPC. Buildword
> ends up with a world where almost all binaries are segfaulting Especially 
> gdb
> (but svn, ls or so all segfault).
>
> Best regards
> Michael

This is rather surprising since if anything the range of the icache
flush should increase rather than decrease after this change.

I can only see this causing a behaviour change if we actually need to
flush more than just the executable segments.
Is it possible that some binary/library contains a non-executable
segment as the first PT_LOAD?
Or is there some linker script that adds custom PHDRS?

Alex


> >
> >  Reviewed By: kib
> >  Approved By: brooks (mentor)
> >  Differential Revision: https://reviews.freebsd.org/D17117
> >
> > Modified:
> >  head/libexec/rtld-elf/map_object.c
> >  head/libexec/rtld-elf/rtld.c
> >
> > Modified: head/libexec/rtld-elf/map_object.c
> > ==
> > --- head/libexec/rtld-elf/map_object.cMon Oct 29 21:03:43 2018  
> >   (r339875)
> > +++ head/libexec/rtld-elf/map_object.cMon Oct 29 21:08:02 2018  
> >   (r339876)
> > @@ -93,6 +93,7 @@ map_object(int fd, const char *path, const struct stat
> > Elf_Addr note_end;
> > char *note_map;
> > size_t note_map_len;
> > +Elf_Addr text_end;
> >
> > hdr = get_elf_header(fd, path, sb);
> > if (hdr == NULL)
> > @@ -116,6 +117,7 @@ map_object(int fd, const char *path, const struct stat
> > note_map = NULL;
> > segs = alloca(sizeof(segs[0]) * hdr->e_phnum);
> > stack_flags = RTLD_DEFAULT_STACK_PF_EXEC | PF_R | PF_W;
> > +text_end = 0;
> > while (phdr < phlimit) {
> >   switch (phdr->p_type) {
> >
> > @@ -130,6 +132,10 @@ map_object(int fd, const char *path, const struct stat
> >   path, nsegs);
> >   goto error;
> >   }
> > + if ((segs[nsegs]->p_flags & PF_X) == PF_X) {
> > + text_end = MAX(text_end,
> > + round_page(segs[nsegs]->p_vaddr + segs[nsegs]->p_memsz));
> > + }
> >   break;
> >
> >   case PT_PHDR:
> > @@ -280,8 +286,7 @@ map_object(int fd, const char *path, const struct stat
> > }
> > obj->mapbase = mapbase;
> > obj->mapsize = mapsize;
> > -obj->textsize = round_page(segs[0]->p_vaddr + segs[0]->p_memsz) -
> > -  base_vaddr;
> > +obj->textsize = text_end - base_vaddr;
> > obj->vaddrbase = base_vaddr;
> > obj->relocbase = mapbase - base_vaddr;
> > obj->dynamic = (const Elf_Dyn *) (obj->relocbase + phdyn->p_vaddr);
> >
> > Modified: head/libexec/rtld-elf/rtld.c
> > ==
> > --- head/libexec/rtld-elf/rtld.c  Mon Oct 29 21:03:43 2018
> > (r339875)
> > +++ head/libexec/rtld-elf/rtld.c  Mon Oct 29 21:08:02 2018
> > (r339876)
> > @@ -1390,13 +1390,15 @@ digest_phdr(const Elf_Phdr *phdr, int phnum, 
> > caddr_t e
> >   if (nsegs == 0) {   /* First load segment */
> >   obj->vaddrbase = trunc_page(ph->p_vaddr);
> >   obj->mapbase = obj->vaddrbase + obj->relocbase;
> > - obj->textsize = round_page(ph->p_vaddr + ph->p_memsz) -
> > -   obj->vaddrbase;
> >   } else {/* Last load segment */
> >   obj->mapsize = round_page(ph->p_vaddr + ph->p_memsz) -
> > obj->vaddrbase;
> >   }
> >   nsegs++;
> > + if ((ph->p_flags & PF_X) == PF_X) {
> > + obj->textsize = MAX(obj->textsize,
> > + round_page(ph->p_vaddr + ph->p_memsz) - obj->vaddrbase);
> > + }
> >   break;
> >
> >   case PT_DYNAMIC:
> >
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"

Re: svn commit: r338098 - in head: . tools/build

[Alexander Richardson]

On Wed, 29 Aug 2018 at 20:21 Cy Schubert  wrote:

> In message <201808201039.w7kadrmm079...@repo.freebsd.org>, Alex
> Richardson writ
> es:
> > Author: arichardson
> > Date: Mon Aug 20 10:39:53 2018
> > New Revision: 338098
> > URL: https://svnweb.freebsd.org/changeset/base/338098
> >
> > Log:
> >   Don't create directories in ${WORLDTMP}/legacy with mtree
> >
> >   This has two advantages:
> >   1) We no longer create lots of empty directories that are not needed
> >   2) This is a requirement for building on non-FreeBSD hosts since mtree
> will
> >   only exist after the bootstrap-tools phase there.
> >
> >   Aproved By: jhb (mentor)
> >   Differential Revision: https://reviews.freebsd.org/D16773
> >
> > Modified:
> >   head/Makefile.inc1
> >   head/tools/build/Makefile
> >
> > Modified: head/Makefile.inc1
> >
> =
> > =
> > --- head/Makefile.inc1Mon Aug 20 10:39:48 2018(r338097)
> > +++ head/Makefile.inc1Mon Aug 20 10:39:53 2018(r338098)
> > @@ -968,29 +968,10 @@ _worldtmp: .PHONY
> >  .endif   # !defined(NO_CLEAN)
> >   @mkdir -p ${WORLDTMP}
> >   @touch ${WORLDTMP}/${.TARGET}
> > -
> > -.for _dir in \
> > -lib lib/casper lib/geom usr legacy/bin legacy/usr
> > - mkdir -p ${WORLDTMP}/${_dir}
> > -.endfor
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.usr.dist \
> > - -p ${WORLDTMP}/legacy/usr >/dev/null
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.include.dist \
> > - -p ${WORLDTMP}/legacy/usr/include >/dev/null
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.usr.dist \
> > - -p ${WORLDTMP}/usr >/dev/null
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.include.dist \
> > - -p ${WORLDTMP}/usr/include >/dev/null
> > - ln -sf ${.CURDIR}/sys ${WORLDTMP}
> > -.if ${MK_DEBUG_FILES} != "no"
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.debug.dist \
> > - -p ${WORLDTMP}/legacy/usr/lib >/dev/null
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.debug.dist \
> > - -p ${WORLDTMP}/usr/lib >/dev/null
> > -.endif
> > -.for _mtree in ${LOCAL_MTREE}
> > - ${WORLDTMP_MTREE} -f ${.CURDIR}/${_mtree} -p ${WORLDTMP} >
> /dev/null
> > -.endfor
> > +# We can't use mtree to create the worldtmp directories since it may
> not be
> > +# available on the target system (this happens e.g. when building on
> non-Fre
> > eBSD)
> > + cd ${.CURDIR}/tools/build; \
> > + ${MAKE} DIRPRFX=tools/build/ DESTDIR=${WORLDTMP}/legacy
> installdirs
> >  _legacy:
> >   @echo
> >   @echo
> "--"
> > @@ -1003,6 +984,19 @@ _bootstrap-tools:
> >   @echo ">>> stage 1.2: bootstrap tools"
> >   @echo
> "--"
> >   ${_+_}cd ${.CURDIR}; ${BMAKE} bootstrap-tools
> > + mkdir -p ${WORLDTMP}/usr ${WORLDTMP}/lib/casper
> ${WORLDTMP}/lib/geom
> > + ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.usr.dist \
> > + -p ${WORLDTMP}/usr >/dev/null
> > + ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.include.dist \
> > + -p ${WORLDTMP}/usr/include >/dev/null
> > + ln -sf ${.CURDIR}/sys ${WORLDTMP}
> > +.if ${MK_DEBUG_FILES} != "no"
> > + ${WORLDTMP_MTREE} -f ${.CURDIR}/etc/mtree/BSD.debug.dist \
> > + -p ${WORLDTMP}/usr/lib >/dev/null
> > +.endif
> > +.for _mtree in ${LOCAL_MTREE}
> > + ${WORLDTMP_MTREE} -f ${.CURDIR}/${_mtree} -p ${WORLDTMP} >
> /dev/null
> > +.endfor
> >  _cleanobj:
> >  .if !defined(NO_CLEAN)
> >   @echo
> >
> > Modified: head/tools/build/Makefile
> >
> =
> > =
> > --- head/tools/build/Makefile Mon Aug 20 10:39:48 2018(r338097)
> > +++ head/tools/build/Makefile Mon Aug 20 10:39:53 2018(r338098)
> > @@ -59,4 +59,17 @@ SUBDIR=cross-build
> >  # Needed to build config (since it uses libnv)
> >  SYSINCS+=${SRCTOP}/sys/sys/nv.h ${SRCTOP}/sys/sys/cnv.h
> >
> > +
> > +# Create all the directories that are needed during the legacy,
> bootstrap-to
> > ols
> > +# and cross-tools stages. We do this here using mkdir since mtree may
> not ex
> > ist
> > +# yet (this happens if we are crossbuilding from Linux/Mac).
> > +installdirs:
> > +.for _dir in bin sbin usr/bin usr/sbin usr/lib usr/include lib/geom
> lib/casp
> > er
> > + mkdir -p "${DESTDIR}/${_dir}"
> > +.endfor
> > +
> > +.for _group in ${INCSGROUPS:NINCS}
> > + mkdir -p "${DESTDIR}/${${_group}DIR}"
> > +.endfor
> > +
> >  .include 
> >
>
> This broke my krb5 project branch. The fix for me is easy but rather
> inelegant, adding yet another for loop. Would it not be better to
> guarantee that mtree is in $WORLDTEMP for platforms that don't have it
> naturally or unconditionally? And, before we use it? With this change
> it requires, depending on what any future 

Re: svn commit: r328934 - in head: . bin/sh

[Alexander Richardson]

On 6 February 2018 at 19:25, Rodney W. Grimes
 wrote:
>> Author: arichardson
>> Date: Tue Feb  6 15:41:35 2018
>> New Revision: 328934
>> URL: https://svnweb.freebsd.org/changeset/base/328934
>>
>> Log:
>>   Don't hardcode /usr/bin as the path for mktemp in build tools
>>
>>   It won't work e.g. when crossbuilding from Ubuntu Linux as mktemp is in
>>   /bin there.
>>
>>   Reviewed By:bdrewery
>>   Approved By:jhb (mentor)
>>   Differential Revision: https://reviews.freebsd.org/D13937
>
> Would it be better to create the variable MKTEMP to point at
> either /bin/mktemp or /usr/bin/mktemp dependent on platform,
> there are reasons we use full paths in Makefiles, mostly to
> stop /usr/local/bin/foo contimaton, which I believe this
> change now opens up, though very slight as I dont know of
> a third party mktemp binary.
>
I'm happy to have a mktemp variable instead, but I don't believe files
in /usr/local/bin are a problem. When in add an echo $PATH to the
mktokens.sh file I get the following output:
PATH=/home/alr48/obj/build/freebsd-mips-build/exports/users/alr48/sources/freebsd-mips/mips.mips64/tmp/legacy/usr/sbin:/home/alr48/obj/build/freebsd-mips-build/exports/users/alr48/sources/freebsd-mips/mips.mips64/tmp/legacy/usr/bin:/home/alr48/obj/build/freebsd-mips-build/exports/users/alr48/sources/freebsd-mips/mips.mips64/tmp/legacy/bin:/home/alr48/obj/build/freebsd-mips-build/exports/users/alr48/sources/freebsd-mips/mips.mips64/tmp/usr/sbin:/home/alr48/obj/build/freebsd-mips-build/exports/users/alr48/sources/freebsd-mips/mips.mips64/tmp/usr/bin:/sbin:/bin:/usr/sbin:/usr/bin

This only contains WORLDTMP and /sbin:/bin:/usr/sbin:/usr/bin since
the toplevel makefile already sets PATH to
/sbin:/bin:/usr/sbin:/usr/bin.
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"