Re: svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Bruce Evans]

On Thu, 3 Aug 2017, Joerg Sonnenberger wrote:


On Thu, Aug 03, 2017 at 05:53:43PM +1000, Bruce Evans wrote:

Freestanding versions (static and otherwise) cause problems with builtins.
-ffreestanding turns off all builtins.  The static memcpy used to be
ifdefed so as to use __builtin_memcpy instead of the static one if the
compiler is gcc.  That apparently broke with gcc-4.2, since the builtin
will call libc memcpy() in some cases, although memcpy() is unavailable
in the freestanding case.


GCC always had a requirement that freestanding environment must provide
certain functions (memcpy, memmove, memset, memcmp). At least memcpy and
memset are regulary used for certain code constructs. While most calls
will be inlined, it is not required.


I had forgotten about that bug.  boot2 doesn't have many fancy code
constructs, but it might have struct copying and gcc uses memcpy for
copying large structs.  clang-4.0 has the same bug.  Testing showed
1 bug nearby: for newer CPUs, it generates large code with several
movups's for small struct copies.  The bug is that -Os doesn't turn
this off.  But boot2 uses -march=i386 which does turn this off.  For
not so new CPUs like core2 where movups is slower than movaps but
movaps is unusable because the structs might not be aligned, it
normally generates large code with moves through integer registers,
but -Os works to turn this off.

The change for gcc-4.2 was apparently to support this.  The static
memcpy is not quite compatible, but is close enough and you can check
this in the generated code.  The problem is that it should be re-checked
for every change in the source code and compiler.

Bruce
___
svn-src-all@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"

Re: svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Ngie Cooper (yaneurabeya)]

> On Aug 3, 2017, at 00:53, Bruce Evans  wrote:
> 
> On Thu, 3 Aug 2017, Ngie Cooper wrote:
> 
>> Log:
>> Fix the return types for printf and putchar to match their libc and
>> POSIX equivalents
>> 
>> Both printf and putchar return int, not void.
>> 
>> This will allow code that leverages the libcalls and checks/rely on the
>> return type to interchangeably between loader code and non-loader
>> code.
>> 
>> MFC after:   1 month
>> 
>> Modified:
>> head/sys/boot/arm/at91/libat91/lib.h
>> head/sys/boot/arm/at91/libat91/printf.c
>> head/sys/boot/arm/at91/libat91/putchar.c
>> head/sys/boot/arm/ixp425/boot2/ixp425_board.c
>> head/sys/boot/arm/ixp425/boot2/lib.h
>> head/sys/boot/i386/boot2/boot2.c
> 
> This is wrong for at least i386/boot2.  It isn't part of the loader, and
> saves space by not returning unused values.
> 
>> Modified: head/sys/boot/i386/boot2/boot2.c
>> ==
>> --- head/sys/boot/i386/boot2/boot2.c Thu Aug  3 03:45:48 2017
>> (r321968)
>> +++ head/sys/boot/i386/boot2/boot2.c Thu Aug  3 05:27:05 2017
>> (r321969)
>> @@ -114,8 +114,8 @@ void exit(int);
>> static void load(void);
>> static int parse(void);
>> static int dskread(void *, unsigned, unsigned);
>> -static void printf(const char *,...);
>> -static void putchar(int);
>> +static int printf(const char *,...);
>> +static int putchar(int);
> 
> These are freestanding static functions, so they have nothing to do
> with library functions except their name is a hint that they are
> similar.
> 
> Since they are static, -funit-at-a-time might allow the unused return values
> to be optimized away.  Then returning unused values would be just an
> obfuscation.
> 
> This file still has a static memcpy() which is quite different from the
> libc version.  It doesn't return an unused value, and its arg types are
> all different (no newfangled size_t or newerfangled restrict).
> 
> Freestanding versions (static and otherwise) cause problems with builtins.
> -ffreestanding turns off all builtins.  The static memcpy used to be
> ifdefed so as to use __builtin_memcpy instead of the static one if the
> compiler is gcc.  That apparently broke with gcc-4.2, since the builtin
> will call libc memcpy() in some cases, although memcpy() is unavailable
> in the freestanding case.

