quiet gcc warnings

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

quiet gcc warnings

Peter Karman
Not sure that this merits a JIRA ticket, but this little patch quiets a gcc warning:

Index: core/Lucy/Object/Obj.c
===================================================================
--- core/Lucy/Object/Obj.c (revision 884883)
+++ core/Lucy/Object/Obj.c (working copy)
@@ -60,7 +60,7 @@
 #if (SIZEOF_PTR == 4)
     return CB_newf("%o@0x%x32", Obj_Get_Class_Name(self), self);
 #elif (SIZEOF_PTR == 8)
-    size_t address = self;
+    size_t address = (size_t)self;
     u32_t  address_hi = address >> 32;
     u32_t  address_lo = address & 0xFFFFFFFF;
     return CB_newf("%o@0x%x32%x32", Obj_Get_Class_Name(self), address_hi,

--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: quiet gcc warnings

Marvin Humphrey
On Fri, Nov 27, 2009 at 09:04:27AM -0600, Peter Karman wrote:
> Not sure that this merits a JIRA ticket, but this little patch quiets a gcc warning:

>  #elif (SIZEOF_PTR == 8)
> -    size_t address = self;
> +    size_t address = (size_t)self;

Hmm, is that 100% right?  The existing code is definitely wrong and needs
fixing, but I can never remember all the ways that pointer-to-integer and
integer-to-pointer conversion can go screwy.  There are some really weird
truncation effects on some systems.

Probably what we ought to do is rely on macros *every* time we need to convert
between integers and pointers.  The conversion macros would be implemented in
Charmonizer and would get thorough testing.

There's already a PTR2I64 macro in Charmonizer/Probe/Integers, but we probably
need a larger menu:

   PTR2I64  
   PTR2U64  
   PTR2I32
   PTR2U32
   PTR2SIZET

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
Marvin Humphrey wrote on 11/27/09 12:10 PM:

> On Fri, Nov 27, 2009 at 09:04:27AM -0600, Peter Karman wrote:
>> Not sure that this merits a JIRA ticket, but this little patch quiets a gcc warning:
>
>>  #elif (SIZEOF_PTR == 8)
>> -    size_t address = self;
>> +    size_t address = (size_t)self;
>
> Hmm, is that 100% right?  The existing code is definitely wrong and needs
> fixing, but I can never remember all the ways that pointer-to-integer and
> integer-to-pointer conversion can go screwy.  There are some really weird
> truncation effects on some systems.
>
> Probably what we ought to do is rely on macros *every* time we need to convert
> between integers and pointers.  The conversion macros would be implemented in
> Charmonizer and would get thorough testing.
>
> There's already a PTR2I64 macro in Charmonizer/Probe/Integers, but we probably
> need a larger menu:
>
>    PTR2I64  
>    PTR2U64  
>    PTR2I32
>    PTR2U32
>    PTR2SIZET
>

sounds reasonable.

Should pointers always be converted to unsigned types? Seems like it.

So in the example above, with the macro it becomes:

 chy_u64_t address = PTR2U64(self);

?

--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
On Sat, Nov 28, 2009 at 08:58:04PM -0600, Peter Karman wrote:

> > There's already a PTR2I64 macro in Charmonizer/Probe/Integers, but we probably
> > need a larger menu:
> >
> >    PTR2I64  
> >    PTR2U64  
> >    PTR2I32
> >    PTR2U32
> >    PTR2SIZET
>
> sounds reasonable.
>
> Should pointers always be converted to unsigned types?

Sometimes if you're subtracting two pointers from each other, you need the
difference to be a signed result.  The type "ptrdiff_t" exists for this
reason, but it's tricky to use correctly because you have to think about how
it gets upconverted on different systems.

http://blogs.msdn.com/david_leblanc/archive/2008/09/02/ptrdiff-t-is-evil.aspx

My approach is to avoid ptrdiff_t altogether and always upconvert all operands
to i64_t before performing the arithmetic.  For example[1]:

    static INLINE i64_t
    SI_tell(InStream *self)
    {
        FileWindow *const window = self->window;
        i64_t pos_in_buf = PTR2I64(self->buf) - PTR2I64(window->buf);
        return pos_in_buf + window->offset - self->offset;
    }

A thought: should we only define PTR2I32 and PTR2U32 on 32-bit systems?
They'd silently discard data on 64-bit systems.  That would be bad.  

We should force the programmer to either wrap the PTR2x32 conversion in an
#ifdef, or go through PTR2x64 first and then truncate with an explicit cast
which verifies their intent.

    /* Assume that "start" and "end" cannot be more than I32_MAX apart. */
    #if (SIZEOF_PTR == 4)
       i32_t length = PTR2I32(end) - PTR2I32(start);
    #else
       i64_t a = PTR2I64(start);
       i64_t b = PTR2I64(end);
       i32_t length = (i32_t)(b - a);
    #endif

[1] The instream->window->buf pointer is used to keep track of where the
    mmap'd region starts, and instream->buf serves as the cursor.  To tell
    where we are in the file, we need to do some pointer math, then add the
    result into an i64_t file position calculation.  We don't have to worry
    about integer promotion in the last line, because all three operands have
    the type i64_t.

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
Marvin Humphrey wrote on 11/29/09 6:36 PM:

> On Sat, Nov 28, 2009 at 08:58:04PM -0600, Peter Karman wrote:
>
>>> There's already a PTR2I64 macro in Charmonizer/Probe/Integers, but we probably
>>> need a larger menu:
>>>
>>>    PTR2I64  
>>>    PTR2U64  
>>>    PTR2I32
>>>    PTR2U32
>>>    PTR2SIZET
>> sounds reasonable.
>>
>> Should pointers always be converted to unsigned types?
>
> Sometimes if you're subtracting two pointers from each other, you need the
> difference to be a signed result.  The type "ptrdiff_t" exists for this
> reason, but it's tricky to use correctly because you have to think about how
> it gets upconverted on different systems.
>
> http://blogs.msdn.com/david_leblanc/archive/2008/09/02/ptrdiff-t-is-evil.aspx
>

that's a good example.


> My approach is to avoid ptrdiff_t altogether and always upconvert all operands
> to i64_t before performing the arithmetic.  For example[1]:
>
>     static INLINE i64_t
>     SI_tell(InStream *self)
>     {
>         FileWindow *const window = self->window;
>         i64_t pos_in_buf = PTR2I64(self->buf) - PTR2I64(window->buf);
>         return pos_in_buf + window->offset - self->offset;
>     }
>

yes, makes sense.

> A thought: should we only define PTR2I32 and PTR2U32 on 32-bit systems?
> They'd silently discard data on 64-bit systems.  That would be bad.  
>
> We should force the programmer to either wrap the PTR2x32 conversion in an
> #ifdef, or go through PTR2x64 first and then truncate with an explicit cast
> which verifies their intent.
>
>     /* Assume that "start" and "end" cannot be more than I32_MAX apart. */
>     #if (SIZEOF_PTR == 4)
>        i32_t length = PTR2I32(end) - PTR2I32(start);
>     #else
>        i64_t a = PTR2I64(start);
>        i64_t b = PTR2I64(end);
>        i32_t length = (i32_t)(b - a);
>     #endif
>

yes, that seems sane.

Shall I have a go at this for my next task, or do you have something else in mind?




--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
On Tue, Dec 08, 2009 at 08:33:45PM -0600, Peter Karman wrote:

> >My approach is to avoid ptrdiff_t altogether and always upconvert all
> >operands
> >to i64_t before performing the arithmetic.  For example[1]:
> >
> >    static INLINE i64_t
> >    SI_tell(InStream *self)
> >    {
> >        FileWindow *const window = self->window;
> >        i64_t pos_in_buf = PTR2I64(self->buf) - PTR2I64(window->buf);
> >        return pos_in_buf + window->offset - self->offset;
> >    }
> >
>
> yes, makes sense.

I should mention for the record that technically it's a violation of the C
standard to perform arbitrary pointer math.  This is officially undefined:

  char *foo = (char*)malloc(2);
  foo = foo--;

The only officially valid addresses with regards to that pointer are:

  foo
  foo + 1
  foo + 2

See:

  http://stackoverflow.com/questions/784764/what-c-compilers-have-pointer-subtraction-underflows

However, on common modern systems, you get away with it... and in fact
InStream's design depends on that construct working.

> >A thought: should we only define PTR2I32 and PTR2U32 on 32-bit systems?
> >They'd silently discard data on 64-bit systems.  That would be bad.  
> >
> >We should force the programmer to either wrap the PTR2x32 conversion in an
> >#ifdef, or go through PTR2x64 first and then truncate with an explicit cast
> >which verifies their intent.
> >
> >    /* Assume that "start" and "end" cannot be more than I32_MAX apart. */
> >    #if (SIZEOF_PTR == 4)
> >       i32_t length = PTR2I32(end) - PTR2I32(start);
> >    #else
> >       i64_t a = PTR2I64(start);
> >       i64_t b = PTR2I64(end);
> >       i32_t length = (i32_t)(b - a);
> >    #endif
> >
>
> yes, that seems sane.
>
> Shall I have a go at this for my next task, or do you have something else
> in mind?

That would be a good one.

Anything that moves us towards the goal of an easy to understand and use
Charmonizer code base would be handy.  

The next step is probably for me to redraft the top level docs -- Charmonizer
has the functionality it needs, but the organization can be improved.  Then if
I can persuade you and Nate to critique them, we'll refactor accordingly.

Since Charmonizer is an internal API, it doesn't have to be perfect, but it's
a dialect of C that Lucy contributors will have to familiarize themselves with
and the less effort that takes the better.

Hmmm....

Along those lines, it would be nice if we could phase out all the
Charmonizer-specific integer types and replace them with types from the C99
header stdint.h.

http://www.opengroup.org/onlinepubs/000095399/basedefs/stdint.h.html

So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.

We could actually generate a complete and usable stdint.h on systems where it
isn't available using no more compilation tests than we already have going in
Charmonizer/Probe/Integers.c.  But as a quick and dirty fix, we can either
pound-include stdint.h in charmony.h if it's there, or change the naming of
our typedefs if it's not.

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
On Tue, Dec 08, 2009 at 09:24:44PM -0800, Marvin Humphrey wrote:

> So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
>
> We could actually generate a complete and usable stdint.h on systems where it
> isn't available using no more compilation tests than we already have going in
> Charmonizer/Probe/Integers.c.  But as a quick and dirty fix, we can either
> pound-include stdint.h in charmony.h if it's there, or change the naming of
> our typedefs if it's not.

PS...

If you take this on, just add rather than replace the typedefs and call it a
day.  If we try to remove the old defs right now, we'll get massive failure
throughout the rest of the system, and it's not worth trying to fix that all
at once.

After the stdint.h typedefs are in Charmonizer, we can expand Clownfish to
accept the new type.  Then we replace all instances in core and the Perl
bindings.  Lastly, once the substitution has completed, we can safely remove
support for the chy int types from Clownfish and Charmonizer.

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
Marvin Humphrey wrote on 12/9/09 6:15 PM:

> On Tue, Dec 08, 2009 at 09:24:44PM -0800, Marvin Humphrey wrote:
>
>> So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
>>
>> We could actually generate a complete and usable stdint.h on systems where it
>> isn't available using no more compilation tests than we already have going in
>> Charmonizer/Probe/Integers.c.  But as a quick and dirty fix, we can either
>> pound-include stdint.h in charmony.h if it's there, or change the naming of
>> our typedefs if it's not.
>
> PS...
>
> If you take this on, just add rather than replace the typedefs and call it a
> day.  If we try to remove the old defs right now, we'll get massive failure
> throughout the rest of the system, and it's not worth trying to fix that all
> at once.
>
> After the stdint.h typedefs are in Charmonizer, we can expand Clownfish to
> accept the new type.  Then we replace all instances in core and the Perl
> bindings.  Lastly, once the substitution has completed, we can safely remove
> support for the chy int types from Clownfish and Charmonizer.
>

got it. I'll take this on, including opening the JIRA ticket.


--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
On Thu, Dec 10, 2009 at 09:04:35AM -0600, Peter Karman wrote:

> got it. I'll take this on, including opening the JIRA ticket.

Great, I'll go work on the following couple of things today:

  * Finish phase 1 of the switchover from Boilerplater to Clownfish, so that
    we can eliminate trunk/boilerplater and use only trunk/clownfish.
  * Mod Clownfish to accept the new integer types.

Once the two of us finish our respective tasks, it will be OK to start using
the new integer types in the Lucy core.

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
On Thu, Dec 10, 2009 at 09:22:00AM -0800, Marvin Humphrey wrote:

> Great, I'll go work on the following couple of things today:
>
>   * Finish phase 1 of the switchover from Boilerplater to Clownfish, so that
>     we can eliminate trunk/boilerplater and use only trunk/clownfish.
>   * Mod Clownfish to accept the new integer types.

Done.

I didn't include everything from stdint.h (e.g. no int_fast32_t, no
int_least32_t, no intmax_t), because it's not clear that Clownfish ought to
support all that.  Just the following for now:

  int8_t
  int16_t
  int32_t
  int64_t
  uint8_t
  uint16_t
  uint32_t
  uint64_t

