eCos uSTL 1.3 port for review

eCos uSTL 1.3 port for review

[cetoni GmbH - Uwe Kindler]

Hi,

Mike Sharov released the new version 1.3 of the uSTL library two days 
ago. I updated the eCos uSTL port to this new version. The port is 
available here for review:

http://www.cetoni.de/ecos_ustl20090806.zip

I changed some further stuff we dicussed on the mailing list:

- the library now builds without -Wundef - so I could keep the changes
to the uSTL 1.3 sources to a minimum

- new stdint.h file - i created a stdint.h file from the newlib stdint.h 
mentioned in this message 
http://ecos.sourceware.org/ml/ecos-patches/2009-08/msg00008.html that 
should be more portable


If the uSTL package is compiled with filestreams support then building 
test cases succeeds. If filestrams are disabled and package File I/O is 
not present then the build error mentioned in 
http://ecos.sourceware.org/ml/ecos-devel/2009-08/msg00000.html occures 
when building eCos test cases.

I would kindly ask you to review this port to tell me things that should 
be changed prior to CVS import of this package.

Regards, Uwe

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

Uwe Kindler wrote:
> Hi,
> 
> Mike Sharov released the new version 1.3 of the uSTL library two days 
> ago. I updated the eCos uSTL port to this new version. The port is 
> available here for review:
> 
> http://www.cetoni.de/ecos_ustl20090806.zip
> 
> I changed some further stuff we dicussed on the mailing list:
> 
> - the library now builds without -Wundef - so I could keep the changes
> to the uSTL 1.3 sources to a minimum
> 
> - new stdint.h file - i created a stdint.h file from the newlib stdint.h 
> mentioned in this message 
> http://ecos.sourceware.org/ml/ecos-patches/2009-08/msg00008.html that 
> should be more portable
> 
> 
> If the uSTL package is compiled with filestreams support then building 
> test cases succeeds. If filestrams are disabled and package File I/O is 
> not present then the build error mentioned in 
> http://ecos.sourceware.org/ml/ecos-devel/2009-08/msg00000.html occures 
> when building eCos test cases.
> 
> I would kindly ask you to review this port to tell me things that should 
> be changed prior to CVS import of this package.

Hi Uwe

Thank you for your contribution! Just now I tested your latest stuff
with your libc patch together against clean CVS

i. arm-eabi toolchain

A) uSTL package was built successfully with 1 warning. That's was

diff --git a/packages/language/cxx/ustl/current/src/uexception.cpp b/packages/language/cxx/ustl/current/src/uexception.cpp
--- a/packages/language/cxx/ustl/current/src/uexception.cpp
+++ b/packages/language/cxx/ustl/current/src/uexception.cpp
@@ -35,7 +35,7 @@ void exception::info (string& msgbuf, co
 /// Reads the exception from stream \p is.
 void exception::read (istream& is)
 {
-    uint32_t stmSize;
+    uint32_t stmSize = 0;
     xfmt_t fmt = xfmt_Exception;
     is >> fmt >> stmSize >> m_Backtrace;
     assert (fmt == m_Format && "The saved exception is of a different type.");

B) All test passed, but: btv05, btv16 (my target has not enough memory)
and bvt25 (I didn't turn on filestream support);


ii. i386-elf toolchain

A) All tests were built with this quick tweak:

diff --git a/packages/language/cxx/ustl/current/src/bktrace.cpp b/packages/language/cxx/ustl/current/src/bktrace.cpp
--- a/packages/language/cxx/ustl/current/src/bktrace.cpp
+++ b/packages/language/cxx/ustl/current/src/bktrace.cpp
@@ -6,7 +6,7 @@
 #include "bktrace.h"
 #include "sostream.h"
 #include "mistream.h"
-#if linux && __GNUC__
+#if 0 // linux && __GNUC__
     #include <execinfo.h>
 #else
     static inline int backtrace (void**, int)			{ return (0); }

Certainly, It can be fixed more elegant with some ifdef  __ECOS__, if
you would place in CDL

    cdl_option CYGPKG_USTL_CFLAGS_ADD {
        ...
        default_value -D__ECOS__
    }

B) Unfortunately, I got SIGSEG in vsnprintf(). It seems, I should
investigate more time for this issue, may be I missed something.

