librelist archives

« back to archive

winx64 compilation warnings and more...

winx64 compilation warnings and more...

From:
Sniffles
Date:
2012-01-30 @ 07:57
Hi there people,

 

I've been looking at the libgit2 library over the last couple of days.

 

A bit of background from where I'm coming from so you don't think that I'm
banging on libgit2 (which I think is a long overdue answer to a fundamental
architecture problem with git)

 

1.       I'm a windows user/programmer - this doesn't mean I want to start a
windows/unix flavor war.

 

2.       IMHO git for windows is a disgrace - why? Trying to call msysgit a
windows native port is a flat out lie. sure it compiled on a windows machine
but guess what it still requires a unix/linux emulation layer to sit on top
of. the same goes for tortoisegit. what a joke. Forget about trying to set
git up as a service on a windows machine

 

3.       It is my opinion that the unix git tools are simply a proof of
concept (that works very well in a unix world but has a less than admirable
windows ability), that is waiting to be properly cemented in a truly
portable library. and things like command line tools and gui tools are
simply thin shells leveraging the central library/core functions.

 

I'm excited that libgit2 will actually be able to bring git to the windows
masses as a proper and mature source control system and represent the next
evolution of git beyond unix/linux shell commands.

 

On with the main thrust of this email.

 

Firstly:

 

I note that one of the open issues is the number of warnings when compiling
under vs2010-winx64 to do with "conversion from 'ssize_t' to 'int', possible
loss of data" and various other warnings of this nature. which I've
verified.

 

Now I note by looking at the source code and api that you are in fact
targeting perhaps incorrectly.

 

Eg:

 

int git_buf_grow(git_buf *buf, size_t target_size)

 

is one of the functions i've pulled at random from the buffer.c file.

 

Note that the specific mention of 'int' as a return type automatically
assumes 32-bit architecture.

This is for all api functions and all internal functions from a quick peruse
of the code. and published api.

 

I also note that internal local variables inside functions are also doing
the same thing and assuming 32-bit.

 

1.       Wouldn't it be more prudent to actually use something like 'size_t'
which automatically sizes to the target architecture? This for all return
types and 'int' style parameters??? Only actually use 'int' where it has be
to 32-bit eg say in a 'struct' like a packet that gets passed across a
network where it's definitely got to be 32-bit. according to the protocol in
use???

This would require all the api functions to be changed to: size_t
git_function_to_do_something(git_type *parameter,....)

 

I only say this as otherwise it is kind of pointless to be compiling for
64-bit architecture if you are just going to be using 32-bit internally
anyway. You might as well only provide 32-bit targets and have the operating
system worry about trying to run 32-bit on a 64-bit architecture. (which in
the windows world is no biggie)

 

Secondly:

 

I also note that a lot of the malloc/calloc etc. calls are not being type
cast to the respective variables type. 

 

      new_ptr = git__realloc(new_ptr, new_size);

 

Where new_ptr is a char*

 

It should read:

 

                new_ptr = (char*)git__realloc(new_ptr, new_size)

 

If not, you get stupid intellisense errors. (might be a flaw of vs2010.) in
the ide

 

Either that or a specific compiler warning has been turned off somewhere so
the warnings don't get spit out. during compilation (this is lazy.)

 

Thirdly:

 

Packaging - Why isn't this done yet??? Is there some master plan which you
guys are working to??? Milestones planned??? Etc. Can't seem to see anything
online regarding this???

 

Are you planning on writing/providing the equivalent thin shells (as
mentioned above) to provide a complete git package/experience??? Or is this
to be a separate thing??? I note that people are working on language
bindings which is cool but what about being able to use the library from the
command line etc. just like the normal unix/linux git tool(s).

 

Fourthly:

 

What is the time frame for network capability and being able to hook this
library up as a daemon and use as a windows service??? (god help me trying
to do it the unix/linux way with ssh and all the other crap.) svn simply
kills git in this regard.

 

Do I want to host propriety source code on github??? No I don't.

Do I want to run my own git server which the developers can access? Why yes
I do. But guess what - you have to be blessed by the gods to be able to do
it on a windows machine. let me stick with svn because it is so easy to set
up..

 

I apologise in advance for the long winded email..

 

Thanks, David.

Re: [libgit2] winx64 compilation warnings and more...

From:
Russell Belfer
Date:
2012-01-30 @ 18:32
On Sun, Jan 29, 2012 at 11:57 PM, Sniffles <sniffles@xnet.co.nz> wrote:

> I’ve been looking at the libgit2 library over the last couple of days…
>

Hi Sniffles, It's great to have more eyes on the library, particularly
Windows developers. Thanks.