Those will allow us to eliminate all the chy int types except for bool_t,
which we'll replace with "bool".  For "bool", we could use a three-part probe:

    If the C99 header stdbool.h is available, we pound-include it in
    charmony.h.

    Else if the macro "__cplusplus" is defined, then we just use C++'s
    built-in bool type.

    Else we typedef "bool" to int and conditionally define "true" and "false",
    just like we're doing now for chy_bool_t.

I'm not sure about that third branch, though, because it could potentially
conflict with somebody else typedef-ing bool to "char".  "chy_bool_t" was
safe to muck with, but "bool" is not.

So, probably the best move is to just omit the third branch, making either
stdbool.h or C++ a prerequisite for Lucy.

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
In reply to this post by Peter Karman
Peter Karman wrote on 12/10/09 9:04 AM:

> Marvin Humphrey wrote on 12/9/09 6:15 PM:
>> On Tue, Dec 08, 2009 at 09:24:44PM -0800, Marvin Humphrey wrote:
>>
>>> So, instead of "u32_t" or "chy_u32_t", we'd use "uint32_t" everywhere.
>>>
>>> We could actually generate a complete and usable stdint.h on systems
>>> where it
>>> isn't available using no more compilation tests than we already have
>>> going in
>>> Charmonizer/Probe/Integers.c.  But as a quick and dirty fix, we can
>>> either
>>> pound-include stdint.h in charmony.h if it's there, or change the
>>> naming of
>>> our typedefs if it's not.
>>
>> PS...
>>
>> If you take this on, just add rather than replace the typedefs and
>> call it a
>> day.  If we try to remove the old defs right now, we'll get massive
>> failure
>> throughout the rest of the system, and it's not worth trying to fix
>> that all
>> at once.
>>
>> After the stdint.h typedefs are in Charmonizer, we can expand
>> Clownfish to
>> accept the new type.  Then we replace all instances in core and the Perl
>> bindings.  Lastly, once the substitution has completed, we can safely
>> remove
>> support for the chy int types from Clownfish and Charmonizer.
>>
>
> got it. I'll take this on, including opening the JIRA ticket.
>
>