Had you success with uSTL on synthetic target? All tests I built as

ecosconfig new <target> ustl

Once again, thank you for this package.

Kind regards,

Sergei

Re: eCos uSTL 1.3 port for review

[cetoni GmbH - Uwe Kindler]

Hello Sergei,

 > B) Unfortunately, I got SIGSEG in vsnprintf(). It seems, I should
 > investigate more time for this issue, may be I missed something.
 >
 > Had you success with uSTL on synthetic target? All tests I built as
 >
 > ecosconfig new <target> ustl
 >
 > Once again, thank you for this package.
 >
 > Kind regards,
 >
 > Sergei

thank you very much for testing this package. Unfortunately I do not 
have a running linux system here and have never used the synthetic 
target before. So I can test uSTL only with arm-eabi toolchain.

It would be really great, If you could investigate a little bit more, 
what causes the SIGSEG for synthetic target. Would you be so kind to run 
the new snprintf_c99.c test on synthetic target when C99 compliance 
activated to check if my vfnprintf() patch is working properly?

This would be really great.

If there would be a preconfigured VM linux image available on the eCos 
homapage with all tools installed and a working synthetic target I would 
immediatelly start testing ;O)

Kind regards, Uwe

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

Uwe Kindler wrote:
> It would be really great, If you could investigate a little bit more, 
> what causes the SIGSEG for synthetic target. Would you be so kind to run 
> the new snprintf_c99.c test on synthetic target when C99 compliance 
> activated to check if my vfnprintf() patch is working properly?
 
Uwe, 

snprintf_c99 test passed successfully on synth. It seems that I should
grep all  #if linux in ustl and look on that. I will try to rebuild ustl
with assertions and will try to re-run the tests. I'll let you know what
I will get then.

Sergei

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

On Thu, Aug 06, 2009 at 03:44:16PM +0300, Sergei Gavrikov wrote:
> Uwe Kindler wrote:
> > It would be really great, If you could investigate a little bit more, 
> > what causes the SIGSEG for synthetic target. Would you be so kind to run 
> > the new snprintf_c99.c test on synthetic target when C99 compliance 
> > activated to check if my vfnprintf() patch is working properly?
>  
> Uwe, 
> 
> snprintf_c99 test passed successfully on synth. It seems that I should
> grep all  #if linux in ustl and look on that. I will try to rebuild ustl
> with assertions and will try to re-run the tests. I'll let you know what
> I will get then.

Hm, Uwe, SIGSEG occured on a call of a destructor in vsnprintf.cxx (it's
not your source). The uSTL test run on synthetic target with this dirty
tweak. Do you have any thoughts?

diff -u -5 -r1.6 vsnprintf.cxx
--- src/common/vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
+++ src/common/vsnprintf.cxx	6 Aug 2009 13:13:35 -0000
@@ -69,11 +69,11 @@
 class Cyg_VsnprintfStream: public Cyg_OutputStream
 {
 public:
     Cyg_VsnprintfStream(char* s): s_(s) {}
 
-    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
+    virtual ~Cyg_VsnprintfStream() { /**s_ = '\0';*/ }
 
     virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
         cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );
 
     virtual Cyg_ErrNo get_error( void ) { return ENOERR; }


Sergei

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

On Thu, Aug 06, 2009 at 04:25:11PM +0300, Sergei Gavrikov wrote:
> On Thu, Aug 06, 2009 at 03:44:16PM +0300, Sergei Gavrikov wrote:
> > Uwe Kindler wrote:
> > > It would be really great, If you could investigate a little bit more, 
> > > what causes the SIGSEG for synthetic target. Would you be so kind to run 
> > > the new snprintf_c99.c test on synthetic target when C99 compliance 
> > > activated to check if my vfnprintf() patch is working properly?
> >  
> > Uwe, 
> > 
> > snprintf_c99 test passed successfully on synth. It seems that I should
> > grep all  #if linux in ustl and look on that. I will try to rebuild ustl
> > with assertions and will try to re-run the tests. I'll let you know what
> > I will get then.
> 
> Hm, Uwe, SIGSEG occured on a call of a destructor in vsnprintf.cxx (it's
> not your source). The uSTL test run on synthetic target with this dirty
> tweak. Do you have any thoughts?
 
