diff mbox

ARC: gcc - Fix SIZE_TYPE to be "unsigned int" instead of long unsigned int"

Message ID 1410787585-20387-1-git-send-email-abrodkin@synopsys.com
State Superseded
Headers show

Commit Message

Alexey Brodkin Sept. 15, 2014, 1:26 p.m. UTC
This makes size_t to be "unsigned" ssize_t which makes happy compiler on data
type checks.

Fix is taken from current development branch of GCC for ARC and will be a
part of the next release of ARC tools, so at that point patch should be dropped.

https://github.com/foss-for-synopsys-dwc-arc-processors/gcc/commit/249f040299402647525c3f15b79d319fa7acddd3

Fixes http://autobuild.buildroot.net/results/405/405da9a945511329929b18740b983c51b8dcc43e

Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>

Cc: Anton Kolesov <akolesov@synopsys.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Peter Korsgaard <peter@korsgaard.com>
---
 package/gcc/arc-2014.08/200-size_type_unsigned_int.patch | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 package/gcc/arc-2014.08/200-size_type_unsigned_int.patch

Comments

Thomas Petazzoni Sept. 15, 2014, 1:50 p.m. UTC | #1
Dear Alexey Brodkin,

On Mon, 15 Sep 2014 17:26:25 +0400, Alexey Brodkin wrote:
> This makes size_t to be "unsigned" ssize_t which makes happy compiler on data
> type checks.
> 
> Fix is taken from current development branch of GCC for ARC and will be a
> part of the next release of ARC tools, so at that point patch should be dropped.
> 
> https://github.com/foss-for-synopsys-dwc-arc-processors/gcc/commit/249f040299402647525c3f15b79d319fa7acddd3
> 
> Fixes http://autobuild.buildroot.net/results/405/405da9a945511329929b18740b983c51b8dcc43e
> 
> Signed-off-by: Alexey Brodkin <abrodkin@synopsys.com>
> 
> Cc: Anton Kolesov <akolesov@synopsys.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Peter Korsgaard <peter@korsgaard.com>
> ---
>  package/gcc/arc-2014.08/200-size_type_unsigned_int.patch | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>  create mode 100644 package/gcc/arc-2014.08/200-size_type_unsigned_int.patch

Thanks, looks good, but...

> 
> diff --git a/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> new file mode 100644
> index 0000000..78d4b10
> --- /dev/null
> +++ b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch

Patch lacks a description + SoB.

Thanks!

Thomas
Alexey Brodkin Sept. 15, 2014, 2:06 p.m. UTC | #2
Hi Thomas,

On Mon, 2014-09-15 at 15:50 +0200, Thomas Petazzoni wrote:
> Dear Alexey Brodkin,
> > diff --git a/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> > new file mode 100644
> > index 0000000..78d4b10
> > --- /dev/null
> > +++ b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> 
> Patch lacks a description + SoB.

Well first of all IMHO patch name is self-explanatory.
Than I didn't want to duplicate the same text in commit message and in
patch itself.

If you think this is helpful I will happily add the same text and URL in
the patch.

Let me know what do you think about it.

-Alexey
Thomas Petazzoni Sept. 15, 2014, 2:29 p.m. UTC | #3
Dear Alexey Brodkin,

On Mon, 15 Sep 2014 14:06:43 +0000, Alexey Brodkin wrote:

> > > diff --git a/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> > > new file mode 100644
> > > index 0000000..78d4b10
> > > --- /dev/null
> > > +++ b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> > 
> > Patch lacks a description + SoB.
> 
> Well first of all IMHO patch name is self-explanatory.
> Than I didn't want to duplicate the same text in commit message and in
> patch itself.
> 
> If you think this is helpful I will happily add the same text and URL in
> the patch.
> 
> Let me know what do you think about it.

We have a stupid rule, and stupid rules are meant to be complied
with :-)

Joke aside, I think what you mentioned in the commit log is useful:
what the patch does, and the fact that it's already "upstream" and
therefore should be dropped at the next version bump of the ARC gcc
compiler.

Thanks!

Thomas
Alexey Brodkin Sept. 15, 2014, 2:41 p.m. UTC | #4
Hi Thomas,