I get the point about them being freestanding functions, but if the functions 
aren’t meant to be compatible they should be named differently. Part of the 
issue some code that bridged loader and non-loader users (bootdevtest and 
zfsboottest for example) relied on feature parity (in part because the ZFS code 
required it and because of how things are compiled/linked together). If 
compilers get pedantic enough, then they’ll flag these issues as errors and 
we’ll have to address these compatibilities at that point.

My intent was ok (I think), but the implementation I did is wrong, so I’ll 
revert the change completely and leave it for someone else to deal with the 
incompatibilities (I’ll just integrate bootdevtest and zfsboottest into 
buildworld under sys/boot so they won’t remain broken for weeks on end, like 
they were recently).

Thanks,
-Ngie


signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Joerg Sonnenberger]

On Thu, Aug 03, 2017 at 05:53:43PM +1000, Bruce Evans wrote:
> Freestanding versions (static and otherwise) cause problems with builtins.
> -ffreestanding turns off all builtins.  The static memcpy used to be
> ifdefed so as to use __builtin_memcpy instead of the static one if the
> compiler is gcc.  That apparently broke with gcc-4.2, since the builtin
> will call libc memcpy() in some cases, although memcpy() is unavailable
> in the freestanding case.

GCC always had a requirement that freestanding environment must provide
certain functions (memcpy, memmove, memset, memcmp). At least memcpy and
memset are regulary used for certain code constructs. While most calls
will be inlined, it is not required.

Joerg
___
svn-src-all@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"

Re: svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Ngie Cooper (yaneurabeya)]

> On Aug 3, 2017, at 04:48, Ed Schouten  wrote:
> 
> 2017-08-03 7:27 GMT+02:00 Ngie Cooper :
>> Modified: head/sys/boot/arm/at91/libat91/printf.c
>> ==
>> --- head/sys/boot/arm/at91/libat91/printf.c Thu Aug  3 03:45:48 2017 
>>(r321968)
>> +++ head/sys/boot/arm/at91/libat91/printf.c Thu Aug  3 05:27:05 2017 
>>(r321969)
>> @@ -20,12 +20,13 @@
>> #include 
>> #include "lib.h"
>> 
>> -void
>> +int
>> printf(const char *fmt,...)
>> {
>>va_list ap;
>>const char *hex = "0123456789abcdef";
>>char buf[10];
>> +   const char *fmt_orig = fmt;
>>char *s;
>>unsigned u;
>>int c;
>> @@ -66,5 +67,5 @@ printf(const char *fmt,...)
>>}
>>va_end(ap);
>> 
>> -   return;
>> +   return (int)(fmt - fmt_orig);
>> }
> 
> This makes printf() return the number of characters from the format
> processed, right? This is different from libc's printf(), which
> returns the number of characters printed.

Yes. markj identified flaws with my approach that need to be addressed 
in another commit (unfortunately I didn’t pay close enough attention to the 
details when I implemented the change).
Thanks,
-Ngie



signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Ed Schouten]

2017-08-03 7:27 GMT+02:00 Ngie Cooper :
> Modified: head/sys/boot/arm/at91/libat91/printf.c
> ==
> --- head/sys/boot/arm/at91/libat91/printf.c Thu Aug  3 03:45:48 2017  
>   (r321968)
> +++ head/sys/boot/arm/at91/libat91/printf.c Thu Aug  3 05:27:05 2017  
>   (r321969)
> @@ -20,12 +20,13 @@
>  #include 
>  #include "lib.h"
>
> -void
> +int
>  printf(const char *fmt,...)
>  {
> va_list ap;
> const char *hex = "0123456789abcdef";
> char buf[10];
> +   const char *fmt_orig = fmt;
> char *s;
> unsigned u;
> int c;
> @@ -66,5 +67,5 @@ printf(const char *fmt,...)
> }
> va_end(ap);
>
> -   return;
> +   return (int)(fmt - fmt_orig);
>  }

This makes printf() return the number of characters from the format
processed, right? This is different from libc's printf(), which
returns the number of characters printed.