With this patch both set of tests libc/stdio and cxx/ustl passed
successfully on synthetic target.

Sergei

[-- Attachment #2: vsnprintf.cxx.patch --]
[-- Type: text/x-diff, Size: 635 bytes --]

Index: vsnprintf.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
retrieving revision 1.6
diff -u -r1.6 vsnprintf.cxx
--- vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
+++ vsnprintf.cxx	6 Aug 2009 13:49:24 -0000
@@ -71,7 +71,7 @@
 public:
     Cyg_VsnprintfStream(char* s): s_(s) {}
 
-    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
+    virtual ~Cyg_VsnprintfStream() { if (s_) *s_ = '\0'; }
 
     virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
         cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );

Re: eCos uSTL 1.3 port for review

[cetoni GmbH - Uwe Kindler]

Hi Sergei,

 > Uwe, now all works fine on synthetic with that vsnprintf.cxx.patch (your
 > C99 snprintf runs successfully on synthetic). My minor tweaks were (see
 > an attachement) to manage uSTL tests on synthetic.
 >
 > What do you think about -D__ECOS__=1 in pkg ADD_FLAGS instead those #if
 > 0?

thank you very much for both patches. I think adding -D__ECOS__ would be 
the solution.

Could you please build uSTL package + tests for synthetic target without 
filestreams support (CYGCLS_USTL_FSTREAMS diables) and without the File 
I/O package.

Building test with arm-eabi toolchain fails for this configuration 
because of this problem 
(http://ecos.sourceware.org/ml/ecos-devel/2009-08/msg00000.html). Can 
you reproduce this with i386 toolchain?

Thany you for you efforts.

Regards, Uwe


Sergei Gavrikov schrieb:

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

On Fri, Aug 07, 2009 at 07:53:54AM +0200, cetoni GmbH - Uwe Kindler wrote:
> Hi Sergei,
> 
> > Uwe, now all works fine on synthetic with that vsnprintf.cxx.patch (your
> > C99 snprintf runs successfully on synthetic). My minor tweaks were (see
> > an attachement) to manage uSTL tests on synthetic.
> >
> > What do you think about -D__ECOS__=1 in pkg ADD_FLAGS instead those #if
> > 0?
> 
> thank you very much for both patches. I think adding -D__ECOS__ would be 
> the solution.
> 
> Could you please build uSTL package + tests for synthetic target without 
> filestreams support (CYGCLS_USTL_FSTREAMS diables) and without the File 
> I/O package.

Hi Uwe,

First, when I reject fileio package, I cannot build eCos using ustl
template for linux synthetic target at all. The reason is

error: cyg/fileio/limits.h: No such file or directory

If I just disable CYGCLS_USTL_FSTREAMS option, and do not use ustl
template, but, add ustl package for default template, I can build eCos.
But, unfortunately, ustl tests leave eCos test farm they depend on
CYGVAR_USTL_CIN_COUT_CERR != 0, but that is 0 (cause is disabling
CYGCLS_USTL_FSTREAMS).

> Building test with arm-eabi toolchain fails for this configuration 
> because of this problem 
> (http://ecos.sourceware.org/ml/ecos-devel/2009-08/msg00000.html). Can 
> you reproduce this with i386 toolchain?

Infra tests build/run itself with optimization -O2, but, without 
optimization (-O0) build fails: __cxa_pure_virtual issue.

When I remove ustl package and build infra tests without optimization,
the issue leave the box. Well, I confirm the issue with i386-elf
toolchain from eCosCentric on synthetic target too.

HIH.

Regards,
Sergei

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

On Fri, Aug 07, 2009 at 10:14:54AM +0300, Sergei Gavrikov wrote:
> Infra tests build/run itself with optimization -O2, but, without 
> optimization (-O0) build fails: __cxa_pure_virtual issue.
> 
> When I remove ustl package and build infra tests without optimization,
  ^^^^^^^^^^^^^^^^^^^^^^^^^^
In fact libsupc++.a (= __cxa_pure_virtual) is not linked then

> the issue leave the box. Well, I confirm the issue with i386-elf
> toolchain from eCosCentric on synthetic target too.

Uwe, I "found" a dirty workaround for the issue using GCC options :-)

    -Wl,--defsym -Wl,__cxa_pure_virtual=0

So, my flags become

cdl_option CYGBLD_GLOBAL_CFLAGS {
    user_value "-Wall -Wpointer-arith -Wstrict-prototypes -Wundef -Woverloaded-virtual -Wno-write-strings -g -O0 -ffunction-sections -fdata-sections -fno-rtti -fno-exceptions -Wl,--defsym -Wl,__cxa_pure_virtual=0 "
};

... and it was possible to build diag_sprintf{1,2} with added ustl.

Well, it seems for me it can be solved more elegant. And that is not
ustl/snprintf issue.

Sergei

Re: eCos uSTL 1.3 port for review

[cetoni GmbH - Uwe Kindler]

Hello Sergei.

 >> When I remove ustl package and build infra tests without optimization,
 >   ^^^^^^^^^^^^^^^^^^^^^^^^^^
 > In fact libsupc++.a (= __cxa_pure_virtual) is not linked then
 >
 >> the issue leave the box. Well, I confirm the issue with i386-elf
 >> toolchain from eCosCentric on synthetic target too.
 >
 > Uwe, I "found" a dirty workaround for the issue using GCC options :-)
 >
 >     -Wl,--defsym -Wl,__cxa_pure_virtual=0
 >
 > So, my flags become
 >
 > cdl_option CYGBLD_GLOBAL_CFLAGS {
 >     user_value "-Wall -Wpointer-arith -Wstrict-prototypes -Wundef 
-Woverloaded-virtual -Wno-write-strings -g -O0 -ffunction-sections 
-fdata-sections -fno-rtti -fno-exceptions -Wl,--defsym 
-Wl,__cxa_pure_virtual=0 "
 > };
 >
 > ... and it was possible to build diag_sprintf{1,2} with added ustl.
 >
 > Well, it seems for me it can be solved more elegant. And that is not
 > ustl/snprintf issue.
 >
 > Sergei