I note that one of the open issues is the number of warnings when compiling
> under vs2010-winx64 to do with “conversion fro m ‘ssize_t’ to ‘int’,
> possible loss of data” and various other warnings of this nature… which
> I’ve verified…
>

At this point, I believe that this issue has been examined carefully and
all of the remaining warnings are thought to be harmless. The issue is
still open because we would really like it to be cleared up, but it is not
at the top of anyone's priority list right now.


> ****
>
> int git_buf_grow(git_buf *buf, size_t target_size)**
>
> **
>
> ** **
>
> is one of the functions i’ve pulled at random from the buffer.c file…****
>
> ** **
>
> Note that the specific ment ion of ‘int’ as a return type automatically
> assumes 32-bit architecture…****
>
> This is for all api functions and all internal functions from a quick
> peruse of the code… and published api…****
>
> ** **
>
> I also note that internal local variables inside functions are also doing
> the same thing and assuming 32-bit…
>

I think there may be some confusion here. Currently the library uses ints
for returning error codes. In this case, int *is* an appropriate choice
because it allows the compiler to choose an efficient data size for
returning small but signed numeric values on the platform. We have
discussed improvements to the error handling strategy in libgit2 and would
be open to suggestions, but returning an int is fairly simply and common
strategy that seems to be working fine for the various language binding
implementors who rely on libgit2.

I have just re-reviewed the library and pretty much only see us using ints
as error return codes, as internal storage for boolean values or small
known enumeration values, and as loop iterators. There are a couple of
exceptions that we can probably examine, but we are generally extremely
careful about this. Please take a little time to examine the code more
carefully.


> I also note that a lot of the malloc/calloc etc… calls are not being type
> cast to the respective variables type…
>
> **
>
> ** **
>
>       new_ptr = git__realloc(new_ptr, new_size);****
>
> ** **
>
> Where new_ptr is a char*****
>
> ** **
>
> It should read:****
>
> ** **
>
>                 new_ptr = (char*)git__realloc(new_ptr, new_size)****
>
> ** **
>
> If not, you get stupid intellisense errors… (might be a flaw of vs2010…)
> in the ide****
>
> ** **
>
> Either that or a specific compiler warning has been turned off somewhere
> so the warnings don’t get spit out… during compilation (this is lazy…)
>

As others have commented, this type of cast is not required by a standards
compliant C compiler, however, I am open to adding casts like this if it
make it easier for Windows developers to adopt libgit2. Please send a pull
request!

Also, for reference, we compile with an fairly high level of warnings
turned on, much higher than many other projects I have been involved in in
my career, so please don't call us lazy. You should see what most other
libraries do when compiled with our warning flags.

Packaging – Why isn’t this done yet??? Is there some master plan which you
> guys are working to??? Milestones planne d??? Etc… Can’t seem to see
> anything online regarding this???****
>
> ** **
>
> Are you planning on writing/providing the equivalent thin shells (as
> mentioned above) to provide a complete git package/experience??? Or is this
> to be a separate thing??? I note that people are working on language
> bindings which is cool but what about being able to use the library from
> the command line etc… just like the normal unix/linux git tool(s)…
>

I think this question is really about the priorities of the libgit2 project
and we should be clear about what those are, perhaps clearer than we are
given your questions. Embed-ability and language bindings are actually our
top priority with libgit2 since those are the areas where core git is most
problematic. For the majority of the world, command line git support is
sufficiently functional that there is no pressing need for us to address
that. Of course, if that is *your* itch, I would love to see a
libgit2-based client - if for no other reason than it would really help
identify our bugs.


> ****
>
> Do I want to run my own git server which the developers can access? Why
> yes I do… But guess what – you have to be blessed by the gods to be able to
> do it on a windows machine… let me stick with svn because it is so easy to
> set up….
>

Each person has to make their own choices about where they want to spend
their time and energy. For you, sticking with svn may very well be the
right way to go for now. We will keep working away on libgit2 and hopefully
in time you'll find it will meet your needs. Svn has about a decade of
extra development time over us, so give us a few more months to catch up.

-- Russell

Re: [libgit2] winx64 compilation warnings and more...

From:
Carlos Martín Nieto
Date:
2012-01-30 @ 13:54
On Mon, 2012-01-30 at 20:57 +1300, Sniffles wrote:
> Hi there people,
> 
>  
> 
> I’ve been looking at the libgit2 library over the last couple of days…
> 
>  
> 
> A bit of background from where I’m coming from so you don’t think that
> I’m banging on libgit2 (which I think is a long overdue answer to a
> fundamental architecture problem with git)
> 
>  
> 
> 1.       I’m a windows user/programmer – this doesn’t mean I want to
> start a windows/unix flavor war…
> 
> <
>  p class=MsoListParagraph> 
> 
> 2.       IMHO git for windows is a disgrace – why? Trying to call
> msysgit a windows native port is a flat out lie… sure it compiled on a

