Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
Hi Tom
Yes, Nds32 has a newer toolchain gcc 4.9 available now.
Rick
-Original Message-
From: Tom Rini [mailto:tr...@konsulko.com]
Sent: Wednesday, August 09, 2017 7:27 PM
To: Rob Clark; Rick Jian-Zhi Chen(陳建志)
Cc: Alexander Graf; Heinrich Schuchardt; U-Boot Mailing List; Peter Jones;
Simon Glass; Sekhar Nori; Bin Meng
Subject: Re: [U-Boot,v0,07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 08, 2017 at 08:14:41PM -0400, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 7:55 PM, Alexander Graf wrote:
> >
> >
> > On 09.08.17 00:39, Rob Clark wrote:
> >>
> >> On Tue, Aug 8, 2017 at 7:08 PM, Heinrich Schuchardt
> >>
> >> wrote:
> >>>
> >>> On 08/09/2017 12:44 AM, Rob Clark wrote:
>
> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
>
> wrote:
> >
> > On 08/04/2017 09:31 PM, Rob Clark wrote:
> >>
> >> This is convenient for efi_loader which deals a lot with utf16.
> >>
> >> Signed-off-by: Rob Clark
> >
> >
> > Please, put this patch together with [PATCH] vsprintf.c: add
> > GUID printing https://patchwork.ozlabs.org/patch/798362/
> > and
> > [PATCH v0 06/20] common: add some utf16 handling helpers
> > https://patchwork.ozlabs.org/patch/797968/
> > into a separate patch series.
> >
> > These three patches can be reviewed independently of the
> > efi_loader patches and probably will not be integrated via the efi-next
> > tree.
>
>
> I'll resend these as a separate patchset, and just not in next
> revision of efi_loader patchset that it is a dependency
>
> >> ---
> >> lib/vsprintf.c | 30 --
> >> 1 file changed, 28 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c index
> >> 874a2951f7..0c40f852ce 100644
> >> --- a/lib/vsprintf.c
> >> +++ b/lib/vsprintf.c
> >> @@ -17,6 +17,7 @@
> >> #include
> >>
> >> #include
> >> +#include
> >>
> >> #include
> >> #define noinline __attribute__((noinline)) @@ -270,6 +271,26
> >> @@ static char *string(char *buf, char *end, char *s, int
> >> field_width,
> >>return buf;
> >> }
> >>
> >> +static char *string16(char *buf, char *end, u16 *s, int field_width,
> >> + int precision, int flags) {
> >> + u16 *str = s ? s : L"";
> >
> > Please, do not use the L-notation here as it requires -fshort-wchar.
> > As we currently cannot switch the complete project to C11 you
> > cannot use the u-notation either.
>
>
> current plan was to either switch whole project to -fshort-wchar or
> c11 and rework these patches (as well as a few patches in the
> efi_loader patchset). (In the c11 case, I'm not sure what we'll use
> as the fmt string, since afaict that isn't specified. We could use %S
> although that seems to be a deprecated way to do %ls, or something
> different like %A, I guess)..
>
> how far are we from c11? If there is stuff I can do to help let me
> know. If feasible, I'd rather do that first rather than have a bunch
> of stuff in vsprintf and elsewhere that needs to be cleaned up later
> after the switch.
> >>>
> >>>
> >>> buildman downloads very old compilers (gcc < 4.8) from kernel.org which
> >>> do not support C11.
> >>> Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
> >>> error for disk/part.c in C11 mode.
> >>
> >>
> >> ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
> >> uses. It seems like *some* c11 was supported w/ >=4.6 so if we
> >> approach the conversion piecemeal (for example skipping code that
> >> triggers gcc bugs on old compilers) we might be able to keep 4.8.4
> >> working until travis provides something newer.
> >>
> >> (btw, even going back say 8 fedora releases or more, I've used distro
> >> packaged arm and aarch64 toolchains exclusively.. are there that many
> >> distro's where we really can't assume availability of an
> >> cross-toolchain? If there isn't something newer from kernel.org can
> >> we just drop relying on ancient prebuilt toolchains? I'm anyways not
> >> hugely a fan of downloading binary executables from even kernel.org,
> >> instead of using something from a distro build system which I at least
> >> know is very locked down.)
> >>
> >>> To get things right we would have to
> >>> * build our own cross tool chains based on a current gcc version
> >>> * use our own tool chain in Travis for x86-64 or use a docker
> >>>container with a current gcc version.
> >>>
> >>> In the long run heading for C11 would be the right thing to do.
> >>> Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
> >>> It looks ugly but does not consume more bytes once compiled.
> >>>
> >>
> >> Sure, that I'm less worried about, as much as adding stuff that is
> >> very soon going to be legacy.
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Wed, Aug 9, 2017 at 9:48 AM, Alexander Graf wrote:
>
>
>> Am 09.08.2017 um 14:38 schrieb Rob Clark :
>>
>>> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
>>> wrote:
On 08/04/2017 09:31 PM, Rob Clark wrote:
@@ -528,8 +549,13 @@ repeat:
continue;
case 's':
- str = string(str, end, va_arg(args, char *),
- field_width, precision, flags);
+ if (qualifier == 'l') {
>>>
>>> %ls refers to wchar with implementation dependent width in the C standard.
>>> There is no qualifier for 16-bit wchar. Couldn't we use %us here in
>>> reference to the u-notation ( u'MyString' ). This would leave the path
>>> open for a standard compliant '%ls'.
>>>
>>
>> So two drawbacks I'm running into when converting to c11 u"string"
>> style, compared to the -fshort-wchar:
>>
>> 1) with -fshort-wchar plus %ls, gcc knows how to typecheck the
>> printf/sprintf/etc args
>> 2) introducing a non-standard conversion character (since there
>> doesn't seem to be a standard one) means we need to drop -Wformat
>>
>> So far, afaict, the only argument against -fshort-wchar seems to be
>> that someday ext4 might support utf32 filenames? (And really
>> -fshort-wchar doesn't preclude that. So I'm not sure this is a valid
>> argument.)
>>
>> So independent of c11 (which might be a good idea for other reasons),
>> I'm back to thinking we should use -fshort-wchar. Possibly as a
>> kconfig option that EFI_LOADER selects.. or possibly just
>> unconditionally.
>>
>> Thoughts?
>
> If we select it, I'd rather have it be unconditional, to not oprn potential
> for undetected breakage.
>
I could go either way on kconfig option vs unconditional
-fshort-wchar. Although as far as breakage, that seems pretty
solvable by adding a fallback.efi -> grub.efi test, and maybe
something that exercises device-path-to-text, in travis.
I suppose it might be useful, for example, for TINY_PRINTF to depend
on !CC_SHORT_WCHAR? Not sure.
I'll include a patch w/ kconfig option in my patchset for now, but
happy to drop it if folks want to do -fshort-wchar unconditionally.
BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
> Am 09.08.2017 um 14:38 schrieb Rob Clark :
>
>> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
>> wrote:
>>> On 08/04/2017 09:31 PM, Rob Clark wrote:
>>>
>>> @@ -528,8 +549,13 @@ repeat:
>>> continue;
>>>
>>> case 's':
>>> - str = string(str, end, va_arg(args, char *),
>>> - field_width, precision, flags);
>>> + if (qualifier == 'l') {
>>
>> %ls refers to wchar with implementation dependent width in the C standard.
>> There is no qualifier for 16-bit wchar. Couldn't we use %us here in
>> reference to the u-notation ( u'MyString' ). This would leave the path
>> open for a standard compliant '%ls'.
>>
>
> So two drawbacks I'm running into when converting to c11 u"string"
> style, compared to the -fshort-wchar:
>
> 1) with -fshort-wchar plus %ls, gcc knows how to typecheck the
> printf/sprintf/etc args
> 2) introducing a non-standard conversion character (since there
> doesn't seem to be a standard one) means we need to drop -Wformat
>
> So far, afaict, the only argument against -fshort-wchar seems to be
> that someday ext4 might support utf32 filenames? (And really
> -fshort-wchar doesn't preclude that. So I'm not sure this is a valid
> argument.)
>
> So independent of c11 (which might be a good idea for other reasons),
> I'm back to thinking we should use -fshort-wchar. Possibly as a
> kconfig option that EFI_LOADER selects.. or possibly just
> unconditionally.
>
> Thoughts?
If we select it, I'd rather have it be unconditional, to not oprn potential for
undetected breakage.
Alex
>
> BR,
> -R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt wrote:
> On 08/04/2017 09:31 PM, Rob Clark wrote:
>>
>> @@ -528,8 +549,13 @@ repeat:
>> continue;
>>
>> case 's':
>> - str = string(str, end, va_arg(args, char *),
>> - field_width, precision, flags);
>> + if (qualifier == 'l') {
>
> %ls refers to wchar with implementation dependent width in the C standard.
> There is no qualifier for 16-bit wchar. Couldn't we use %us here in
> reference to the u-notation ( u'MyString' ). This would leave the path
> open for a standard compliant '%ls'.
>
So two drawbacks I'm running into when converting to c11 u"string"
style, compared to the -fshort-wchar:
1) with -fshort-wchar plus %ls, gcc knows how to typecheck the
printf/sprintf/etc args
2) introducing a non-standard conversion character (since there
doesn't seem to be a standard one) means we need to drop -Wformat
So far, afaict, the only argument against -fshort-wchar seems to be
that someday ext4 might support utf32 filenames? (And really
-fshort-wchar doesn't preclude that. So I'm not sure this is a valid
argument.)
So independent of c11 (which might be a good idea for other reasons),
I'm back to thinking we should use -fshort-wchar. Possibly as a
kconfig option that EFI_LOADER selects.. or possibly just
unconditionally.
Thoughts?
BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 08, 2017 at 08:14:41PM -0400, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 7:55 PM, Alexander Graf wrote:
> >
> >
> > On 09.08.17 00:39, Rob Clark wrote:
> >>
> >> On Tue, Aug 8, 2017 at 7:08 PM, Heinrich Schuchardt
> >> wrote:
> >>>
> >>> On 08/09/2017 12:44 AM, Rob Clark wrote:
>
> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
> wrote:
> >
> > On 08/04/2017 09:31 PM, Rob Clark wrote:
> >>
> >> This is convenient for efi_loader which deals a lot with utf16.
> >>
> >> Signed-off-by: Rob Clark
> >
> >
> > Please, put this patch together with
> > [PATCH] vsprintf.c: add GUID printing
> > https://patchwork.ozlabs.org/patch/798362/
> > and
> > [PATCH v0 06/20] common: add some utf16 handling helpers
> > https://patchwork.ozlabs.org/patch/797968/
> > into a separate patch series.
> >
> > These three patches can be reviewed independently of the efi_loader
> > patches and probably will not be integrated via the efi-next tree.
>
>
> I'll resend these as a separate patchset, and just not in next
> revision of efi_loader patchset that it is a dependency
>
> >> ---
> >> lib/vsprintf.c | 30 --
> >> 1 file changed, 28 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> >> index 874a2951f7..0c40f852ce 100644
> >> --- a/lib/vsprintf.c
> >> +++ b/lib/vsprintf.c
> >> @@ -17,6 +17,7 @@
> >> #include
> >>
> >> #include
> >> +#include
> >>
> >> #include
> >> #define noinline __attribute__((noinline))
> >> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char
> >> *s, int field_width,
> >>return buf;
> >> }
> >>
> >> +static char *string16(char *buf, char *end, u16 *s, int field_width,
> >> + int precision, int flags)
> >> +{
> >> + u16 *str = s ? s : L"";
> >
> > Please, do not use the L-notation here as it requires -fshort-wchar.
> > As we currently cannot switch the complete project to C11 you cannot
> > use
> > the u-notation either.
>
>
> current plan was to either switch whole project to -fshort-wchar or
> c11 and rework these patches (as well as a few patches in the
> efi_loader patchset). (In the c11 case, I'm not sure what we'll use
> as the fmt string, since afaict that isn't specified. We could use %S
> although that seems to be a deprecated way to do %ls, or something
> different like %A, I guess)..
>
> how far are we from c11? If there is stuff I can do to help let me
> know. If feasible, I'd rather do that first rather than have a bunch
> of stuff in vsprintf and elsewhere that needs to be cleaned up later
> after the switch.
> >>>
> >>>
> >>> buildman downloads very old compilers (gcc < 4.8) from kernel.org which
> >>> do not support C11.
> >>> Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
> >>> error for disk/part.c in C11 mode.
> >>
> >>
> >> ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
> >> uses. It seems like *some* c11 was supported w/ >=4.6 so if we
> >> approach the conversion piecemeal (for example skipping code that
> >> triggers gcc bugs on old compilers) we might be able to keep 4.8.4
> >> working until travis provides something newer.
> >>
> >> (btw, even going back say 8 fedora releases or more, I've used distro
> >> packaged arm and aarch64 toolchains exclusively.. are there that many
> >> distro's where we really can't assume availability of an
> >> cross-toolchain? If there isn't something newer from kernel.org can
> >> we just drop relying on ancient prebuilt toolchains? I'm anyways not
> >> hugely a fan of downloading binary executables from even kernel.org,
> >> instead of using something from a distro build system which I at least
> >> know is very locked down.)
> >>
> >>> To get things right we would have to
> >>> * build our own cross tool chains based on a current gcc version
> >>> * use our own tool chain in Travis for x86-64 or use a docker
> >>>container with a current gcc version.
> >>>
> >>> In the long run heading for C11 would be the right thing to do.
> >>> Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
> >>> It looks ugly but does not consume more bytes once compiled.
> >>>
> >>
> >> Sure, that I'm less worried about, as much as adding stuff that is
> >> very soon going to be legacy. Even in vfprintf.c it isn't such a big
> >> deal, as efi_loader where it would be more cumbersome.
> >>
> >> Maybe we can write out u"" longhand in vsprintf.c as you
> >> suggest, but restrict efi_loader to gcc >= 4.9? That seems like it
> >> shouldn't be a problem for any arm/arm64 device and it shouldn't be a
> >> problem for any device that is likely to have an efi payload to load
> >> in the f
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
>> buildman downloads very old compilers (gcc < 4.8) from kernel.org which
>> do not support C11.
>> Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
>> error for disk/part.c in C11 mode.
>
> ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
> uses. It seems like *some* c11 was supported w/ >=4.6 so if we
> approach the conversion piecemeal (for example skipping code that
> triggers gcc bugs on old compilers) we might be able to keep 4.8.4
> working until travis provides something newer.
For reference el7 (RHEL/CentOS etc) has gcc 4.8.5
> (btw, even going back say 8 fedora releases or more, I've used distro
> packaged arm and aarch64 toolchains exclusively.. are there that many
> distro's where we really can't assume availability of an
> cross-toolchain? If there isn't something newer from kernel.org can
> we just drop relying on ancient prebuilt toolchains? I'm anyways not
> hugely a fan of downloading binary executables from even kernel.org,
> instead of using something from a distro build system which I at least
> know is very locked down.)
>
>> To get things right we would have to
>> * build our own cross tool chains based on a current gcc version
>> * use our own tool chain in Travis for x86-64 or use a docker
>> container with a current gcc version.
>>
>> In the long run heading for C11 would be the right thing to do.
>> Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
>> It looks ugly but does not consume more bytes once compiled.
>>
>
> Sure, that I'm less worried about, as much as adding stuff that is
> very soon going to be legacy. Even in vfprintf.c it isn't such a big
> deal, as efi_loader where it would be more cumbersome.
>
> Maybe we can write out u"" longhand in vsprintf.c as you
> suggest, but restrict efi_loader to gcc >= 4.9? That seems like it
> shouldn't be a problem for any arm/arm64 device and it shouldn't be a
> problem for any device that is likely to have an efi payload to load
> in the first place..
>
> BR,
> -R
> ___
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 8, 2017 at 8:14 PM, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 7:55 PM, Alexander Graf wrote:
>>
>>
>> On 09.08.17 00:39, Rob Clark wrote:
>>>
>>> On Tue, Aug 8, 2017 at 7:08 PM, Heinrich Schuchardt
>>> wrote:
On 08/09/2017 12:44 AM, Rob Clark wrote:
>
> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
> wrote:
>>
>> On 08/04/2017 09:31 PM, Rob Clark wrote:
>>>
>>> This is convenient for efi_loader which deals a lot with utf16.
>>>
>>> Signed-off-by: Rob Clark
>>
>>
>> Please, put this patch together with
>> [PATCH] vsprintf.c: add GUID printing
>> https://patchwork.ozlabs.org/patch/798362/
>> and
>> [PATCH v0 06/20] common: add some utf16 handling helpers
>> https://patchwork.ozlabs.org/patch/797968/
>> into a separate patch series.
>>
>> These three patches can be reviewed independently of the efi_loader
>> patches and probably will not be integrated via the efi-next tree.
>
>
> I'll resend these as a separate patchset, and just not in next
> revision of efi_loader patchset that it is a dependency
>
>>> ---
>>> lib/vsprintf.c | 30 --
>>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> index 874a2951f7..0c40f852ce 100644
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -17,6 +17,7 @@
>>> #include
>>>
>>> #include
>>> +#include
>>>
>>> #include
>>> #define noinline __attribute__((noinline))
>>> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char
>>> *s, int field_width,
>>>return buf;
>>> }
>>>
>>> +static char *string16(char *buf, char *end, u16 *s, int field_width,
>>> + int precision, int flags)
>>> +{
>>> + u16 *str = s ? s : L"";
>>
>> Please, do not use the L-notation here as it requires -fshort-wchar.
>> As we currently cannot switch the complete project to C11 you cannot
>> use
>> the u-notation either.
>
>
> current plan was to either switch whole project to -fshort-wchar or
> c11 and rework these patches (as well as a few patches in the
> efi_loader patchset). (In the c11 case, I'm not sure what we'll use
> as the fmt string, since afaict that isn't specified. We could use %S
> although that seems to be a deprecated way to do %ls, or something
> different like %A, I guess)..
>
> how far are we from c11? If there is stuff I can do to help let me
> know. If feasible, I'd rather do that first rather than have a bunch
> of stuff in vsprintf and elsewhere that needs to be cleaned up later
> after the switch.
buildman downloads very old compilers (gcc < 4.8) from kernel.org which
do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
error for disk/part.c in C11 mode.
>>>
>>>
>>> ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
>>> uses. It seems like *some* c11 was supported w/ >=4.6 so if we
>>> approach the conversion piecemeal (for example skipping code that
>>> triggers gcc bugs on old compilers) we might be able to keep 4.8.4
>>> working until travis provides something newer.
>>>
>>> (btw, even going back say 8 fedora releases or more, I've used distro
>>> packaged arm and aarch64 toolchains exclusively.. are there that many
>>> distro's where we really can't assume availability of an
>>> cross-toolchain? If there isn't something newer from kernel.org can
>>> we just drop relying on ancient prebuilt toolchains? I'm anyways not
>>> hugely a fan of downloading binary executables from even kernel.org,
>>> instead of using something from a distro build system which I at least
>>> know is very locked down.)
>>>
To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker
container with a current gcc version.
In the long run heading for C11 would be the right thing to do.
Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
It looks ugly but does not consume more bytes once compiled.
>>>
>>> Sure, that I'm less worried about, as much as adding stuff that is
>>> very soon going to be legacy. Even in vfprintf.c it isn't such a big
>>> deal, as efi_loader where it would be more cumbersome.
>>>
>>> Maybe we can write out u"" longhand in vsprintf.c as you
>>> suggest, but restrict efi_loader to gcc >= 4.9? That seems like it
>>> shouldn't be a problem for any arm/arm64 device and it shouldn't be a
>>> problem for any device that is likely to have an efi payload to load
>>> in the first place..
>>
>>
>> I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a good
>> reason, so they all get ch
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 8, 2017 at 7:55 PM, Alexander Graf wrote:
>
>
> On 09.08.17 00:39, Rob Clark wrote:
>>
>> On Tue, Aug 8, 2017 at 7:08 PM, Heinrich Schuchardt
>> wrote:
>>>
>>> On 08/09/2017 12:44 AM, Rob Clark wrote:
On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
wrote:
>
> On 08/04/2017 09:31 PM, Rob Clark wrote:
>>
>> This is convenient for efi_loader which deals a lot with utf16.
>>
>> Signed-off-by: Rob Clark
>
>
> Please, put this patch together with
> [PATCH] vsprintf.c: add GUID printing
> https://patchwork.ozlabs.org/patch/798362/
> and
> [PATCH v0 06/20] common: add some utf16 handling helpers
> https://patchwork.ozlabs.org/patch/797968/
> into a separate patch series.
>
> These three patches can be reviewed independently of the efi_loader
> patches and probably will not be integrated via the efi-next tree.
I'll resend these as a separate patchset, and just not in next
revision of efi_loader patchset that it is a dependency
>> ---
>> lib/vsprintf.c | 30 --
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 874a2951f7..0c40f852ce 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -17,6 +17,7 @@
>> #include
>>
>> #include
>> +#include
>>
>> #include
>> #define noinline __attribute__((noinline))
>> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char
>> *s, int field_width,
>>return buf;
>> }
>>
>> +static char *string16(char *buf, char *end, u16 *s, int field_width,
>> + int precision, int flags)
>> +{
>> + u16 *str = s ? s : L"";
>
> Please, do not use the L-notation here as it requires -fshort-wchar.
> As we currently cannot switch the complete project to C11 you cannot
> use
> the u-notation either.
current plan was to either switch whole project to -fshort-wchar or
c11 and rework these patches (as well as a few patches in the
efi_loader patchset). (In the c11 case, I'm not sure what we'll use
as the fmt string, since afaict that isn't specified. We could use %S
although that seems to be a deprecated way to do %ls, or something
different like %A, I guess)..
how far are we from c11? If there is stuff I can do to help let me
know. If feasible, I'd rather do that first rather than have a bunch
of stuff in vsprintf and elsewhere that needs to be cleaned up later
after the switch.
>>>
>>>
>>> buildman downloads very old compilers (gcc < 4.8) from kernel.org which
>>> do not support C11.
>>> Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
>>> error for disk/part.c in C11 mode.
>>
>>
>> ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
>> uses. It seems like *some* c11 was supported w/ >=4.6 so if we
>> approach the conversion piecemeal (for example skipping code that
>> triggers gcc bugs on old compilers) we might be able to keep 4.8.4
>> working until travis provides something newer.
>>
>> (btw, even going back say 8 fedora releases or more, I've used distro
>> packaged arm and aarch64 toolchains exclusively.. are there that many
>> distro's where we really can't assume availability of an
>> cross-toolchain? If there isn't something newer from kernel.org can
>> we just drop relying on ancient prebuilt toolchains? I'm anyways not
>> hugely a fan of downloading binary executables from even kernel.org,
>> instead of using something from a distro build system which I at least
>> know is very locked down.)
>>
>>> To get things right we would have to
>>> * build our own cross tool chains based on a current gcc version
>>> * use our own tool chain in Travis for x86-64 or use a docker
>>>container with a current gcc version.
>>>
>>> In the long run heading for C11 would be the right thing to do.
>>> Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
>>> It looks ugly but does not consume more bytes once compiled.
>>>
>>
>> Sure, that I'm less worried about, as much as adding stuff that is
>> very soon going to be legacy. Even in vfprintf.c it isn't such a big
>> deal, as efi_loader where it would be more cumbersome.
>>
>> Maybe we can write out u"" longhand in vsprintf.c as you
>> suggest, but restrict efi_loader to gcc >= 4.9? That seems like it
>> shouldn't be a problem for any arm/arm64 device and it shouldn't be a
>> problem for any device that is likely to have an efi payload to load
>> in the first place..
>
>
> I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a good
> reason, so they all get checked by travis. If we break travis, that won't do
> anyone any good.
I was more thinking if there was some oddball non-arm arch that u-boot
supported which didn't haven good
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On 09.08.17 00:39, Rob Clark wrote:
On Tue, Aug 8, 2017 at 7:08 PM, Heinrich Schuchardt wrote:
On 08/09/2017 12:44 AM, Rob Clark wrote:
On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt wrote:
On 08/04/2017 09:31 PM, Rob Clark wrote:
This is convenient for efi_loader which deals a lot with utf16.
Signed-off-by: Rob Clark
Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.
These three patches can be reviewed independently of the efi_loader
patches and probably will not be integrated via the efi-next tree.
I'll resend these as a separate patchset, and just not in next
revision of efi_loader patchset that it is a dependency
---
lib/vsprintf.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 874a2951f7..0c40f852ce 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
#define noinline __attribute__((noinline))
@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int
field_width,
return buf;
}
+static char *string16(char *buf, char *end, u16 *s, int field_width,
+ int precision, int flags)
+{
+ u16 *str = s ? s : L"";
Please, do not use the L-notation here as it requires -fshort-wchar.
As we currently cannot switch the complete project to C11 you cannot use
the u-notation either.
current plan was to either switch whole project to -fshort-wchar or
c11 and rework these patches (as well as a few patches in the
efi_loader patchset). (In the c11 case, I'm not sure what we'll use
as the fmt string, since afaict that isn't specified. We could use %S
although that seems to be a deprecated way to do %ls, or something
different like %A, I guess)..
how far are we from c11? If there is stuff I can do to help let me
know. If feasible, I'd rather do that first rather than have a bunch
of stuff in vsprintf and elsewhere that needs to be cleaned up later
after the switch.
buildman downloads very old compilers (gcc < 4.8) from kernel.org which
do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
error for disk/part.c in C11 mode.
ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we
approach the conversion piecemeal (for example skipping code that
triggers gcc bugs on old compilers) we might be able to keep 4.8.4
working until travis provides something newer.
(btw, even going back say 8 fedora releases or more, I've used distro
packaged arm and aarch64 toolchains exclusively.. are there that many
distro's where we really can't assume availability of an
cross-toolchain? If there isn't something newer from kernel.org can
we just drop relying on ancient prebuilt toolchains? I'm anyways not
hugely a fan of downloading binary executables from even kernel.org,
instead of using something from a distro build system which I at least
know is very locked down.)
To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker
container with a current gcc version.
In the long run heading for C11 would be the right thing to do.
Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
It looks ugly but does not consume more bytes once compiled.
Sure, that I'm less worried about, as much as adding stuff that is
very soon going to be legacy. Even in vfprintf.c it isn't such a big
deal, as efi_loader where it would be more cumbersome.
Maybe we can write out u"" longhand in vsprintf.c as you
suggest, but restrict efi_loader to gcc >= 4.9? That seems like it
shouldn't be a problem for any arm/arm64 device and it shouldn't be a
problem for any device that is likely to have an efi payload to load
in the first place..
I don't understand? We enable EFI_LOADER on all arm/arm64 systems for a
good reason, so they all get checked by travis. If we break travis, that
won't do anyone any good.
I do remember however that Tom wanted to set certain compiler versions
as minimum required versions. Tom, do you remember which one that was?
Alex
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 8, 2017 at 7:08 PM, Heinrich Schuchardt wrote:
> On 08/09/2017 12:44 AM, Rob Clark wrote:
>> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
>> wrote:
>>> On 08/04/2017 09:31 PM, Rob Clark wrote:
This is convenient for efi_loader which deals a lot with utf16.
Signed-off-by: Rob Clark
>>>
>>> Please, put this patch together with
>>> [PATCH] vsprintf.c: add GUID printing
>>> https://patchwork.ozlabs.org/patch/798362/
>>> and
>>> [PATCH v0 06/20] common: add some utf16 handling helpers
>>> https://patchwork.ozlabs.org/patch/797968/
>>> into a separate patch series.
>>>
>>> These three patches can be reviewed independently of the efi_loader
>>> patches and probably will not be integrated via the efi-next tree.
>>
>> I'll resend these as a separate patchset, and just not in next
>> revision of efi_loader patchset that it is a dependency
>>
---
lib/vsprintf.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 874a2951f7..0c40f852ce 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
#define noinline __attribute__((noinline))
@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s,
int field_width,
return buf;
}
+static char *string16(char *buf, char *end, u16 *s, int field_width,
+ int precision, int flags)
+{
+ u16 *str = s ? s : L"";
>>> Please, do not use the L-notation here as it requires -fshort-wchar.
>>> As we currently cannot switch the complete project to C11 you cannot use
>>> the u-notation either.
>>
>> current plan was to either switch whole project to -fshort-wchar or
>> c11 and rework these patches (as well as a few patches in the
>> efi_loader patchset). (In the c11 case, I'm not sure what we'll use
>> as the fmt string, since afaict that isn't specified. We could use %S
>> although that seems to be a deprecated way to do %ls, or something
>> different like %A, I guess)..
>>
>> how far are we from c11? If there is stuff I can do to help let me
>> know. If feasible, I'd rather do that first rather than have a bunch
>> of stuff in vsprintf and elsewhere that needs to be cleaned up later
>> after the switch.
>
> buildman downloads very old compilers (gcc < 4.8) from kernel.org which
> do not support C11.
> Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
> error for disk/part.c in C11 mode.
ugg, 4.8 is pretty old.. Not sure how much older than 4.8 buildman
uses. It seems like *some* c11 was supported w/ >=4.6 so if we
approach the conversion piecemeal (for example skipping code that
triggers gcc bugs on old compilers) we might be able to keep 4.8.4
working until travis provides something newer.
(btw, even going back say 8 fedora releases or more, I've used distro
packaged arm and aarch64 toolchains exclusively.. are there that many
distro's where we really can't assume availability of an
cross-toolchain? If there isn't something newer from kernel.org can
we just drop relying on ancient prebuilt toolchains? I'm anyways not
hugely a fan of downloading binary executables from even kernel.org,
instead of using something from a distro build system which I at least
know is very locked down.)
> To get things right we would have to
> * build our own cross tool chains based on a current gcc version
> * use our own tool chain in Travis for x86-64 or use a docker
> container with a current gcc version.
>
> In the long run heading for C11 would be the right thing to do.
> Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
> It looks ugly but does not consume more bytes once compiled.
>
Sure, that I'm less worried about, as much as adding stuff that is
very soon going to be legacy. Even in vfprintf.c it isn't such a big
deal, as efi_loader where it would be more cumbersome.
Maybe we can write out u"" longhand in vsprintf.c as you
suggest, but restrict efi_loader to gcc >= 4.9? That seems like it
shouldn't be a problem for any arm/arm64 device and it shouldn't be a
problem for any device that is likely to have an efi payload to load
in the first place..
BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
> Am 09.08.2017 um 00:08 schrieb Heinrich Schuchardt :
>
>> On 08/09/2017 12:44 AM, Rob Clark wrote:
>>> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
>>> wrote:
On 08/04/2017 09:31 PM, Rob Clark wrote:
This is convenient for efi_loader which deals a lot with utf16.
Signed-off-by: Rob Clark
>>>
>>> Please, put this patch together with
>>> [PATCH] vsprintf.c: add GUID printing
>>> https://patchwork.ozlabs.org/patch/798362/
>>> and
>>> [PATCH v0 06/20] common: add some utf16 handling helpers
>>> https://patchwork.ozlabs.org/patch/797968/
>>> into a separate patch series.
>>>
>>> These three patches can be reviewed independently of the efi_loader
>>> patches and probably will not be integrated via the efi-next tree.
>>
>> I'll resend these as a separate patchset, and just not in next
>> revision of efi_loader patchset that it is a dependency
>>
---
lib/vsprintf.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 874a2951f7..0c40f852ce 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -17,6 +17,7 @@
#include
#include
+#include
#include
#define noinline __attribute__((noinline))
@@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s,
int field_width,
return buf;
}
+static char *string16(char *buf, char *end, u16 *s, int field_width,
+ int precision, int flags)
+{
+ u16 *str = s ? s : L"";
>>> Please, do not use the L-notation here as it requires -fshort-wchar.
>>> As we currently cannot switch the complete project to C11 you cannot use
>>> the u-notation either.
>>
>> current plan was to either switch whole project to -fshort-wchar or
>> c11 and rework these patches (as well as a few patches in the
>> efi_loader patchset). (In the c11 case, I'm not sure what we'll use
>> as the fmt string, since afaict that isn't specified. We could use %S
>> although that seems to be a deprecated way to do %ls, or something
>> different like %A, I guess)..
>>
>> how far are we from c11? If there is stuff I can do to help let me
>> know. If feasible, I'd rather do that first rather than have a bunch
>> of stuff in vsprintf and elsewhere that needs to be cleaned up later
>> after the switch.
>
> buildman downloads very old compilers (gcc < 4.8) from kernel.org which
> do not support C11.
> Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
> error for disk/part.c in C11 mode.
>
> To get things right we would have to
> * build our own cross tool chains based on a current gcc version
> * use our own tool chain in Travis for x86-64 or use a docker
> container with a current gcc version.
>
> In the long run heading for C11 would be the right thing to do.
> Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
Don't forget the , 0 :)
Alex
> It looks ugly but does not consume more bytes once compiled.
>
> Regards
>
> Heinrich
>
>>
>>>
+ int len = utf16_strnlen(str, precision);
+ u8 utf8[len * MAX_UTF8_PER_UTF16];
>>>
>>> Didn't you forget 1 byte for \0 here?
>>>
>>> This is what strlnlen does:
>>>
>>> The strnlen() function returns the number of characters in the string
>>> pointed to by s, **excluding** the terminating null byte ('\0'), but at
>>> most maxlen.
>>>
>>> I would expect the exclusion of the terminating null word by an
>>> utf16_strnlen function.
>>
>> you are right, but fixing the wrong problem.. the code is definitely
>> wrong since length of a utf16 string != length of a utf8 string, and
>> we don't need to append a null terminator.. so my logic below using
>> 'len' is wrong. I'll fix that in the next version.
>>
+ int i;
+
+ *utf16_to_utf8(utf8, str, len) = '\0';
+
+ if (!(flags & LEFT))
+ while (len < field_width--)
+ ADDCH(buf, ' ');
+ for (i = 0; i < len; ++i)
+ ADDCH(buf, utf8[i]);
+ while (len < field_width--)
+ ADDCH(buf, ' ');
+ return buf;
+}
+
#ifdef CONFIG_CMD_NET
static const char hex_asc[] = "0123456789abcdef";
#define hex_asc_lo(x)hex_asc[((x) & 0x0f)]
@@ -528,8 +549,13 @@ repeat:
continue;
case 's':
- str = string(str, end, va_arg(args, char *),
- field_width, precision, flags);
+ if (qualifier == 'l') {
>>>
>>> %ls refers to wchar with implementation dependent width in the C standard.
>>> There is no qualifier for 16-bit wchar. Couldn't we use %us here in
>>> reference to the u-notation ( u'MyString' ). This would leave the path
>>> open for a standard compliant '%ls'.
>>
>> hmm, yeah, that is a clever idea, and I like it better
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On 08/09/2017 12:44 AM, Rob Clark wrote:
> On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt
> wrote:
>> On 08/04/2017 09:31 PM, Rob Clark wrote:
>>> This is convenient for efi_loader which deals a lot with utf16.
>>>
>>> Signed-off-by: Rob Clark
>>
>> Please, put this patch together with
>> [PATCH] vsprintf.c: add GUID printing
>> https://patchwork.ozlabs.org/patch/798362/
>> and
>> [PATCH v0 06/20] common: add some utf16 handling helpers
>> https://patchwork.ozlabs.org/patch/797968/
>> into a separate patch series.
>>
>> These three patches can be reviewed independently of the efi_loader
>> patches and probably will not be integrated via the efi-next tree.
>
> I'll resend these as a separate patchset, and just not in next
> revision of efi_loader patchset that it is a dependency
>
>>> ---
>>> lib/vsprintf.c | 30 --
>>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>>> index 874a2951f7..0c40f852ce 100644
>>> --- a/lib/vsprintf.c
>>> +++ b/lib/vsprintf.c
>>> @@ -17,6 +17,7 @@
>>> #include
>>>
>>> #include
>>> +#include
>>>
>>> #include
>>> #define noinline __attribute__((noinline))
>>> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int
>>> field_width,
>>> return buf;
>>> }
>>>
>>> +static char *string16(char *buf, char *end, u16 *s, int field_width,
>>> + int precision, int flags)
>>> +{
>>> + u16 *str = s ? s : L"";
>> Please, do not use the L-notation here as it requires -fshort-wchar.
>> As we currently cannot switch the complete project to C11 you cannot use
>> the u-notation either.
>
> current plan was to either switch whole project to -fshort-wchar or
> c11 and rework these patches (as well as a few patches in the
> efi_loader patchset). (In the c11 case, I'm not sure what we'll use
> as the fmt string, since afaict that isn't specified. We could use %S
> although that seems to be a deprecated way to do %ls, or something
> different like %A, I guess)..
>
> how far are we from c11? If there is stuff I can do to help let me
> know. If feasible, I'd rather do that first rather than have a bunch
> of stuff in vsprintf and elsewhere that needs to be cleaned up later
> after the switch.
buildman downloads very old compilers (gcc < 4.8) from kernel.org which
do not support C11.
Travis CI uses Ubuntu 14.04 with gcc 4.8.4 which incorrectly throws an
error for disk/part.c in C11 mode.
To get things right we would have to
* build our own cross tool chains based on a current gcc version
* use our own tool chain in Travis for x86-64 or use a docker
container with a current gcc version.
In the long run heading for C11 would be the right thing to do.
Until then use an initializer { '<', 'N', 'U', 'L', 'L', '>' }.
It looks ugly but does not consume more bytes once compiled.
Regards
Heinrich
>
>>
>>> + int len = utf16_strnlen(str, precision);
>>> + u8 utf8[len * MAX_UTF8_PER_UTF16];
>>
>> Didn't you forget 1 byte for \0 here?
>>
>> This is what strlnlen does:
>>
>> The strnlen() function returns the number of characters in the string
>> pointed to by s, **excluding** the terminating null byte ('\0'), but at
>> most maxlen.
>>
>> I would expect the exclusion of the terminating null word by an
>> utf16_strnlen function.
>
> you are right, but fixing the wrong problem.. the code is definitely
> wrong since length of a utf16 string != length of a utf8 string, and
> we don't need to append a null terminator.. so my logic below using
> 'len' is wrong. I'll fix that in the next version.
>
>>> + int i;
>>> +
>>> + *utf16_to_utf8(utf8, str, len) = '\0';
>>> +
>>> + if (!(flags & LEFT))
>>> + while (len < field_width--)
>>> + ADDCH(buf, ' ');
>>> + for (i = 0; i < len; ++i)
>>> + ADDCH(buf, utf8[i]);
>>> + while (len < field_width--)
>>> + ADDCH(buf, ' ');
>>> + return buf;
>>> +}
>>> +
>>> #ifdef CONFIG_CMD_NET
>>> static const char hex_asc[] = "0123456789abcdef";
>>> #define hex_asc_lo(x)hex_asc[((x) & 0x0f)]
>>> @@ -528,8 +549,13 @@ repeat:
>>> continue;
>>>
>>> case 's':
>>> - str = string(str, end, va_arg(args, char *),
>>> - field_width, precision, flags);
>>> + if (qualifier == 'l') {
>>
>> %ls refers to wchar with implementation dependent width in the C standard.
>> There is no qualifier for 16-bit wchar. Couldn't we use %us here in
>> reference to the u-notation ( u'MyString' ). This would leave the path
>> open for a standard compliant '%ls'.
>
> hmm, yeah, that is a clever idea, and I like it better than %A or %S..
> so if we go the c11 route I'll do that. The c11 committee should have
> thought of that ;-)
>
> BR,
> -R
>
>> Best regards
>>
>> Heinrich
>>
>>> + str = string16(str, end, va_arg(args, u16 *),
>>> +
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt wrote:
> On 08/04/2017 09:31 PM, Rob Clark wrote:
>> This is convenient for efi_loader which deals a lot with utf16.
>>
>> Signed-off-by: Rob Clark
>
> Please, put this patch together with
> [PATCH] vsprintf.c: add GUID printing
> https://patchwork.ozlabs.org/patch/798362/
> and
> [PATCH v0 06/20] common: add some utf16 handling helpers
> https://patchwork.ozlabs.org/patch/797968/
> into a separate patch series.
>
> These three patches can be reviewed independently of the efi_loader
> patches and probably will not be integrated via the efi-next tree.
>
>> ---
>> lib/vsprintf.c | 30 --
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 874a2951f7..0c40f852ce 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -17,6 +17,7 @@
>> #include
>>
>> #include
>> +#include
>>
>> #include
>> #define noinline __attribute__((noinline))
>> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int
>> field_width,
>> return buf;
>> }
>>
>> +static char *string16(char *buf, char *end, u16 *s, int field_width,
>> + int precision, int flags)
>> +{
>> + u16 *str = s ? s : L"";
> Please, do not use the L-notation here as it requires -fshort-wchar.
> As we currently cannot switch the complete project to C11 you cannot use
> the u-notation either.
>
actually, one thought.. unlike -fshort-wchar, we could probably
switch u-boot over to c11 piecemeal, if there are a lot of places
where things need to be fixed for c11. For example start by switching
efi_loader and vsprintf.c ;-)
(I'm not completely sure what the issues are, so this may or may not
make sense.. but if c11 is causing a lot of compile errors all over
the place, this might be a reasonable approach.)
BR,
-R
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On Tue, Aug 8, 2017 at 6:03 PM, Heinrich Schuchardt wrote:
> On 08/04/2017 09:31 PM, Rob Clark wrote:
>> This is convenient for efi_loader which deals a lot with utf16.
>>
>> Signed-off-by: Rob Clark
>
> Please, put this patch together with
> [PATCH] vsprintf.c: add GUID printing
> https://patchwork.ozlabs.org/patch/798362/
> and
> [PATCH v0 06/20] common: add some utf16 handling helpers
> https://patchwork.ozlabs.org/patch/797968/
> into a separate patch series.
>
> These three patches can be reviewed independently of the efi_loader
> patches and probably will not be integrated via the efi-next tree.
I'll resend these as a separate patchset, and just not in next
revision of efi_loader patchset that it is a dependency
>> ---
>> lib/vsprintf.c | 30 --
>> 1 file changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
>> index 874a2951f7..0c40f852ce 100644
>> --- a/lib/vsprintf.c
>> +++ b/lib/vsprintf.c
>> @@ -17,6 +17,7 @@
>> #include
>>
>> #include
>> +#include
>>
>> #include
>> #define noinline __attribute__((noinline))
>> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int
>> field_width,
>> return buf;
>> }
>>
>> +static char *string16(char *buf, char *end, u16 *s, int field_width,
>> + int precision, int flags)
>> +{
>> + u16 *str = s ? s : L"";
> Please, do not use the L-notation here as it requires -fshort-wchar.
> As we currently cannot switch the complete project to C11 you cannot use
> the u-notation either.
current plan was to either switch whole project to -fshort-wchar or
c11 and rework these patches (as well as a few patches in the
efi_loader patchset). (In the c11 case, I'm not sure what we'll use
as the fmt string, since afaict that isn't specified. We could use %S
although that seems to be a deprecated way to do %ls, or something
different like %A, I guess)..
how far are we from c11? If there is stuff I can do to help let me
know. If feasible, I'd rather do that first rather than have a bunch
of stuff in vsprintf and elsewhere that needs to be cleaned up later
after the switch.
>
>> + int len = utf16_strnlen(str, precision);
>> + u8 utf8[len * MAX_UTF8_PER_UTF16];
>
> Didn't you forget 1 byte for \0 here?
>
> This is what strlnlen does:
>
> The strnlen() function returns the number of characters in the string
> pointed to by s, **excluding** the terminating null byte ('\0'), but at
> most maxlen.
>
> I would expect the exclusion of the terminating null word by an
> utf16_strnlen function.
you are right, but fixing the wrong problem.. the code is definitely
wrong since length of a utf16 string != length of a utf8 string, and
we don't need to append a null terminator.. so my logic below using
'len' is wrong. I'll fix that in the next version.
>> + int i;
>> +
>> + *utf16_to_utf8(utf8, str, len) = '\0';
>> +
>> + if (!(flags & LEFT))
>> + while (len < field_width--)
>> + ADDCH(buf, ' ');
>> + for (i = 0; i < len; ++i)
>> + ADDCH(buf, utf8[i]);
>> + while (len < field_width--)
>> + ADDCH(buf, ' ');
>> + return buf;
>> +}
>> +
>> #ifdef CONFIG_CMD_NET
>> static const char hex_asc[] = "0123456789abcdef";
>> #define hex_asc_lo(x)hex_asc[((x) & 0x0f)]
>> @@ -528,8 +549,13 @@ repeat:
>> continue;
>>
>> case 's':
>> - str = string(str, end, va_arg(args, char *),
>> - field_width, precision, flags);
>> + if (qualifier == 'l') {
>
> %ls refers to wchar with implementation dependent width in the C standard.
> There is no qualifier for 16-bit wchar. Couldn't we use %us here in
> reference to the u-notation ( u'MyString' ). This would leave the path
> open for a standard compliant '%ls'.
hmm, yeah, that is a clever idea, and I like it better than %A or %S..
so if we go the c11 route I'll do that. The c11 committee should have
thought of that ;-)
BR,
-R
> Best regards
>
> Heinrich
>
>> + str = string16(str, end, va_arg(args, u16 *),
>> +field_width, precision, flags);
>> + } else {
>> + str = string(str, end, va_arg(args, char *),
>> + field_width, precision, flags);
>> + }
>> continue;
>>
>> case 'p':
>>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, v0, 07/20] vsprintf.c: add wide string (%ls) support
On 08/04/2017 09:31 PM, Rob Clark wrote:
> This is convenient for efi_loader which deals a lot with utf16.
>
> Signed-off-by: Rob Clark
Please, put this patch together with
[PATCH] vsprintf.c: add GUID printing
https://patchwork.ozlabs.org/patch/798362/
and
[PATCH v0 06/20] common: add some utf16 handling helpers
https://patchwork.ozlabs.org/patch/797968/
into a separate patch series.
These three patches can be reviewed independently of the efi_loader
patches and probably will not be integrated via the efi-next tree.
> ---
> lib/vsprintf.c | 30 --
> 1 file changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 874a2951f7..0c40f852ce 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -17,6 +17,7 @@
> #include
>
> #include
> +#include
>
> #include
> #define noinline __attribute__((noinline))
> @@ -270,6 +271,26 @@ static char *string(char *buf, char *end, char *s, int
> field_width,
> return buf;
> }
>
> +static char *string16(char *buf, char *end, u16 *s, int field_width,
> + int precision, int flags)
> +{
> + u16 *str = s ? s : L"";
Please, do not use the L-notation here as it requires -fshort-wchar.
As we currently cannot switch the complete project to C11 you cannot use
the u-notation either.
> + int len = utf16_strnlen(str, precision);
> + u8 utf8[len * MAX_UTF8_PER_UTF16];
Didn't you forget 1 byte for \0 here?
This is what strlnlen does:
The strnlen() function returns the number of characters in the string
pointed to by s, **excluding** the terminating null byte ('\0'), but at
most maxlen.
I would expect the exclusion of the terminating null word by an
utf16_strnlen function.
> + int i;
> +
> + *utf16_to_utf8(utf8, str, len) = '\0';
> +
> + if (!(flags & LEFT))
> + while (len < field_width--)
> + ADDCH(buf, ' ');
> + for (i = 0; i < len; ++i)
> + ADDCH(buf, utf8[i]);
> + while (len < field_width--)
> + ADDCH(buf, ' ');
> + return buf;
> +}
> +
> #ifdef CONFIG_CMD_NET
> static const char hex_asc[] = "0123456789abcdef";
> #define hex_asc_lo(x)hex_asc[((x) & 0x0f)]
> @@ -528,8 +549,13 @@ repeat:
> continue;
>
> case 's':
> - str = string(str, end, va_arg(args, char *),
> - field_width, precision, flags);
> + if (qualifier == 'l') {
%ls refers to wchar with implementation dependent width in the C standard.
There is no qualifier for 16-bit wchar. Couldn't we use %us here in
reference to the u-notation ( u'MyString' ). This would leave the path
open for a standard compliant '%ls'.
Best regards
Heinrich
> + str = string16(str, end, va_arg(args, u16 *),
> +field_width, precision, flags);
> + } else {
> + str = string(str, end, va_arg(args, char *),
> + field_width, precision, flags);
> + }
> continue;
>
> case 'p':
>
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