well, I've opened the ticket and started poking around but I am not sure where
to include stdint.h and/or test for its existence.

Is Charmonizer/Probe/Integers.c where I want to make this addition of include
stdint.h?

It *seems* like I would add something in Integers_run() in Integers.c. Is that
right?


--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
On Fri, Dec 11, 2009 at 08:59:32PM -0600, Peter Karman wrote:
> well, I've opened the ticket and started poking around but I am not sure
> where to include stdint.h and/or test for its existence.
>
> Is Charmonizer/Probe/Integers.c where I want to make this addition of
> include stdint.h?

Yes.  Use the HeadCheck_check_header() function to determine whether stdint.h
is available.

If it's there use ModHand_append_conf() to add the pound-include.

If it's not there, use ModHand_append_conf() to add the typedefs.

Marvin Humphrey
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Marvin Humphrey
> Yes.  Use the HeadCheck_check_header() function to determine whether stdint.h
> is available.
>
> If it's there use ModHand_append_conf() to add the pound-include.
>
> If it's not there, use ModHand_append_conf() to add the typedefs.

PS: Oh, and if you can muster the energy, please keep a brainlog as you try to
figure this out.  It would be a valuable refactoring aid.

Marvin Humphrey

Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
Marvin Humphrey wrote on 12/11/09 9:19 PM:

