[PATCH 1/6] generated character data for libc/ctype

Makefile add-ons for both patch series (libc/string and libc/ctype) will
be sent separately.

Thomas Wolff

2018-03-07 23:20:26 UTC

Thomas Wolff

2018-03-07 23:20:45 UTC

Corinna Vinschen

2018-03-08 08:04:26 UTC

From 15999d30c011be9041821456d23807249981dd86 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:11:44 +0100
Subject: [PATCH 3/6] remove hard-coded character data

Sorry, but no. I like the idea of this and the followup patch in that
you call, e.g., iswalpha_l from iswalpha, rather than vice versa as
today. However, don't delete and re-introduce files, just overwrite
them with the new code, for sake of a sane history.

Also, these two patches(*) should get a descriptive text in the git log.
Especially the fact that it turns around the calling order is worth a
couple of words.

Thanks,
Corinna

(*) ...which should actually be only one as outlined above. The
easiest way to accomplish that is probably an interactive rebase
with squashing the second patch into the first.

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-08 22:05:22 UTC

From 15999d30c011be9041821456d23807249981dd86 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:11:44 +0100
Subject: [PATCH 3/6] remove hard-coded character data

Sorry, but no. ... don't delete and re-introduce files, just overwrite
them with the new code, for sake of a sane history.

The only reason for the split was that I recall a 64KB mail size
limitation on this mailing list;
given that git format-patch quotes all deleted lines, this would have
been exceeded,
so how should we handle this? Would format-patch option -D be
acceptable? Can you drop that limitation?

Also, these two patches(*) should get a descriptive text in the git log.
Especially the fact that it turns around the calling order is worth a
couple of words.

OK.

Thanks,
Corinna
(*) ...which should actually be only one as outlined above. The
easiest way to accomplish that is probably an interactive rebase
with squashing the second patch into the first.

As I'm not a master at git fiddling, for me the easiest way is rather
poking the files into a fresh git clone...
To be honest, I think this git format-patch is quite bothersome when
things need to be iterated...
Maybe you'd like to merge these two patches? I'll contribute an
additional description of course.

Thomas

---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Thomas Wolff

2018-03-09 07:11:56 UTC

From 15999d30c011be9041821456d23807249981dd86 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:11:44 +0100
Subject: [PATCH 3/6] remove hard-coded character data

Sorry, but no. ... don't delete and re-introduce files, just overwrite
them with the new code, for sake of a sane history.

Also, these two patches(*) should get a descriptive text in the git log.
Especially the fact that it turns around the calling order is worth a
couple of words.

OK.

Extended commit description:

The tow* functions use an included case conversion table which can be
generated from Unicode data.
The isw* functions use a character categories table (provided by
categories.c)
which can be generated from Unicode data.
Delegation between current-locale and locale-dependent functions was
reverted towards the generic locale-dependent functions; this is however
only relevant on systems with non-Unicode wide character locales,
thus not on Cygwin.

Thanks,
Corinna
(*) ...which should actually be only one as outlined above. The
easiest way to accomplish that is probably an interactive rebase
with squashing the second patch into the first.

As I'm not a master at git fiddling, for me the easiest way is rather
poking the files into a fresh git clone...
To be honest, I think this git format-patch is quite bothersome when
things need to be iterated...
Maybe you'd like to merge these two patches? I'll contribute an
additional description of course.
Thomas
---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Thomas Wolff

2018-03-09 22:54:29 UTC

Â From 15999d30c011be9041821456d23807249981dd86 Mon Sep 17 00:00:00
2001
Date: Sun, 25 Feb 2018 17:11:44 +0100
Subject: [PATCH 3/6] remove hard-coded character data

Sorry, but no.Â ... don't delete and re-introduce files, just overwrite
them with the new code, for sake of a sane history.

Also, these two patches(*) should get a descriptive text in the git log.
Especially the fact that it turns around the calling order is worth a
couple of words.

OK.

