diff mbox

Fix compilation errors with libstdc++v3 for AVR target and allow --enable-libstdcxx

Message ID CADfx-VRqN12m4ONjegr8rmtU026s4v9Pqp4MizLnFY3Yy118Ug@mail.gmail.com
State Superseded
Headers show

Commit Message

Felipe Magno de Almeida Dec. 14, 2016, 9:38 p.m. UTC 
Hello Jonathan,

Sorry for the delay, I was in mid-vacation.

Comments are inline.

On Tue, Dec 6, 2016 at 3:45 PM, Jonathan Wakely <jwakely@redhat.com> wrote:
> On 16/09/16 02:53 -0300, Felipe Magno de Almeida wrote:

>>


[snip]

>> I've tried both approaches. Templates were causing problems of not

>> defined instantations because they were being used as ints too

>> in other _M_extract functions through a tmp integer. And typedef's

>> caused the same problem of having to use a tmp value of the right

>> type but for example _M_extract_wday_or_month could not have the

>> same type (in AVR they do) and I'd have to use a temporary anyway

>> then.

>>

>> This was the least intrusive way.

>

>

> Did you consider something like this?


I have, but I did not like how it could run code that is not very explicit
in case of exceptions, possibly assigning from a non-initialized variable,
causing, theoretically UB. Besides, __mem was already used for
tm_mom, so it seemed best to just mimic for the rest.

I have another patch attached which fixes the problem you mentioned
and the same problem with the tm_wday in a different switch-case.
I've removed all initializations of __mem, the same way that tm_mon
already did not initialize it, because the value is not actually used at
all in extract_num and variants.

I have ran the test and nothing on locale failed, I've ran again with the fixes,
and there are also no errors on locale. Testing with x86_64.

Changelog is:

2016-11-10  Felipe Magno de Almeida <felipe@expertisesolutions.com.br>

   * include/bits/locale_facets_nonio.tcc: Avoid compilation errors
   with non-standard struct tm.

> This should have no overhead for the targets with standard conforming

> struct tm, and only uses a temporary variable and additional

> assignments for AVR.

>

> It's ugly though.


Quite ugly, specially because of exceptions IMO.

Kind regards,
-- 
Felipe Magno de Almeida

Comments

Jonathan Wakely Dec. 15, 2016, 11:15 a.m. UTC | #1 
On 14/12/16 18:38 -0300, Felipe Magno de Almeida wrote:
>Hello Jonathan,

>

>Sorry for the delay, I was in mid-vacation.

>

>Comments are inline.

>

>On Tue, Dec 6, 2016 at 3:45 PM, Jonathan Wakely <jwakely@redhat.com> wrote:

>> On 16/09/16 02:53 -0300, Felipe Magno de Almeida wrote:

>>>

>

>[snip]

>

>>> I've tried both approaches. Templates were causing problems of not

>>> defined instantations because they were being used as ints too

>>> in other _M_extract functions through a tmp integer. And typedef's

>>> caused the same problem of having to use a tmp value of the right

>>> type but for example _M_extract_wday_or_month could not have the

>>> same type (in AVR they do) and I'd have to use a temporary anyway

>>> then.

>>>

>>> This was the least intrusive way.

>>

>>

>> Did you consider something like this?

>

>I have, but I did not like how it could run code that is not very explicit

>in case of exceptions, possibly assigning from a non-initialized variable,


OK, but that assignment from a possibly-uninitialized variable was
also present in your original patch.

>causing, theoretically UB. Besides, __mem was already used for

>tm_mom, so it seemed best to just mimic for the rest.


Yes, but now we have that overhead of extra reads and writes for every
function, not just that one.

>I have another patch attached which fixes the problem you mentioned

>and the same problem with the tm_wday in a different switch-case.

>I've removed all initializations of __mem, the same way that tm_mon

>already did not initialize it, because the value is not actually used at

>all in extract_num and variants.


Doesn't this now mean that if an error happens in the _M_extract_xxx
function we set tm.tm_xxx to garbage?

The existing tm.tm_mon case handles that correctly, by checking for an
error first.

>I have ran the test and nothing on locale failed, I've ran again with the fixes,

>and there are also no errors on locale. Testing with x86_64.


Good, thanks for testing it.
Jonathan Wakely Dec. 16, 2016, 12:45 p.m. UTC | #2 
On 15/12/16 21:41 -0300, Felipe Magno de Almeida wrote:
>Good point. Do you want me to update the patch with __men or go your way

>with the wrapper?


I think my wrapper's too ugly :-)

So please follow the same approach as for tm_mon (pass __mem to the
function, the copy it to tm_mon if there was no error). That adds a
write and a branch for every extracted field, which I hope shouldn't
have too much impact.
diff mbox

Patch

From a16f976cc02d5d8431923943ba3962733e17d13f Mon Sep 17 00:00:00 2001
From: Felipe Magno de Almeida <felipe@expertisesolutions.com.br>
Date: Thu, 15 Sep 2016 18:52:57 -0300
Subject: [PATCH] Use temporary int objects to access struct tm members

Call _M_extract_* functions family through temporary int objects, so
it doesn't convert from lvalue to rvalue through a temporary in AVR
because of the incompatible types used in AVR-Libc.

This fixes compilation errors with AVR-Libc while compiling libstdc++
for AVR target.
---
 libstdc++-v3/include/bits/locale_facets_nonio.tcc | 34 +++++++++++++++--------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/libstdc++-v3/include/bits/locale_facets_nonio.tcc b/libstdc++-v3/include/bits/locale_facets_nonio.tcc