On Mon, 2014-09-15 at 16:29 +0200, Thomas Petazzoni wrote:
> Dear Alexey Brodkin,
> 
> On Mon, 15 Sep 2014 14:06:43 +0000, Alexey Brodkin wrote:
> 
> > > > diff --git a/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> > > > new file mode 100644
> > > > index 0000000..78d4b10
> > > > --- /dev/null
> > > > +++ b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
> > > 
> > > Patch lacks a description + SoB.
> > 
> > Well first of all IMHO patch name is self-explanatory.
> > Than I didn't want to duplicate the same text in commit message and in
> > patch itself.
> > 
> > If you think this is helpful I will happily add the same text and URL in
> > the patch.
> > 
> > Let me know what do you think about it.
> 
> We have a stupid rule, and stupid rules are meant to be complied
> with :-)

Understood - uniformity is good even if the rule itself is not that
good :)

> Joke aside, I think what you mentioned in the commit log is useful:
> what the patch does, and the fact that it's already "upstream" and
> therefore should be dropped at the next version bump of the ARC gcc
> compiler.

Funny enough that initially I had the same comment in the patch itself,
but while looking at output of "git format-patch" I found myself a bit
frustrated - if I were a reviewer I would say that submitted "full"
patch looks ridiculous.

Then I removed comment from commit message. That made me think that you
guys won't apply a patch that won't show any useful info in git log. 

So I decided to move a message to the Buildroot commit. This made sense
for me because at least when I bump version of a package I look at
history of this package and this way try to figure out what's still
required and what's not.

Moreover looking at the next folder "package/gcc/4.9.1" I found that
half of patches don't have any comment inside, some have URLs to
bugs/commits in GCC, some are full-scale patches with verbose
description.

Now you may understand my frustration.

Anyways, I'm about to send v2 with comments in both commit message and
patch itself.

Thanks for your time.

-Alexey
Thomas Petazzoni Sept. 15, 2014, 2:55 p.m. UTC | #5
Dear Alexey Brodkin,

On Mon, 15 Sep 2014 14:41:57 +0000, Alexey Brodkin wrote:

> > We have a stupid rule, and stupid rules are meant to be complied
> > with :-)
> 
> Understood - uniformity is good even if the rule itself is not that
> good :)

Absolutely :)

> Funny enough that initially I had the same comment in the patch itself,
> but while looking at output of "git format-patch" I found myself a bit
> frustrated - if I were a reviewer I would say that submitted "full"
> patch looks ridiculous.
> 
> Then I removed comment from commit message. That made me think that you
> guys won't apply a patch that won't show any useful info in git log. 
> 
> So I decided to move a message to the Buildroot commit. This made sense
> for me because at least when I bump version of a package I look at
> history of this package and this way try to figure out what's still
> required and what's not.

Hehe, funny. No problem if there is actually some duplication between
the git commit log, and the patch description itself.

> Moreover looking at the next folder "package/gcc/4.9.1" I found that
> half of patches don't have any comment inside, some have URLs to
> bugs/commits in GCC, some are full-scale patches with verbose
> description.

Half of the patches are carried over since gcc 4.2 or so, in a pre-2008
era, at a point where the state of Buildroot was clearly not as good as
it is today. I think you wouldn't want to look at a pre-2008 Buildroot
tree :-)

That being said, is your comment an indication that you volunteer to do
some research about these patches, write proper descriptions, or even
better help getting them merged upstream? :-)

Joke aside again, it's by having less and less patches with no
description that at some point all patches in Buildroot will have a
description. This way nobody will feel your frustration because it will
just appear to be the "normal thing".

> Now you may understand my frustration.

Yes, of course, I do understand.

Thomas
diff mbox

Patch

diff --git a/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
new file mode 100644
index 0000000..78d4b10
--- /dev/null
+++ b/package/gcc/arc-2014.08/200-size_type_unsigned_int.patch
@@ -0,0 +1,11 @@ 
+--- a/gcc/config/arc/arc.h
++++ b/gcc/config/arc/arc.h
+@@ -487,7 +487,7 @@ if (GET_MODE_CLASS (MODE) == MODE_INT		\
+ /* Define this as 1 if `char' should by default be signed; else as 0.  */
+ #define DEFAULT_SIGNED_CHAR 0
+ 
+-#define SIZE_TYPE "long unsigned int"
++#define SIZE_TYPE "unsigned int"
+ #define PTRDIFF_TYPE "long int"
+ #define WCHAR_TYPE "int"
+ #define WCHAR_TYPE_SIZE 32