The tow* functions use an included case conversion table which can be
generated from Unicode data.
The isw* functions use a character categories table (provided by
categories.c)
which can be generated from Unicode data.
Delegation between current-locale and locale-dependent functions was
reverted towards the generic locale-dependent functions; this is however
only relevant on systems with non-Unicode wide character locales,
thus not on Cygwin.

Thanks,
Corinna
(*) ...which should actually be only one as outlined above. The
Â Â Â Â easiest way to accomplish that is probably an interactive rebase
Â Â Â Â with squashing the second patch into the first.

As I'm not a master at git fiddling, for me the easiest way is rather
poking the files into a fresh git clone...

Here is the updated common ctype patch, replacing previous patches 3/6
and 4/6.
Thomas

---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprÃŒft.
https://www.avast.com/antivirus

Thomas Preudhomme

2018-03-27 10:09:48 UTC

Hi Thomas,

This patch appears to regress 3 tests in libstdc++:

22_locale/ctype/is/wchar_t/1.cc execution test
22_locale/ctype/scan/wchar_t/1.cc execution test
28_regex/traits/wchar_t/isctype.cc execution test

I believe the issue is due to ... (scroll down)

On 09/03/18 22:54, Thomas Wolff wrote:

[SNIP]

diff --git a/newlib/libc/ctype/iswupper_l.c b/newlib/libc/ctype/iswupper_l.c
index 2555cd0..7ce8b5e
--- a/newlib/libc/ctype/iswupper_l.c
+++ b/newlib/libc/ctype/iswupper_l.c
@@ -1,10 +1,20 @@
+/* Modified (m) 2017 Thomas Wolff: revise Unicode and locale/wchar handling */
#include <_ansi.h>
+#include <ctype.h>
#include <wctype.h>
+#include "local.h"
+#include "categories.h"
int
iswupper_l (wint_t c, struct __locale_t *locale)
{
- /* We're using a locale-independent representation of upper/lower case
- based on Unicode data. Thus, the locale doesn't matter. */
- return towlower (c) != c;
+#ifdef _MB_CAPABLE
+ c = _jp2uc_l (c, locale);
+ // The wide-character class "upper" contains at least those characters wc
+ // which are equal to towupper(wc) and different from towlower(wc).
+ enum category cat = category (c);
+ return cat == CAT_Lu || (cat == CAT_LC && towupper (c) == c);
+#else
+ return c < 0x100 ? islower (c) : 0;
+#endif /* _MB_CAPABLE */
}

This change. Shouldn't is call isupper instead of islower? Or perhaps !islower?
I've tried with isupper and it makes the tests mentionned above pass.

Best regards,

Thomas

Corinna Vinschen

2018-03-27 10:37:10 UTC

Post by Thomas Preudhomme
Hi Thomas,
22_locale/ctype/is/wchar_t/1.cc execution test
22_locale/ctype/scan/wchar_t/1.cc execution test
28_regex/traits/wchar_t/isctype.cc execution test
I believe the issue is due to ... (scroll down)
[SNIP]

This change. Shouldn't is call isupper instead of islower? Or perhaps
!islower? I've tried with isupper and it makes the tests mentionned above
pass.

Yeah, calling isupper is correct, just as calling towupper in the
_MB_CAPABLE case. I pushed a patch.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-27 18:16:05 UTC

This change. Shouldn't is call isupper instead of islower? Or perhaps !islower?
I've tried with isupper and it makes the tests mentionned above pass.

Sure. I didn't mean to change the #ifdef _MB_CAPABLE #else case at all,
but due to some refactoring and reversed dependencies, it needed to be
tweaked. This looks like a copy-paste accident, which makes me really
blush. I've meanwhile compared the sources to look out for further flaws
but didn't find any. Sorry and thanks for testing.

Yeah, calling isupper is correct, just as calling towupper in the _MB_CAPABLE case. I pushed a patch.

Thanks.
Thomas

Corinna Vinschen