-- 
Ed Schouten 
Nuxi, 's-Hertogenbosch, the Netherlands
KvK-nr.: 62051717
___
svn-src-all@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"

Re: svn commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Bruce Evans]

On Thu, 3 Aug 2017, Ngie Cooper wrote:


Log:
 Fix the return types for printf and putchar to match their libc and
 POSIX equivalents

 Both printf and putchar return int, not void.

 This will allow code that leverages the libcalls and checks/rely on the
 return type to interchangeably between loader code and non-loader
 code.

 MFC after: 1 month

Modified:
 head/sys/boot/arm/at91/libat91/lib.h
 head/sys/boot/arm/at91/libat91/printf.c
 head/sys/boot/arm/at91/libat91/putchar.c
 head/sys/boot/arm/ixp425/boot2/ixp425_board.c
 head/sys/boot/arm/ixp425/boot2/lib.h
 head/sys/boot/i386/boot2/boot2.c


This is wrong for at least i386/boot2.  It isn't part of the loader, and
saves space by not returning unused values.


Modified: head/sys/boot/i386/boot2/boot2.c
==
--- head/sys/boot/i386/boot2/boot2.cThu Aug  3 03:45:48 2017
(r321968)
+++ head/sys/boot/i386/boot2/boot2.cThu Aug  3 05:27:05 2017
(r321969)
@@ -114,8 +114,8 @@ void exit(int);
static void load(void);
static int parse(void);
static int dskread(void *, unsigned, unsigned);
-static void printf(const char *,...);
-static void putchar(int);
+static int printf(const char *,...);
+static int putchar(int);


These are freestanding static functions, so they have nothing to do
with library functions except their name is a hint that they are
similar.

Since they are static, -funit-at-a-time might allow the unused return values
to be optimized away.  Then returning unused values would be just an
obfuscation.

This file still has a static memcpy() which is quite different from the
libc version.  It doesn't return an unused value, and its arg types are
all different (no newfangled size_t or newerfangled restrict).

Freestanding versions (static and otherwise) cause problems with builtins.
-ffreestanding turns off all builtins.  The static memcpy used to be
ifdefed so as to use __builtin_memcpy instead of the static one if the
compiler is gcc.  That apparently broke with gcc-4.2, since the builtin
will call libc memcpy() in some cases, although memcpy() is unavailable
in the freestanding case.

Bruce
___
svn-src-all@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 commit: r321969 - in head/sys/boot: arm/at91/libat91 arm/ixp425/boot2 i386/boot2

[Ngie Cooper]

Author: ngie
Date: Thu Aug  3 05:27:05 2017
New Revision: 321969
URL: https://svnweb.freebsd.org/changeset/base/321969

Log:
  Fix the return types for printf and putchar to match their libc and
  POSIX equivalents
  
  Both printf and putchar return int, not void.
  
  This will allow code that leverages the libcalls and checks/rely on the
  return type to interchangeably between loader code and non-loader
  code.
  
  MFC after:1 month

Modified:
  head/sys/boot/arm/at91/libat91/lib.h
  head/sys/boot/arm/at91/libat91/printf.c
  head/sys/boot/arm/at91/libat91/putchar.c
  head/sys/boot/arm/ixp425/boot2/ixp425_board.c
  head/sys/boot/arm/ixp425/boot2/lib.h
  head/sys/boot/i386/boot2/boot2.c

Modified: head/sys/boot/arm/at91/libat91/lib.h
==
--- head/sys/boot/arm/at91/libat91/lib.hThu Aug  3 03:45:48 2017
(r321968)
+++ head/sys/boot/arm/at91/libat91/lib.hThu Aug  3 05:27:05 2017
(r321969)
@@ -28,9 +28,9 @@
 #define ARM_BOOT_LIB_H
 
 int getc(int);
-void putchar(int);
-void xputchar(int);
-void printf(const char *fmt,...);
+int putchar(int);
+int xputchar(int);
+int printf(const char *fmt,...);
 
 /* The following function write eeprom at ee_addr using data   */
 /*  from data_add for size bytes.  */