thank you for your investigation and for your detailed error report. 
Does someone of the eCos folks know a better way to solve the 
__cxa_pure_virtual issue or should we go with Sergei's proposal

-Wl,--defsym -Wl,__cxa_pure_virtual=0

Regards, Uwe

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

Uwe

Today when I do re-test all from scratch (ustl build on synthetic), I
found that this patch does not work. It seems that I ran in false clean
up.  The previous tweak:

    virtual ~Cyg_VsnprintfStream() { /* *s_ = '\0'; */ }

works only. I cannot know why this assignment

    *s_ = '\0';

there :-(

So, such the desctuctor (the below) works for synthetic ustl build

    virtual ~Cyg_VsnprintfStream() {}

I'm sorry about. It seems we should ask Jonathan Larmour about this
terminator.

Sergei

On Thu, Aug 06, 2009 at 04:56:03PM +0300, Sergei Gavrikov wrote:
> On Thu, Aug 06, 2009 at 04:25:11PM +0300, Sergei Gavrikov wrote:
> > On Thu, Aug 06, 2009 at 03:44:16PM +0300, Sergei Gavrikov wrote:
> > > Uwe Kindler wrote:
> > > > It would be really great, If you could investigate a little bit more, 
> > > > what causes the SIGSEG for synthetic target. Would you be so kind to run 
> > > > the new snprintf_c99.c test on synthetic target when C99 compliance 
> > > > activated to check if my vfnprintf() patch is working properly?
> > >  
> > > Uwe, 
> > > 
> > > snprintf_c99 test passed successfully on synth. It seems that I should
> > > grep all  #if linux in ustl and look on that. I will try to rebuild ustl
> > > with assertions and will try to re-run the tests. I'll let you know what
> > > I will get then.
> > 
> > Hm, Uwe, SIGSEG occured on a call of a destructor in vsnprintf.cxx (it's
> > not your source). The uSTL test run on synthetic target with this dirty
> > tweak. Do you have any thoughts?
>  
> With this patch both set of tests libc/stdio and cxx/ustl passed
> successfully on synthetic target.
> 
> Sergei

> Index: vsnprintf.cxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
> retrieving revision 1.6
> diff -u -r1.6 vsnprintf.cxx
> --- vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
> +++ vsnprintf.cxx	6 Aug 2009 13:49:24 -0000
> @@ -71,7 +71,7 @@
>  public:
>      Cyg_VsnprintfStream(char* s): s_(s) {}
>  
> -    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
> +    virtual ~Cyg_VsnprintfStream() { if (s_) *s_ = '\0'; }
>  
>      virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
>          cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );

Re: eCos uSTL 1.3 port for review

[John Dallaway]

Hi Uwe

Uwe Kindler wrote:

> thank you for your investigation and for your detailed error report.
> Does someone of the eCos folks know a better way to solve the
> __cxa_pure_virtual issue or should we go with Sergei's proposal
> 
> -Wl,--defsym -Wl,__cxa_pure_virtual=0

CYGPKG_INFRA is supposed to provide a replacement implementation of this
function. Did you try adding your own implementation of
__cxxabiv1::__cxa_pure_virtual() as discussed on the ecos-devel list?

John Dallaway

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

John Dallaway wrote:
> Hi Uwe
> 
> Uwe Kindler wrote:
> 
> > thank you for your investigation and for your detailed error report.
> > Does someone of the eCos folks know a better way to solve the
> > __cxa_pure_virtual issue or should we go with Sergei's proposal
> > 
> > -Wl,--defsym -Wl,__cxa_pure_virtual=0
> 
> CYGPKG_INFRA is supposed to provide a replacement implementation of this
> function. Did you try adding your own implementation of
> __cxxabiv1::__cxa_pure_virtual() as discussed on the ecos-devel list?

Hi Jonh,

Are such implementations (in pure.cxx) useful? I'm looking on linker
group

    GROUP(libtarget.a libgcc.a libsupc++.a)

    $ nm install/lib/libtarget.a | grep __cxa_pure_virtual
    00000000 T __cxa_pure_virtual

Okay it came there from infra_pure.o. Is it right? But, there is next
implementation in libsupc++.a.

    $ nm i386-elf/i386-elf/lib/libsupc++.a 2>/dev/null | grep __cxa_pure_virtual
             U __cxa_pure_virtual
    00000000 T __cxa_pure_virtual

I think arm-eabi chain is the same.

RFC: What kind of implementation will be used then? Well, using 'ar x' I
would answer :-) but may be you just know.

Thanks for any clarifications,

Sergei

Re: eCos uSTL 1.3 port for review

[Jonathan Larmour]

Sergei Gavrikov wrote:
> John Dallaway wrote:
> 
>>Hi Uwe
>>
>>Uwe Kindler wrote:
>>
>>
>>>thank you for your investigation and for your detailed error report.
>>>Does someone of the eCos folks know a better way to solve the
>>>__cxa_pure_virtual issue or should we go with Sergei's proposal
>>>
>>>-Wl,--defsym -Wl,__cxa_pure_virtual=0
>>
>>CYGPKG_INFRA is supposed to provide a replacement implementation of this
>>function. Did you try adding your own implementation of
>>__cxxabiv1::__cxa_pure_virtual() as discussed on the ecos-devel list?
> 
> 
> Hi Jonh,
> 
> Are such implementations (in pure.cxx) useful? I'm looking on linker
> group
> 
>     GROUP(libtarget.a libgcc.a libsupc++.a)
> 
>     $ nm install/lib/libtarget.a | grep __cxa_pure_virtual
>     00000000 T __cxa_pure_virtual
> 
> Okay it came there from infra_pure.o. Is it right? But, there is next
> implementation in libsupc++.a.
> 
>     $ nm i386-elf/i386-elf/lib/libsupc++.a 2>/dev/null | grep __cxa_pure_virtual
>              U __cxa_pure_virtual
>     00000000 T __cxa_pure_virtual

However since libtarget.a is mentioned first in the GROUP() it should use 
that first, before libsupc++. If that isn't happening, something odd is 
going on - generate a linker map to see.

Jifl
-- 
--["No sense being pessimistic, it wouldn't work anyway"]-- Opinions==mine

Re: eCos uSTL 1.3 port for review

[Jonathan Larmour]