2018-03-09 08:48:24 UTC

From 15999d30c011be9041821456d23807249981dd86 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:11:44 +0100
Subject: [PATCH 3/6] remove hard-coded character data

Sorry, but no. ... don't delete and re-introduce files, just overwrite
them with the new code, for sake of a sane history.

The only reason for the split was that I recall a 64KB mail size
limitation on this mailing list;

There is no 64K limit. The limit is 1 Meg.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-03-09 11:06:10 UTC

From 15999d30c011be9041821456d23807249981dd86 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:11:44 +0100
Subject: [PATCH 3/6] remove hard-coded character data

Sorry, but no. ... don't delete and re-introduce files, just overwrite
them with the new code, for sake of a sane history.

Also, these two patches(*) should get a descriptive text in the git log.
Especially the fact that it turns around the calling order is worth a
couple of words.

OK.

Thanks,
Corinna
(*) ...which should actually be only one as outlined above. The
easiest way to accomplish that is probably an interactive rebase
with squashing the second patch into the first.

As I'm not a master at git fiddling, for me the easiest way is rather
poking the files into a fresh git clone...

Btw., why don't you just try my suggestion? The beauty of git is that
you can test and try in a separate branch locally without breaking
anything important in a jiffy. `git rebase --interactive is a wonderful
instrument, well worth learning it.

Here's an excellent online book for learning git:

https://git-scm.com/book/en/v2

also available in german:

https://git-scm.com/book/de/v2

Post by Thomas Wolff
To be honest, I think this git format-patch is quite bothersome when
things need to be iterated...

Not in conjunction with git send-email...

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-07 23:21:04 UTC

Corinna Vinschen

2018-03-13 21:10:05 UTC

From 58a9cfcb253165d7073a9ed25e143daa2e979c10 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:22:34 +0100
Subject: [PATCH 4/6] use generated character data
---
newlib/libc/ctype/categories.c | 39 +++
newlib/libc/ctype/categories.h | 7 +
newlib/libc/ctype/iswalnum.c | 2 +-
newlib/libc/ctype/iswalnum_l.c | 19 +-
newlib/libc/ctype/iswalpha.c | 73 ++++++
newlib/libc/ctype/iswalpha_l.c | 17 +-
newlib/libc/ctype/iswblank.c | 19 +-
newlib/libc/ctype/iswblank_l.c | 16 +-
newlib/libc/ctype/iswcntrl.c | 17 +-
newlib/libc/ctype/iswcntrl_l.c | 16 +-
newlib/libc/ctype/iswctype_l.c | 37 ++-
newlib/libc/ctype/iswdigit.c | 3 +-
newlib/libc/ctype/iswdigit_l.c | 2 +-
newlib/libc/ctype/iswgraph.c | 3 +-
newlib/libc/ctype/iswgraph_l.c | 19 +-
newlib/libc/ctype/iswlower.c | 4 +-
newlib/libc/ctype/iswlower_l.c | 16 +-
newlib/libc/ctype/iswprint.c | 72 ++++++
newlib/libc/ctype/iswprint_l.c | 17 +-
newlib/libc/ctype/iswpunct.c | 7 +-
newlib/libc/ctype/iswpunct_l.c | 22 +-
newlib/libc/ctype/iswspace.c | 20 +-
newlib/libc/ctype/iswspace_l.c | 17 +-
newlib/libc/ctype/iswupper.c | 6 +-
newlib/libc/ctype/iswupper_l.c | 16 +-
newlib/libc/ctype/iswxdigit.c | 6 +-
newlib/libc/ctype/jp2uc.c | 51 +++-
newlib/libc/ctype/local.h | 19 +-
newlib/libc/ctype/towctrans.c | 16 +-
newlib/libc/ctype/towctrans_l.c | 97 +++++++-
newlib/libc/ctype/towlower.c | 81 +++++++
newlib/libc/ctype/towlower_l.c | 7 +-
newlib/libc/ctype/towupper.c | 515 +---------------------------------------
newlib/libc/ctype/towupper_l.c | 8 +-
34 files changed, 650 insertions(+), 639 deletions(-)
create mode 100644 newlib/libc/ctype/categories.c
create mode 100644 newlib/libc/ctype/categories.h
create mode 100644 newlib/libc/ctype/iswalpha.c
create mode 100644 newlib/libc/ctype/iswprint.c
create mode 100644 newlib/libc/ctype/towlower.c

Looks like I pushed too soon. After a full rebuild Cygwin didn't work
at all anymore. After some experimenting it turned out that it depends
on the optimization settings. If I build with -O2, all is well. If I
build with just -g and no optimzation, Cygwin doesn't run anymore.

Fortunately strace is a native tool, so I could fetch an strace.

What catched my eye was that *all* paths converted to native NT
paths had a Ctrl-A in place of the drive letter 'C', like this:

\??\^A:\WINDOWS

The culprit was apparently a call to towupper() on the drive letter,
required for case sensitivity. This in turn led to the towctrans_l
function.

After some head scratching (without functioning debugger...) I realized
that there are cases which neglect to return a value due to `return c'.