> PS: Oh, and if you can muster the energy, please keep a brainlog as you try to
> figure this out.  It would be a valuable refactoring aid.

* I want to basically add this logic to the generated charmony.h file:

#ifdef _HAVE_STDINT_
#include <stdint.h>
#else
typedef .. // add our basic int types here based on Probe/Integer.c
#endif

where do I make this change in Charmonizer so that the code is generated correctly?

* I have 2 file names to go on from the email thread (upon which I must rely
because I can't remember anything anymore. early onset senility. Now, what was I
doing? Oh yes.) Charmonizer/Probe/Integer.c and charmony.h

  % find . -name 'charmony.h'

one hit, in perl/charmony.h -- but that file clearly says it is generated by
Charmonizer, so Charmonizer/Probe/Integer.c is my likely bet.

* open Integer.c and Integer.h to poke around. My first instinct is to add the
#include <stdint.h> at the top to go with the other standard includes there, but
since part of the task assumes that file is not always available, I'll need to
avoid my first instinct.

* ah, here is some text I recognize from charmony.h in Integers_run(). This must
be the generator. Write to lucy list to confirm before I dive in.

[… time passes …]

Marvin replies with perfectly clear advice on the functions to use. Exactly what
I need. Also, started this brainlog.

* Knowing which functions to use makes this very easy. The hardest part becomes
where in Integers_run() is most appropriate place to put the new code. I settle
on a spot right below the comment about writing affirmations and typedefs, with
the assumption that Some Day in the Future, the stdint defs will replace all the
chy_* stuff.