index 1a4f9a0..cfbd381 100644
--- a/libstdc++-v3/include/bits/locale_facets_nonio.tcc
+++ b/libstdc++-v3/include/bits/locale_facets_nonio.tcc
@@ -659,30 +659,34 @@  _GLIBCXX_END_NAMESPACE_LDBL_OR_CXX11
 		  // Abbreviated weekday name [tm_wday]
 		  const char_type*  __days1[7];
 		  __tp._M_days_abbreviated(__days1);
-		  __beg = _M_extract_name(__beg, __end, __tm->tm_wday, __days1,
+		  __beg = _M_extract_name(__beg, __end, __mem, __days1,
 					  7, __io, __tmperr);
+                  __tm->tm_wday = __mem;
 		  break;
 		case 'A':
 		  // Weekday name [tm_wday].
 		  const char_type*  __days2[7];
 		  __tp._M_days(__days2);
-		  __beg = _M_extract_name(__beg, __end, __tm->tm_wday, __days2,
+		  __beg = _M_extract_name(__beg, __end, __mem, __days2,
 					  7, __io, __tmperr);
+                  __tm->tm_wday = __mem;
 		  break;
 		case 'h':
 		case 'b':
 		  // Abbreviated month name [tm_mon]
 		  const char_type*  __months1[12];
 		  __tp._M_months_abbreviated(__months1);
-		  __beg = _M_extract_name(__beg, __end, __tm->tm_mon, 
+		  __beg = _M_extract_name(__beg, __end, __mem, 
 					  __months1, 12, __io, __tmperr);
+                  __tm->tm_mon = __mem;
 		  break;
 		case 'B':
 		  // Month name [tm_mon].
 		  const char_type*  __months2[12];
 		  __tp._M_months(__months2);
-		  __beg = _M_extract_name(__beg, __end, __tm->tm_mon, 
+		  __beg = _M_extract_name(__beg, __end, __mem, 
 					  __months2, 12, __io, __tmperr);
+                  __tm->tm_mon = __mem;
 		  break;
 		case 'c':
 		  // Default time and date representation.
@@ -693,18 +697,20 @@  _GLIBCXX_END_NAMESPACE_LDBL_OR_CXX11
 		  break;
 		case 'd':
 		  // Day [01, 31]. [tm_mday]
-		  __beg = _M_extract_num(__beg, __end, __tm->tm_mday, 1, 31, 2,
+		  __beg = _M_extract_num(__beg, __end, __mem, 1, 31, 2,
 					 __io, __tmperr);
+                  __tm->tm_mday = __mem;
 		  break;
 		case 'e':
 		  // Day [1, 31], with single digits preceded by
 		  // space. [tm_mday]
 		  if (__ctype.is(ctype_base::space, *__beg))
-		    __beg = _M_extract_num(++__beg, __end, __tm->tm_mday, 1, 9,
+		    __beg = _M_extract_num(++__beg, __end, __mem, 1, 9,
 					   1, __io, __tmperr);
 		  else
-		    __beg = _M_extract_num(__beg, __end, __tm->tm_mday, 10, 31,
+		    __beg = _M_extract_num(__beg, __end, __mem, 10, 31,
 					   2, __io, __tmperr);
+                  __tm->tm_mday = __mem;
 		  break;
 		case 'D':
 		  // Equivalent to %m/%d/%y.[tm_mon, tm_mday, tm_year]
@@ -715,13 +721,15 @@  _GLIBCXX_END_NAMESPACE_LDBL_OR_CXX11
 		  break;
 		case 'H':
 		  // Hour [00, 23]. [tm_hour]
-		  __beg = _M_extract_num(__beg, __end, __tm->tm_hour, 0, 23, 2,
+		  __beg = _M_extract_num(__beg, __end, __mem, 0, 23, 2,
 					 __io, __tmperr);
+                  __tm->tm_hour = __mem;
 		  break;
 		case 'I':
 		  // Hour [01, 12]. [tm_hour]
-		  __beg = _M_extract_num(__beg, __end, __tm->tm_hour, 1, 12, 2,
+		  __beg = _M_extract_num(__beg, __end, __mem, 1, 12, 2,
 					 __io, __tmperr);
+                  __tm->tm_hour = __mem;
 		  break;
 		case 'm':
 		  // Month [01, 12]. [tm_mon]
@@ -732,8 +740,9 @@  _GLIBCXX_END_NAMESPACE_LDBL_OR_CXX11
 		  break;
 		case 'M':
 		  // Minute [00, 59]. [tm_min]
-		  __beg = _M_extract_num(__beg, __end, __tm->tm_min, 0, 59, 2,
+		  __beg = _M_extract_num(__beg, __end, __mem, 0, 59, 2,
 					 __io, __tmperr);
+                  __tm->tm_min = __mem;
 		  break;
 		case 'n':
 		  if (__ctype.narrow(*__beg, 0) == '\n')
@@ -752,11 +761,12 @@  _GLIBCXX_END_NAMESPACE_LDBL_OR_CXX11
 		  // Seconds. [tm_sec]
 		  // [00, 60] in C99 (one leap-second), [00, 61] in C89.
 #if _GLIBCXX_USE_C99
-		  __beg = _M_extract_num(__beg, __end, __tm->tm_sec, 0, 60, 2,
+		  __beg = _M_extract_num(__beg, __end, __mem, 0, 60, 2,
 #else
-		  __beg = _M_extract_num(__beg, __end, __tm->tm_sec, 0, 61, 2,
+		  __beg = _M_extract_num(__beg, __end, __mem, 0, 61, 2,
 #endif
 					 __io, __tmperr);
+                  __tm->tm_sec = __mem;
 		  break;
 		case 't':
 		  if (__ctype.narrow(*__beg, 0) == '\t')
-- 
2.10.2