Why gcc let this slip through beats me thoroughly.

I pushed a patch.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-13 21:41:14 UTC

Looks like I pushed too soon. After a full rebuild Cygwin didn't work
at all anymore. After some experimenting it turned out that it depends
on the optimization settings. If I build with -O2, all is well. If I
build with just -g and no optimzation, Cygwin doesn't run anymore.
Fortunately strace is a native tool, so I could fetch an strace.
What catched my eye was that *all* paths converted to native NT
\??\^A:\WINDOWS
The culprit was apparently a call to towupper() on the drive letter,
required for case sensitivity. This in turn led to the towctrans_l
function.
After some head scratching (without functioning debugger...) I realized
that there are cases which neglect to return a value due to `return c'.
Why gcc let this slip through beats me thoroughly.
I pushed a patch.
Corinna

Thanks a lot for hot-fixing this. I'll meditate how this could slip
through...
And I'll also check why this wasn't discovered during my extensive testing.
Thomas

---
Diese E-Mail wurde von Avast Antivirus-Software auf Viren geprüft.
https://www.avast.com/antivirus

Thomas Wolff

2018-03-23 19:28:25 UTC

From 58a9cfcb253165d7073a9ed25e143daa2e979c10 Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 17:22:34 +0100
Subject: [PATCH 4/6] use generated character data
---
Â newlib/libc/ctype/towctrans_l.c |Â 97 +++++++-
...

...
I pushed a patch.
Corinna

Thanks a lot for hot-fixing this. I'll meditate how this could slip
through...
And I'll also check why this wasn't discovered during my extensive testing.

Thanks again for helping to get this accomplished.
While meditating, I noticed that the bit packing of the case conversion
entries could use some documentation.
The attached patch adds that (and some tweaking for consistent
indentation); no code changes.
Thomas

Corinna Vinschen

2018-03-23 19:43:15 UTC

Post by Thomas Wolff
While meditating, I noticed that the bit packing of the case conversion
entries could use some documentation.
The attached patch adds that (and some tweaking for consistent indentation);
no code changes.
Thomas
From f8f4784437d319ad3ac2e3c629335fd0f50bee69 Mon Sep 17 00:00:00 2001
Date: Fri, 23 Mar 2018 20:07:22 +0100
Subject: [PATCH] comments to document struct caseconv_entry
explain design of compact (packed) struct caseconv_entry,
in case it needs to be modified for future Unicode versions;
indentation tweaks
[...]
if (cce)
switch (cce->mode)
{
- return c + cce->delta;
- return c + 1;
- switch (cce->delta)
- {
- if (!(c & 1))
- return c + 1;
- break;
- if (c & 1)
- return c + 1;
- break;
- break;
- }
+ return c + cce->delta;
+ return c + 1;
+ switch (cce->delta)
+ {
+ if (!(c & 1))
+ return c + 1;
+ break;
+ if (c & 1)
+ return c + 1;
+ break;
+ break;
+ }

Thanks but the indentation for switch statements is correct as is.
Check and compare with other GNU sources like Cygwin or GDB. The other
style is with sources taken from BSD as in vfprintf.c, but that's not
the case here.

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-23 21:02:35 UTC

Whichever the style is, the previous version was inconsistent in itself,
with "case" sometimes indented from "{" and sometimes not:
    switch (cce->mode)
      {
      case TO1:
        switch (cce->delta)
          {
            case EVENCAP:

But I don't care so much, we can reduce the patch to the documentation,
of couse.
Thomas

Thomas Wolff

2018-03-25 09:02:17 UTC

...

... we can reduce the patch to the documentation, of course.

as attached
Thomas

Corinna Vinschen

2018-03-26 08:01:53 UTC

...

... we can reduce the patch to the documentation, of course.

as attached
Thomas

Thanks, but the patch is broken. The last line in the patch is the
start of another patch hunk, which then is missing. Can you fix that,
please?

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-26 09:53:07 UTC

...

... we can reduce the patch to the documentation, of course.

as attached
Thomas

Thanks, but the patch is broken. The last line in the patch is the
start of another patch hunk, which then is missing. Can you fix that, please?

Yeah, I tried to limit git fiddling effort by manually manipulating the
patch, which failed.
(After I tried to re-sync with the current repository, it would insist
on some merging, and I do not know how to rectify that;
manual fixing of the file, git pull -f... nothing helped (error: Pulling
is not possible because you have unmerged files); I know I should
eventually consult the howto you kindly pointed me to...)
So, based on a fresh git clone, here's an updated patch, also fixing one
remaining minor layout glitch.
Thomas

Corinna Vinschen

2018-03-26 10:30:45 UTC

Post by Thomas Wolff
as attached
Thomas

Thanks, but the patch is broken. The last line in the patch is the
start of another patch hunk, which then is missing. Can you fix that, please?

Yeah, I tried to limit git fiddling effort by manually manipulating the
patch, which failed.

Never a good idea.

Post by Thomas Wolff
(After I tried to re-sync with the current repository, it would insist on
some merging, and I do not know how to rectify that;

Never do your patches on the master branch. Create a new branch from
current master and work there:

git checkout -b fix-towctrans-doc
[hack, hack, hack]
git commit
git format-patch

After the changes have been commited, just remove your hack branch, e.g.:

git checkout master
git fetch && git merge (or `git pull)
git branch -D fix-towctrans-doc