* cd perl && make clean && make test

* /me drinks the Charmonizer kool-aid. I get it now. Very nice. It's sort of
like the next evolution of autoconf, with the added love of letting you generate
C via Clownfish.






--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
In reply to this post by Marvin Humphrey
Marvin Humphrey wrote on 12/11/09 9:16 PM:

> On Fri, Dec 11, 2009 at 08:59:32PM -0600, Peter Karman wrote:
>> well, I've opened the ticket and started poking around but I am not sure
>> where to include stdint.h and/or test for its existence.
>>
>> Is Charmonizer/Probe/Integers.c where I want to make this addition of
>> include stdint.h?
>
> Yes.  Use the HeadCheck_check_header() function to determine whether stdint.h
> is available.
>
> If it's there use ModHand_append_conf() to add the pound-include.
>
> If it's not there, use ModHand_append_conf() to add the typedefs.
>

perfect. patch uploaded to jira.


--
Peter Karman  .  http://peknet.com/  .  [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [Lucy] Re: quiet gcc warnings

Peter Karman
In reply to this post by Marvin Humphrey
Marvin Humphrey wrote on 12/10/09 7:30 PM:

> Those will allow us to eliminate all the chy int types except for bool_t,
> which we'll replace with "bool".  For "bool", we could use a three-part probe:
>
>     If the C99 header stdbool.h is available, we pound-include it in
>     charmony.h.
>
>     Else if the macro "__cplusplus" is defined, then we just use C++'s
>     built-in bool type.
>
>     Else we typedef "bool" to int and conditionally define "true" and "false",
>     just like we're doing now for chy_bool_t.
>
> I'm not sure about that third branch, though, because it could potentially
> conflict with somebody else typedef-ing bool to "char".  "chy_bool_t" was
> safe to muck with, but "bool" is not.
>
> So, probably the best move is to just omit the third branch, making either
> stdbool.h or C++ a prerequisite for Lucy.
>

I will tackle this in a separate patch. I agree about avoiding the 3rd branch.
In libswish3 I define a 'boolean' (spelled out long to avoid conflict) as a
char. I expect others out there have done similar.

--
Peter Karman  .  http://peknet.com/  .  [hidden email]