[-- Attachment #1: Type: text/plain, Size: 3133 bytes --]

Sergei Gavrikov wrote:
> Uwe
> 
> Today when I do re-test all from scratch (ustl build on synthetic), I
> found that this patch does not work. It seems that I ran in false clean
> up.  The previous tweak:
> 
>     virtual ~Cyg_VsnprintfStream() { /* *s_ = '\0'; */ }
> 
> works only. I cannot know why this assignment
> 
>     *s_ = '\0';
> 
> there :-(
> 
> So, such the desctuctor (the below) works for synthetic ustl build
> 
>     virtual ~Cyg_VsnprintfStream() {}
> 
> I'm sorry about. It seems we should ask Jonathan Larmour about this
> terminator.

I don't believe vfnprintf() adds trailing null terminators in its output, 
but the string needs one, so this is how it's added. Otherwise you are 
gambling on the memory pointed to by the string already having NULLs in. I 
suspect when you've had this working, that has happened to be the case.

I think I know what the real problem is - if the string is truncated, the 
NULL will be placed after the "size" argument. Please try the attached 
untested patch (not yet committed).

Jifl

> On Thu, Aug 06, 2009 at 04:56:03PM +0300, Sergei Gavrikov wrote:
> 
>>On Thu, Aug 06, 2009 at 04:25:11PM +0300, Sergei Gavrikov wrote:
>>
>>>On Thu, Aug 06, 2009 at 03:44:16PM +0300, Sergei Gavrikov wrote:
>>>
>>>>Uwe Kindler wrote:
>>>>
>>>>>It would be really great, If you could investigate a little bit more, 
>>>>>what causes the SIGSEG for synthetic target. Would you be so kind to run 
>>>>>the new snprintf_c99.c test on synthetic target when C99 compliance 
>>>>>activated to check if my vfnprintf() patch is working properly?
>>>>
>>>> 
>>>>Uwe, 
>>>>
>>>>snprintf_c99 test passed successfully on synth. It seems that I should
>>>>grep all  #if linux in ustl and look on that. I will try to rebuild ustl
>>>>with assertions and will try to re-run the tests. I'll let you know what
>>>>I will get then.
>>>
>>>Hm, Uwe, SIGSEG occured on a call of a destructor in vsnprintf.cxx (it's
>>>not your source). The uSTL test run on synthetic target with this dirty
>>>tweak. Do you have any thoughts?
>>
>> 
>>With this patch both set of tests libc/stdio and cxx/ustl passed
>>successfully on synthetic target.
>>
>>Sergei
> 
> 
>>Index: vsnprintf.cxx
>>===================================================================
>>RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
>>retrieving revision 1.6
>>diff -u -r1.6 vsnprintf.cxx
>>--- vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
>>+++ vsnprintf.cxx	6 Aug 2009 13:49:24 -0000
>>@@ -71,7 +71,7 @@
>> public:
>>     Cyg_VsnprintfStream(char* s): s_(s) {}
>> 
>>-    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
>>+    virtual ~Cyg_VsnprintfStream() { if (s_) *s_ = '\0'; }
>> 
>>     virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
>>         cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );
> 
> 
> 
> 


-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["The best things in life aren't things."]------      Opinions==mine

[-- Attachment #2: vsnprintf.trunc.patch --]
[-- Type: text/x-patch, Size: 1944 bytes --]

Index: ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/ChangeLog,v
retrieving revision 1.47
diff -u -5 -p -r1.47 ChangeLog
--- ChangeLog	16 Feb 2009 21:41:12 -0000	1.47
+++ ChangeLog	7 Aug 2009 13:40:26 -0000
@@ -1,5 +1,11 @@
+2009-08-07  Jonathan Larmour  <jifl@eCosCentric.com>
+
+	* src/common/vsnprintf.cxx (class Cyg_VsnprintfStream):
+	Remove destructor.
+	(vsnprintf): Apply null terminator correctly when truncated.
+
 2009-02-16  Lars Povlsen  <lpovlsen@vitesse.com>
 
 	* include/stdio.h: Removed extra semicolon after scanf()
 	prototype.
 
Index: src/common/vsnprintf.cxx
===================================================================
RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
retrieving revision 1.6
diff -u -5 -p -r1.6 vsnprintf.cxx
--- src/common/vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
+++ src/common/vsnprintf.cxx	7 Aug 2009 13:40:26 -0000
@@ -69,12 +69,10 @@
 class Cyg_VsnprintfStream: public Cyg_OutputStream
 {
 public:
     Cyg_VsnprintfStream(char* s): s_(s) {}
 
-    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
-
     virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
         cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );
 
     virtual Cyg_ErrNo get_error( void ) { return ENOERR; }
 
@@ -99,10 +97,17 @@ Cyg_VsnprintfStream::write(
 }
 
 externC int
 vsnprintf( char *s, size_t size, const char *format, va_list arg ) __THROW
 {
+    int ret;
     Cyg_VsnprintfStream stream(s);
-    return vfnprintf( (FILE *)(void *)&stream, size, format, arg );
+    ret = vfnprintf( (FILE *)(void *)&stream, size, format, arg );
+    /* If no error, and string not truncated, then apply null termination in
+     * correct place
+     */
+    if ( (ret >= 0) && (ret < size) )
+        s[ret] = '\0';
+    return ret;
 } // vsnprintf()
 
 // EOF vsnprintf.cxx

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

Hello Jonathan, 

I tested the patch, all ustl tests run smoothly for synthetic target
after applying (no SIGSEG). Thank you!

Sergei

Jonathan Larmour wrote:
> Sergei Gavrikov wrote:
>> Uwe
>>
>> Today when I do re-test all from scratch (ustl build on synthetic), I
>> found that this patch does not work. It seems that I ran in false clean
>> up.  The previous tweak:
>>
>>     virtual ~Cyg_VsnprintfStream() { /* *s_ = '\0'; */ }
>>
>> works only. I cannot know why this assignment
>>
>>     *s_ = '\0';
>>
>> there :-(
>>
>> So, such the desctuctor (the below) works for synthetic ustl build
>>
>>     virtual ~Cyg_VsnprintfStream() {}
>>
>> I'm sorry about. It seems we should ask Jonathan Larmour about this
>> terminator.
>
> I don't believe vfnprintf() adds trailing null terminators in its output, 
> but the string needs one, so this is how it's added. Otherwise you are  
> gambling on the memory pointed to by the string already having NULLs in. 
> I suspect when you've had this working, that has happened to be the case.
>
> I think I know what the real problem is - if the string is truncated, the 
> NULL will be placed after the "size" argument. Please try the attached  
> untested patch (not yet committed).
>
> Jifl
>
>> On Thu, Aug 06, 2009 at 04:56:03PM +0300, Sergei Gavrikov wrote:
>>
>>> On Thu, Aug 06, 2009 at 04:25:11PM +0300, Sergei Gavrikov wrote:
>>>
>>>> On Thu, Aug 06, 2009 at 03:44:16PM +0300, Sergei Gavrikov wrote:
>>>>
>>>>> Uwe Kindler wrote:
>>>>>
>>>>>> It would be really great, If you could investigate a little bit 
>>>>>> more, what causes the SIGSEG for synthetic target. Would you be 
>>>>>> so kind to run the new snprintf_c99.c test on synthetic target 
>>>>>> when C99 compliance activated to check if my vfnprintf() patch 
>>>>>> is working properly?
>>>>>
>>>>>
>>>>> Uwe, 
>>>>>
>>>>> snprintf_c99 test passed successfully on synth. It seems that I should
>>>>> grep all  #if linux in ustl and look on that. I will try to rebuild ustl
>>>>> with assertions and will try to re-run the tests. I'll let you know what
>>>>> I will get then.
>>>>
>>>> Hm, Uwe, SIGSEG occured on a call of a destructor in vsnprintf.cxx (it's
>>>> not your source). The uSTL test run on synthetic target with this dirty
>>>> tweak. Do you have any thoughts?
>>>
>>>
>>> With this patch both set of tests libc/stdio and cxx/ustl passed
>>> successfully on synthetic target.
>>>
>>> Sergei
>>
>>
>>> Index: vsnprintf.cxx
>>> ===================================================================
>>> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
>>> retrieving revision 1.6
>>> diff -u -r1.6 vsnprintf.cxx
>>> --- vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
>>> +++ vsnprintf.cxx	6 Aug 2009 13:49:24 -0000
>>> @@ -71,7 +71,7 @@
>>> public:
>>>     Cyg_VsnprintfStream(char* s): s_(s) {}
>>>
>>> -    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
>>> +    virtual ~Cyg_VsnprintfStream() { if (s_) *s_ = '\0'; }
>>>
>>>     virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
>>>         cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );
>>
>>
>>
>>
>
>
> -- 
> eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
> Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
> Registered in England and Wales: Reg No 4422071.
> ------["The best things in life aren't things."]------      Opinions==mine

> Index: ChangeLog
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/ChangeLog,v
> retrieving revision 1.47
> diff -u -5 -p -r1.47 ChangeLog
> --- ChangeLog	16 Feb 2009 21:41:12 -0000	1.47
> +++ ChangeLog	7 Aug 2009 13:40:26 -0000
> @@ -1,5 +1,11 @@
> +2009-08-07  Jonathan Larmour  <jifl@eCosCentric.com>
> +
> +	* src/common/vsnprintf.cxx (class Cyg_VsnprintfStream):
> +	Remove destructor.
> +	(vsnprintf): Apply null terminator correctly when truncated.
> +
>  2009-02-16  Lars Povlsen  <lpovlsen@vitesse.com>
>  
>  	* include/stdio.h: Removed extra semicolon after scanf()
>  	prototype.
>  
> Index: src/common/vsnprintf.cxx
> ===================================================================
> RCS file: /cvs/ecos/ecos/packages/language/c/libc/stdio/current/src/common/vsnprintf.cxx,v
> retrieving revision 1.6
> diff -u -5 -p -r1.6 vsnprintf.cxx
> --- src/common/vsnprintf.cxx	29 Jan 2009 17:49:53 -0000	1.6
> +++ src/common/vsnprintf.cxx	7 Aug 2009 13:40:26 -0000
> @@ -69,12 +69,10 @@
>  class Cyg_VsnprintfStream: public Cyg_OutputStream
>  {
>  public:
>      Cyg_VsnprintfStream(char* s): s_(s) {}
>  
> -    virtual ~Cyg_VsnprintfStream() { *s_ = '\0'; }
> -
>      virtual Cyg_ErrNo write( const cyg_uint8 *buffer,
>          cyg_ucount32 buffer_length, cyg_ucount32 *bytes_written );
>  
>      virtual Cyg_ErrNo get_error( void ) { return ENOERR; }
>  
> @@ -99,10 +97,17 @@ Cyg_VsnprintfStream::write(
>  }
>  
>  externC int
>  vsnprintf( char *s, size_t size, const char *format, va_list arg ) __THROW
>  {
> +    int ret;
>      Cyg_VsnprintfStream stream(s);
> -    return vfnprintf( (FILE *)(void *)&stream, size, format, arg );
> +    ret = vfnprintf( (FILE *)(void *)&stream, size, format, arg );
> +    /* If no error, and string not truncated, then apply null termination in
> +     * correct place
> +     */
> +    if ( (ret >= 0) && (ret < size) )
> +        s[ret] = '\0';
> +    return ret;
>  } // vsnprintf()
>  
>  // EOF vsnprintf.cxx

Re: eCos uSTL 1.3 port for review

[Sergei Gavrikov]

On Fri, Aug 07, 2009 at 07:01:08PM +0300, Sergei Gavrikov wrote:
> Hello Jonathan, 
> 
> I tested the patch, all ustl tests run smoothly for synthetic target
> after applying (no SIGSEG). Thank you!
> 
> Sergei

vsnprintf.cxx:108: warning: comparison between signed and unsigned integer expressions

Jonathan, can you, please, peace it before to check in?

Sergei

Re: eCos uSTL 1.3 port for review

[Jonathan Larmour]

Sergei Gavrikov wrote:
> On Fri, Aug 07, 2009 at 07:01:08PM +0300, Sergei Gavrikov wrote:
> 
>>Hello Jonathan, 
>>
>>I tested the patch, all ustl tests run smoothly for synthetic target
>>after applying (no SIGSEG). Thank you!
>>
>>Sergei
> 
> 
> vsnprintf.cxx:108: warning: comparison between signed and unsigned integer expressions
> 
> Jonathan, can you, please, peace it before to check in?

Done, and committed.

Jifl
-- 
eCosCentric Limited      http://www.eCosCentric.com/     The eCos experts
Barnwell House, Barnwell Drive, Cambridge, UK.       Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.
------["The best things in life aren't things."]------      Opinions==mine