Modified: head/sys/boot/arm/at91/libat91/printf.c
==
--- head/sys/boot/arm/at91/libat91/printf.c Thu Aug  3 03:45:48 2017
(r321968)
+++ head/sys/boot/arm/at91/libat91/printf.c Thu Aug  3 05:27:05 2017
(r321969)
@@ -20,12 +20,13 @@
 #include 
 #include "lib.h"
 
-void
+int
 printf(const char *fmt,...)
 {
va_list ap;
const char *hex = "0123456789abcdef";
char buf[10];
+   const char *fmt_orig = fmt;
char *s;
unsigned u;
int c;
@@ -66,5 +67,5 @@ printf(const char *fmt,...)
}
va_end(ap);
 
-   return;
+   return (int)(fmt - fmt_orig);
 }

Modified: head/sys/boot/arm/at91/libat91/putchar.c
==
--- head/sys/boot/arm/at91/libat91/putchar.cThu Aug  3 03:45:48 2017
(r321968)
+++ head/sys/boot/arm/at91/libat91/putchar.cThu Aug  3 05:27:05 2017
(r321969)
@@ -39,11 +39,11 @@
 #include "lib.h"
 
 /*
- * void putchar(int ch)
+ * int putchar(int ch)
  * Writes a character to the DBGU port.  It assumes that DBGU has
  * already been initialized.
  */
-void
+int
 putchar(int ch)
 {
AT91PS_USART pUSART = (AT91PS_USART)AT91C_BASE_DBGU;
@@ -51,12 +51,14 @@ putchar(int ch)
while (!(pUSART->US_CSR & AT91C_US_TXRDY))
continue;
pUSART->US_THR = (ch & 0xFF);
+   return (1);
 }
 
-void
+int
 xputchar(int ch)
 {
-if (ch == '\n')
-   putchar('\r');
-putchar(ch);
+   if (ch == '\n')
+   putchar('\r');
+   putchar(ch);
+   return (ch == '\n' ? 2 : 1);
 }

Modified: head/sys/boot/arm/ixp425/boot2/ixp425_board.c
==
--- head/sys/boot/arm/ixp425/boot2/ixp425_board.c   Thu Aug  3 03:45:48 
2017(r321968)
+++ head/sys/boot/arm/ixp425/boot2/ixp425_board.c   Thu Aug  3 05:27:05 
2017(r321969)
@@ -165,7 +165,7 @@ getc(int seconds)
return c;
 }
 
-void
+int
 putchar(int ch)
 {
int delay, limit;
@@ -179,14 +179,16 @@ putchar(int ch)
limit = 40;
while ((uart_getreg(ubase, REG_LSR) & LSR_TEMT) == 0 && --limit)
DELAY(delay);
+   return (1);
 }
 
-void
+int
 xputchar(int ch)
 {
if (ch == '\n')
putchar('\r');
putchar(ch);
+   return (ch == '\n' ? 2 : 1);
 }
 
 void

Modified: head/sys/boot/arm/ixp425/boot2/lib.h
==
--- head/sys/boot/arm/ixp425/boot2/lib.hThu Aug  3 03:45:48 2017
(r321968)
+++ head/sys/boot/arm/ixp425/boot2/lib.hThu Aug  3 05:27:05 2017
(r321969)
@@ -35,12 +35,12 @@ int main(void);
 void DELAY(int);
 
 int getc(int);
-void putchar(int);
-void xputchar(int);
+int putchar(int);
+int xputchar(int);
 void putstr(const char *);
 void puthex8(u_int8_t);
 void puthexlist(const u_int8_t *, int);
-void printf(const char *fmt,...);
+int printf(const char *fmt,...);
 
 void bzero(void *, size_t);
 char *strcpy(char *to, const char *from);

Modified: head/sys/boot/i386/boot2/boot2.c
==
--- head/sys/boot/i386/boot2/boot2.cThu Aug  3 03:45:48 2017
(r321968)
+++ head/sys/boot/i386/boot2/boot2.cThu Aug  3 05:27:05 2017
(r321969)
@@ -114,8 +114,8 @@ void exit(int);
 static void load(void);
 static int parse(void);
 static int dskread(void *, unsigned, unsigned);
-static void