Indeed. Whoever says that should be called out on it. The msysgit
developers don't say it is. The only reference to "native" I've found is
the statement that msysgit was started to help make a native client.

>  windows machine but guess what it still requires a unix/linux
> emulation layer to sit on top of… the same goes for tortoisegit… what
> a joke… Forget about trying to set git up as a service on a windows
> machine
> 

git doesn't usually run as a service on other environments either
(assuming by "service" you mean the MS spelling of "daemon". You could
run git-daemon, but you usually don't actually want to run it). You have
your ssh daemon or your web server call git whenever necessary.
TortoiseGit needs git, so you need to install it. It's using libgit2
wherever it can.

>  
> 
> 3.       It is my opinion that the unix git tools are simply a p roof
> of concept (that works very well in a unix world but has a less than
> admirable windows ability), that is waiting to be properly cemented in
> a truly portable library… and things like command line tools and gui
> tools are simply thin shells leveraging the central library/core
> functions…

They are not proof of concept. They are tools many people use in
production. They have some legacy from the git-the-content-tracker era,
but that's a different issue.

The thing is, not everybody cares about Windows. A tool can be mature
and useful and yet not run particularly well on that family of systems.
People using Free OSs often have the same problem of there not being a
native port of an application we'd like or need to use, and for that we
use emulation layers. BSD has a Linux emulation layer for the
pathological cases.

> 
>  
> 
> I’m excited that libgit2 will actually be able to bring git to the
> windows masses as a proper and mature source control system and
> represent the next evolution of git beyond unix/linux shell commands…
> 
>  
> 
> On with the main thrust of this email…
> 
>  
> 
> Firstly:
> 
>  
> 
> I note that one of the open issues is the number of warnings when
> compiling under vs2010-winx64 to do with “conversion fro m ‘ssize_t’
> to ‘int’, possible loss of data” and various other warnings of this
> nature… which I’ve verified…

I must have missed the patch you sent fixing these warnings.

> 
>  
> 
> Now I note by looking at the source code and api that you are in fact
> targeting perhaps incorrectly…
> 
>  
> 
> Eg:
> 
>  
> 
> int git_buf_grow(git_buf *buf, size_t target_size)
> 
>  
> 
> is one of the functions i’ve pulled at random from the buffer.c file…
> 
>  
> 
> Note that the specific ment ion of ‘int’ as a return type
> automatically assumes 32-bit architecture…

Not at all. An int can have any size. It could be 64 or 128 bits wide
(we do tend to assume int is 32 bits wide in the library, but that's
just because we don't care about systems where an int is 16 bits).

> 
> This is for all api functions and all internal functions from a quick
> peruse of the code… and published api…
> 

It seems your peruse was a tad too hasty. Did you notice that we use the
return value as a success indicator? If we ever need more error codes
than an int can carry for us, I'll probably go into a cave and start
looking at shadows[0].

>  
> 
> I also note that internal local variables inside functions are also
> doing the same thing and assuming 32-bit…

This could be an issue, but as you haven't pointed it out, I can't
check. Using an int isn't always an error. Some things do fit in an int
(which again, we do tend to assume it's at least 32 bits wide, but it
could have any size).

> 
>  
> 
> 1.       Wouldn’t it be more prudent to actually use something like
> ‘size_t’ which automatically sizes to the target architect ure? This
> for all return types and ‘int’ style parameters??? Only actually use
> ‘int’ where it has be to 32-bit eg say in a ‘struct’ like a packet
> that gets passed across a network where it’s definitely got to be
> 32-bit… according to the protocol in use???

Network protocols have more things to worry about and again, an int is
not guaranteed to be 32 bits. If we ever were to transfer a struct
across the network, we'd have to specify the actual size (and transform
the number to network-endianness).

> 
> This would require all the api functions to be changed to: size_t
> git_function_to_do_something(git_type *parameter,……..)

The amount of error codes we use doesn't depend on the pointer size of
the processor, so we're definitely not going to do that.

> 
>  
> 
> I only say this as otherwise it is kind of pointless to be compiling
> for 64-bit architecture if you are just going to be using 32-bit
> internally anyway… You might as well only provide 32-bit targets and
> have the operating system worry about trying to run 32-bit on a 64-bit
> architecture… (which in the windows world is no biggie)

You're free to compile it as 32 bits if that's what works for you. We
don't provide binaries and we have said at some point that 64-bit
Windows isn't officially supported (we do want to get there).

> 
>  
> 
> Secondly:
> 
>  
> 
> I also note that a lot of the malloc/calloc etc… calls are not being
> type cast to the respective variables type… 
> 
>  
> 
>       new_ptr = git__realloc(new_ptr, new_size);
> 
>  
> 
> Where new_ptr is a char*
> 
>  
> 
> It should read:
> 
>  
> 
>                 new_ptr = (char*)git__realloc(new_ptr, new_size)
> 

No. This is simply wrong.

>  
> 
> If not, you get stupid intellisense errors… (might be a flaw of
> vs2010…) in the ide
> 

Your IDE seems to have the mistaken idea that you were looking at C++
code. libgit2 is written in C.

>  
> 
> Either that or a specific compiler warning has been turned off
> somewhere so the warnings don’t get spit out… during compilation (this
> is lazy…)

We turn off specific compiler warnings that report non-issues (like do
{} while (0) or while (1)), but most others are turned on. You are
however calling us lazy because you're not seeing some warnings that
aren't there in the first place.

> 
>  
> 
> Thirdly:
> 
>  
> 
> Packaging – Why isn’t this done yet??? Is there some master plan which
> you guys are working to??? Milestones planne d??? Etc… Can’t seem to
> see anything online regarding this???

What packaging? We provide source code for a library. If you want to use
libgit2 in your application, use it and bundle it with your
application[1]. The master plan is to make git "just another user" of
libgit2, much like svn is a human interface to the subversion library.
Whether we'll ever get there, I don't know. Meanwhile we're going to add
features and fix bugs.

> 
>  
> 
> Are you planning on writing/providing the equivalent thin shells (as
> mentioned above) to provide a complete git package/experience??? Or is
> this to be a separate thing??? I note that people are working on
> language bindings which is cool but what about being able to use the
> library from the command line etc… just like the normal unix/linux git
> tool(s)…

libgit2 is a librray, so no. I have a repository where I have a wrapper
so I can test code I've written at github.com/carlosmn/libgit2-utils and
some French students wrote a tool that would use libgit2 where possible
and fall back to git where not available. You could start there if you'd
like to have such a thing.

> 
>  
> 
> Fourthly:
> 
>  
> 
> What is the time frame for network capability and being able to hook
> this library up as a daemon and use as a windows service??? (god help
> me trying to do it the unix/linux way with ssh and all the other crap…
> ) svn simply kills git in this regard…
> 

We already have network capability. What we don't have is code to do a
push because we'd need to bring over the object packer from git and plug
it into the libgit2 methods, which is non-trivial.

AFAICT subversion has authentication, authorisation and svn-serving
bundled in the same binary when you run it over svn://. That works, I
guess. git is product of the Unix mantra "do one thing and do it well"
so it lets ssh handle the authentication. You can then use other layers
like gitolite to handle authorisation. JGit does have all of this
bundled together in a Java-pure way. Windows and Java are two thing I
don't like to use, so I've never tried to do it, but Gerrit should be
able to do all of this in Windows.

>  
> 
> Do I want to host propriety source code on github??? No I don’t…
> 
> Do I want to run my own git server which the developers can access?
> Why yes I do… But guess what – you have to be blessed by the gods to
> be able to do it on a windows machine… let me stick with svn because
> it is so easy to set up….

So when you say "on github" you mean "outside of immediate corporate
control" or "for everyone to see"? This has very little to do with
libgit2. Most git forges or the like probably won't work well on Windows
as they assume a somewhat posixy/unixy environment (which everything
else has). As said above, Gerrit might be what you're looking for.

If you think git on Windows is too complex to set up, then help make it
better. Complaining to a different project isn't going to help. If
setting up sshd on Windows is too complex, help the OpenSSH project make
it easier, propose a way to make it simpler.


   cmn

[0] Yes, that was a reference to platonic philosophy. I don't use my
high-school education often enough.

[1] Respecting the provisions of the library, obviously.


Re: [libgit2] winx64 compilation warnings and more...

From:
Charlie Sharpsteen
Date:
2012-01-30 @ 08:18
On Sun, Jan 29, 2012 at 11:57 PM, Sniffles <sniffles@xnet.co.nz> wrote:
> Now I note by looking at the source code and api that you are in fact
> targeting perhaps incorrectly…
>
>
>
> Eg:
>
>
>
> int git_buf_grow(git_buf *buf, size_t target_size)
>
>
>
> is one of the functions i’ve pulled at random from the buffer.c file…
>
>
>
> Note that the specific ment ion of ‘int’ as a return type automatically
> assumes 32-bit architecture…
>
> This is for all api functions and all internal functions from a quick peruse
> of the code… and published api…
>
>
>
> I also note that internal local variables inside functions are also doing
> the same thing and assuming 32-bit…
>
>
>
> 1.       Wouldn’t it be more prudent to actually use something like ‘size_t’
> which automatically sizes to the target architect ure? This for all return
> types and ‘int’ style parameters??? Only actually use ‘int’ where it has be
> to 32-bit eg say in a ‘struct’ like a packet that gets passed across a
> network where it’s definitely got to be 32-bit… according to the protocol in
> use???
>
> This would require all the api functions to be changed to: size_t
> git_function_to_do_something(git_type *parameter,……..)
>
>
>
> I only say this as otherwise it is kind of pointless to be compiling for
> 64-bit architecture if you are just going to be using 32-bit internally
> anyway… You might as well only provide 32-bit targets and have the operating
> system worry about trying to run 32-bit on a 64-bit architecture… (which in
> the windows world is no biggie)


This part jumped out at me. The `int` return code on those functions
is a simple status code---it could be a `char` if the libgit2
developers thought they wouldn't exceed 255 possible exit conditions
let alone four billion and change. Is there really a need to allow for
64-bit integers in this case?


-Charlie

Re: [libgit2] winx64 compilation warnings and more...

From:
Thomas Rast
Date:
2012-01-30 @ 08:46
I hope I'm not falling for a troll -- but I can at least do so as an
outsider/lurker ;-)