Since branches are local, they are really cheap, very different from
CVS, for instance.

Post by Thomas Wolff
manual fixing of the file, git pull -f... nothing helped (error: Pulling is
not possible because you have unmerged files);

Before you try to merge:

If you accidentally hacked on master, *now* create your hack branch

git branch fix-towctrans-doc master

then reset master to upstream master (in the state known to your local
git repo) and merge the latest from upstream master:

git reset --hard origin/master
git fetch && git merge (or `git pull')

Note that you can do the above even after you already accidentally
merged too early. Your patch will still be available on the
fix-towctrans-doc branch. Just have a look into the branch and find
your patch:

git checkout fix-towctrans-doc
git log --oneline
[prints all patches including yours with a shortened SHA-1 ID, e.g.]
[...]
123abc456def comments to document struct caseconv_entry
[...]

then cherry-pick your patch on top of master and, if required, fix
conflicts:

git co master
git cherry-pick 123abc456def

Post by Thomas Wolff
I know I should eventually
consult the howto you kindly pointed me to...)

Definitely. Because...

Post by Thomas Wolff
So, based on a fresh git clone, ...

... you're making your life much harder than necessary. git has a steep
learning curve, I bet everyone on this list will agree on that, but it's
worth the effort (and you will learn something new even after you think
you got a grip on git).

Post by Thomas Wolff
... here's an updated patch, also fixing one
remaining minor layout glitch.
Thomas

Pushed.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-03-26 10:38:21 UTC

Post by Thomas Wolff
as attached
Thomas

Thanks, but the patch is broken. The last line in the patch is the
start of another patch hunk, which then is missing. Can you fix that, please?

Yeah, I tried to limit git fiddling effort by manually manipulating the
patch, which failed.

Never a good idea.

Post by Thomas Wolff
(After I tried to re-sync with the current repository, it would insist on
some merging, and I do not know how to rectify that;

Never do your patches on the master branch. Create a new branch from
git checkout -b fix-towctrans-doc
[hack, hack, hack]
git commit
git format-patch
git checkout master
git fetch && git merge (or `git pull)
git branch -D fix-towctrans-doc
Since branches are local, they are really cheap, very different from
CVS, for instance.