Sniffles <sniffles@xnet.co.nz> writes:

> 2.       IMHO git for windows is a disgrace - why? Trying to call msysgit a
> windows native port is a flat out lie.

Let's just hope not too many msysgit devs read this.  I also can't help
notice that "native" does not feature on the msysgit home page.

> I note that one of the open issues is the number of warnings when compiling
> under vs2010-winx64 to do with "conversion from 'ssize_t' to 'int', possible
> loss of data" and various other warnings of this nature. which I've
> verified.

Where?  Perhaps these are real warnings, but read on.

> Now I note by looking at the source code and api that you are in fact
> targeting perhaps incorrectly.
[...]
> int git_buf_grow(git_buf *buf, size_t target_size)

It took me all of five seconds (ok, and 30 more to update the repo)
looking at that function to see that it only returns a small handful of
return values.  So 'int', as the architecture's "convenient" type, is a
good choice here.

> 1.       Wouldn't it be more prudent to actually use something like 'size_t'
> which automatically sizes to the target architecture? This for all return
> types and 'int' style parameters??? Only actually use 'int' where it has be
> to 32-bit eg say in a 'struct' like a packet that gets passed across a
> network where it's definitely got to be 32-bit. according to the protocol in
> use???

You need to get your C knowledge straight.  size_t is for the size of
data structures and such, think of it as "big enough to say how big your
memory is".  OTOH basically nothing is guaranteed about the size of
'int', except that it's no less than a short and no more than a long.
(I'm sure someone can dig up a more precise definition.)  Together with
endianness issues this makes all[*] of the basic C types entirely
unsuitable for anything portability-related.

> I only say this as otherwise it is kind of pointless to be compiling for
> 64-bit architecture if you are just going to be using 32-bit internally
> anyway. You might as well only provide 32-bit targets and have the operating
> system worry about trying to run 32-bit on a 64-bit architecture. (which in
> the windows world is no biggie)

The real reason for 64-bit is that you want 64-bit *pointers* so you can
address more than 4GB of data.

  As a little anecdote, I have a HP49G graphic calculator.  Its puny
  processor runs at 4MHz, has 64-bit registers (!) and a lot of quirky
  features, such as half-byte (nibble) addressing and packed decimal
  math.  However, it has only 20-bit address registers and is thus
  properly called a 20-bit processor.

The real reason for 'int' is that it's the "convenient" type.  If the
compiler thinks a "convenient" type for the architecture has only 32
bits, why fight the compiler's decision?

> I also note that a lot of the malloc/calloc etc. calls are not being type
> cast to the respective variables type. 
>
>       new_ptr = git__realloc(new_ptr, new_size);
>
> Where new_ptr is a char*

Converting void* to foo* does not require a cast in C.


> Packaging - Why isn't this done yet???

This is open-source software.  It's done when it's done.  If you want it
done faster, you help.

Cheers,
Thomas


[*] yes, even char, which may be 8, 9, 12, 32 or 64 bits on sufficiently
arcane architectures

-- 
Thomas Rast
trast@{inf,student}.ethz.ch