Oh, and you can keep your hack branch in sync with master, like this:

git checkout master
git fetch && git merge
git co fix-towctrans-doc
git rebase master

Post by Corinna Vinschen
[...]
then cherry-pick your patch on top of master and, if required, fix
git co master

Yikes, local alias here. That should have been:

git checkout master

Post by Corinna Vinschen
git cherry-pick 123abc456def

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-26 11:31:50 UTC

Post by Thomas Wolff
as attached
Thomas

Thanks, but the patch is broken. The last line in the patch is the
start of another patch hunk, which then is missing. Can you fix that, please?

Yeah, I tried to limit git fiddling effort by manually manipulating the
patch, which failed.

Never a good idea.

Post by Thomas Wolff
(After I tried to re-sync with the current repository, it would insist on
some merging, and I do not know how to rectify that;

git checkout master
git fetch && git merge
git co fix-towctrans-doc
git rebase master

Post by Corinna Vinschen
[...]
then cherry-pick your patch on top of master and, if required, fix
git co master

git checkout master

Post by Corinna Vinschen
git cherry-pick 123abc456def

Corinna

Thanks a lot for these use-case-specific howto sniplets. That's very
useful for my local notes. I wonder how a system that makes simple use
cases need series of cryptic commands could get so popular...
Thomas

Corinna Vinschen

2018-03-26 12:45:55 UTC

Post by Thomas Wolff
as attached
Thomas

Thanks, but the patch is broken. The last line in the patch is the
start of another patch hunk, which then is missing. Can you fix that, please?

Yeah, I tried to limit git fiddling effort by manually manipulating the
patch, which failed.

Never a good idea.

Post by Thomas Wolff
(After I tried to re-sync with the current repository, it would insist on
some merging, and I do not know how to rectify that;

git checkout master
git fetch && git merge
git co fix-towctrans-doc
git rebase master

Post by Corinna Vinschen
[...]
then cherry-pick your patch on top of master and, if required, fix
git co master

git checkout master

Post by Corinna Vinschen
git cherry-pick 123abc456def

Corinna

Thanks a lot for these use-case-specific howto sniplets. That's very useful
for my local notes. I wonder how a system that makes simple use cases need
series of cryptic commands could get so popular...

Not cryptic at all as soon as you get a grip on the methodology. It's
just different from CVS and svn due to the local copies of the entire
repository. And the way how you can handle multiple remote repositories
within a single local repository. It's just immensely more powerful
than other sccs. There are a few instances where git saved my work,
where I had lost it with CVS.

Corinna,
once a fearful git critic, now a convinced believer

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-03-26 08:00:50 UTC

Whichever the style is, the previous version was inconsistent in itself,
Â Â Â switch (cce->mode)
Â Â Â Â Â {
Â Â Â Â Â Â Â switch (cce->delta)
Â Â Â Â Â Â Â Â Â {

Oh, you're right. I missed that. I fixed that under your authorship.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-07 23:21:19 UTC

Thomas Wolff

2018-03-07 23:21:32 UTC

Corinna Vinschen

2018-03-12 10:42:41 UTC

Makefile add-ons for both patch series (libc/string and libc/ctype) will be
sent separately.
From 4cd871bea1c6cf677d57587a7e844bb9cb3b19be Mon Sep 17 00:00:00 2001
Date: Sun, 25 Feb 2018 16:29:33 +0100
Subject: [PATCH 1/6] generated case conversion data, Unicode 10.0
[...]

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Can Finner

2018-03-13 10:33:10 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

Hi,
This patch breaks arm-none-eabi cross toolchain build with below error message:

/data/.../obj/gcc1/gcc/xgcc -B/data/.../obj/gcc1/gcc/
-B/data/.../obj/newlib/arm-none-eabi/thumb/newlib/ -isystem
/data/.../obj/newlib/arm-none-eabi/thumb/newlib/targ-include -isystem
/data/.../newlib-cygwin/newlib/libc/include
-B/data/.../obj/newlib/arm-none-eabi/thumb/libgloss/arm
-L/data/.../obj/newlib/arm-none-eabi/thumb/libgloss/libnosys
-L/data/.../newlib-cygwin/libgloss/arm -mthumb
-DPACKAGE_NAME=\"newlib\" -DPACKAGE_TARNAME=\"newlib\"
-DPACKAGE_VERSION=\"3.0.0\" -DPACKAGE_STRING=\"newlib\ 3.0.0\"
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -I.
-I/data/.../newlib-cygwin/newlib/libc/ctype -D__NO_SYSCALLS__
-D_COMPILING_NEWLIB -fno-builtin -g -ffunction-sections
-fdata-sections -O2 -mthumb -c -o lib_a-categories.o `test -f
'categories.c' || echo
'/data/.../newlib-cygwin/newlib/libc/ctype/'`categories.c
/data/.../newlib-cygwin/newlib/libc/ctype/categories.c:5:17: error:
width of 'cat' exceeds its type
enum category cat: 11;
^~~
make[8]: *** [lib_a-categories.o] Error 1

Newlib is configured as:

$ grep TOPLEVEL config.log
TOPLEVEL_CONFIGURE_ARGUMENTS='/data/.../newlib-cygwin/configure
--disable-newlib-supplied-syscalls --enable-newlib-io-long-long
--enable-newlib-io-c99-formats --enable-newlib-mb
--target=arm-none-eabi --prefix=/ --with-pkgversion=unknown'

Also, it might be better to send patch series in the form of:
[PATCH 0/n] Subject_0
[PATCH 1/n] Subject_1
...
[PATCH m/n] Subject_m

So each patch is described, sent and reviewed as an independent
thread. It's really hard for people not understand the code to find
which patch breaks which code, if patches are sent as reply messages
to previous one.

Thanks,
bin

Post by Corinna Vinschen
Thanks,
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

--
Regards.

Corinna Vinschen

2018-03-13 10:40:40 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

Hi,
/data/.../obj/gcc1/gcc/xgcc -B/data/.../obj/gcc1/gcc/
-B/data/.../obj/newlib/arm-none-eabi/thumb/newlib/ -isystem
/data/.../obj/newlib/arm-none-eabi/thumb/newlib/targ-include -isystem
/data/.../newlib-cygwin/newlib/libc/include
-B/data/.../obj/newlib/arm-none-eabi/thumb/libgloss/arm
-L/data/.../obj/newlib/arm-none-eabi/thumb/libgloss/libnosys
-L/data/.../newlib-cygwin/libgloss/arm -mthumb
-DPACKAGE_NAME=\"newlib\" -DPACKAGE_TARNAME=\"newlib\"
-DPACKAGE_VERSION=\"3.0.0\" -DPACKAGE_STRING=\"newlib\ 3.0.0\"
-DPACKAGE_BUGREPORT=\"\" -DPACKAGE_URL=\"\" -I.
-I/data/.../newlib-cygwin/newlib/libc/ctype -D__NO_SYSCALLS__
-D_COMPILING_NEWLIB -fno-builtin -g -ffunction-sections
-fdata-sections -O2 -mthumb -c -o lib_a-categories.o `test -f
'categories.c' || echo
'/data/.../newlib-cygwin/newlib/libc/ctype/'`categories.c
width of 'cat' exceeds its type
enum category cat: 11;
^~~

I don't understand this error. Why is an enum < 11 bits?!?

Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Corinna Vinschen

2018-03-13 10:48:55 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

I don't understand this error. Why is an enum < 11 bits?!?

As a followup, I have no idea how to fix this, building only for targets
with enums 32 bits wide. So a patch to make sure this enum is at least
16 bits on your targets as well would be welcome.

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Can Finner

2018-03-13 11:21:42 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

I don't understand this error. Why is an enum < 11 bits?!?

To be honest, I don't either. Will collect more information about this.

Thanks,
bin

Post by Corinna Vinschen
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

--
Regards.

Can Finner

2018-03-13 12:02:32 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

I don't understand this error. Why is an enum < 11 bits?!?

To be honest, I don't either. Will collect more information about this.

So it looks like arm ABI requires -fshort-enums for bare-metal
toolchain, which will pack enum type into small int-type.
In this case, the enum category as below has fewer than 256 entries in
call cases?
enum category {
#include "categories.cat"
};
If so, can we change it into below? If not, is there any macro can be
used to differentiate situations?
struct _category {
enum category cat: 8;
unsigned int first: 24;
unsigned short delta;
} __attribute__((packed));

Another choice would be adding a last entry with value larger than 255
in the enum.

Any suggestions?

Thanks,
bin

Post by Can Finner
Thanks,
bin

Post by Corinna Vinschen
Corinna
--
Corinna Vinschen
Cygwin Maintainer
Red Hat

--
Regards.

Corinna Vinschen

2018-03-13 12:49:34 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

I don't understand this error. Why is an enum < 11 bits?!?

To be honest, I don't either. Will collect more information about this.

Have a look at the file, it has barely 32 categories, so even a :5
would suffice.

Thomas, what was the idea here? 11 + 21 = 32, so was it just to
fill the struct? If so, we may want to redefine the struct, as

Post by Can Finner
If so, can we change it into below? If not, is there any macro can be
used to differentiate situations?
struct _category {
enum category cat: 8;
unsigned int first: 24;
unsigned short delta;
} __attribute__((packed));

Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer
Red Hat

Thomas Wolff

2018-03-13 13:49:23 UTC

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

I don't understand this error. Why is an enum < 11 bits?!?

To be honest, I don't either. Will collect more information about this.

Have a look at the file, it has barely 32 categories, so even a :5
would suffice.
Thomas, what was the idea here? 11 + 21 = 32, so was it just to
fill the struct?

Yes, and to keep the Unicode value right-aligned. But 8/24 will do
alike, please change it so.
Thomas

Post by Corinna Vinschen
If so, we may want to redefine the struct, as

Thanks,
Corinna

Can Finner

2018-03-13 14:05:47 UTC

On Mon, Mar 12, 2018 at 10:42 AM, Corinna Vinschen

Patchset pushed. I just squashed the makefile patch into the previous
patch to avoid a repository state not building.

I don't understand this error. Why is an enum < 11 bits?!?

To be honest, I don't either. Will collect more information about this.

Have a look at the file, it has barely 32 categories, so even a :5
would suffice.
Thomas, what was the idea here? 11 + 21 = 32, so was it just to
fill the struct?

Yes, and to keep the Unicode value right-aligned. But 8/24 will do alike,
please change it so.

Thanks for clarification, I will prepare a change.

Thanks,
bin

Thomas

Post by Corinna Vinschen
If so, we may want to redefine the struct, as

Thanks,
Corinna

--
Regards.

Corinna Vinschen

2018-03-14 09:43:37 UTC