git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
Search results ordered by [date|relevance]  view[summary|nested|Atom feed]
thread overview below | download mbox.gz: |
* Re: Breakage in master?
  2012-02-02 17:46  0% ` Jeff King
@ 2012-02-03 12:28  0%   ` Erik Faye-Lund
  0 siblings, 0 replies; 5+ results
From: Erik Faye-Lund @ 2012-02-03 12:28 UTC (permalink / raw)
  To: Jeff King; +Cc: Git Mailing List, msysGit, Ævar Arnfjörð

On Thu, Feb 2, 2012 at 6:46 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Feb 02, 2012 at 01:14:19PM +0100, Erik Faye-Lund wrote:
>
>> But here's the REALLY puzzling part: If I add a simple, unused
>> function to diff-lib.c, like this:
>> [...]
>> "git status" starts to error out with that same vsnprintf complaint!
>>
>> ---8<---
>> $ git status
>> # On branch master
>> # Changes not staged for commit:
>> #   (use "git add <file>..." to update what will be committed)
>> fatal: BUG: your vsnprintf is broken (returned -1)
>> ---8<---
>
> OK, that's definitely odd.
>
> At the moment of the die() in strbuf_vaddf, what does errno say?

If I apply this patch:
---8<---
diff --git a/strbuf.c b/strbuf.c
index ff0b96b..52dfdd6 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -218,7 +218,7 @@ void strbuf_vaddf(struct strbuf *sb, const char
*fmt, va_list ap)
 	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);
 	va_end(cp);
 	if (len < 0)
-		die("BUG: your vsnprintf is broken (returned %d)", len);
+		die_errno("BUG: your vsnprintf is broken (returned %d)", len);
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
---8<---

Then I get "fatal: BUG: your vsnprintf is broken (returned -1): Result
too large". This goes both for both failure cases I described. I
assume this means errno=ERANGE.

> vsnprintf should generally never be returning -1 (it should return the
> number of characters that would have been written). Since you're on
> Windows, I assume you're using the replacement version in
> compat/snprintf.c.

No. SNPRINTF_RETURNS_BOGUS is only set for the MSVC target, not for
the MinGW target. I'm assuming that means MinGW-runtime has a sane
vsnprintf implementation. But even if I enable SNPRINTF_RETURNS_BOGUS,
the problem occurs. And it's still "Result too large".

So I decided to do a bit of stepping, and it seems libintl takes over
vsnprintf, directing us to libintl_vsnprintf instead. I guess this is
so it can ensure we support reordering the parameters with $1 etc...
And aparently this vsnprintf implementation calls the system vnsprintf
if the format string does not contain '$', and it's using _vsnprintf
rather than vsnprintf on Windows. _vsnprintf is the MSVCRT-version,
and not the MinGW-runtime, which needs SNPRINTF_RETURNS_BOGUS.

So I guess I can patch libintl to call vsnprintf from MinGW-runtime instead.

> All of that would make sense to me, _except_ for your weird "if I add a
> random function, the problem is more reproducible" bit. Which does seem
> like something is invoking undefined behavior (of course, it could be
> that undefined behavior or stack-smashing that is causing vsnprintf to
> report an error). Lacking any better leads, it might be worth pursuing.

Well, now at least I have some better leads, but I'm still not able to
explain the "if I add a random function, the problem is more
reproducible" bit. :(

>> I've bisected the issues down to 5e9637c (i18n: add infrastructure for
>> translating Git with gettext). Trying to apply my unused-function
>> patch on top of this commit starts giving the same "fatal: BUG: your
>> vsnprintf is broken (returned -1)" error. It's ancestor, bc1bbe0(Git
>> 1.7.8-rc2), does not yield any of the issues.
>
> I've looked at 5e9637c, and it really doesn't do anything that looks
> bad. I wonder if your gettext library is buggy. Does compiling with
> NO_GETTEXT help?

Compiling with NO_GETTEXT does make the symptoms go away, but that's
not curing the problem ;)

But, I have a lead now. I'll see if I can find out *why* libintl calls
_vsnprintf on MinGW. I expect it's so the MSVC and the MinGW versions
behave similarly, MSVC doesn't have a sane vsnprintf. Perhaps I should
back-port SNPRINTF_RETURNS_BOGUS-workaround to libintl, so our MSVC
builds doesn't break also?

^ permalink raw reply related	[relevance 0%]

* Re: Breakage in master?
  2012-02-02 12:14  3% Breakage in master? Erik Faye-Lund
@ 2012-02-02 17:46  0% ` Jeff King
  2012-02-03 12:28  0%   ` Erik Faye-Lund
  0 siblings, 1 reply; 5+ results
From: Jeff King @ 2012-02-02 17:46 UTC (permalink / raw)
  To: Erik Faye-Lund
  Cc: Git Mailing List, msysGit, Ævar Arnfjörð Bjarmason

On Thu, Feb 02, 2012 at 01:14:19PM +0100, Erik Faye-Lund wrote:

> But here's the REALLY puzzling part: If I add a simple, unused
> function to diff-lib.c, like this:
> [...]
> "git status" starts to error out with that same vsnprintf complaint!
> 
> ---8<---
> $ git status
> # On branch master
> # Changes not staged for commit:
> #   (use "git add <file>..." to update what will be committed)
> fatal: BUG: your vsnprintf is broken (returned -1)
> ---8<---

OK, that's definitely odd.

At the moment of the die() in strbuf_vaddf, what does errno say?
vsnprintf should generally never be returning -1 (it should return the
number of characters that would have been written). Since you're on
Windows, I assume you're using the replacement version in
compat/snprintf.c.

That one will return -1 if realloc fails. So I'm curious if that is what
is happening (you might also instrument the call to realloc in
snprintf.c to see if it is failing, and if so, at what maxsize). And/or
check errno in git_vsnprintf after calling the native vsnprintf and
getting -1.

Here's one possible sequence of events that seems plausible to me (and
remember that this is a wild guess):

  1. gettext somehow munges the format string in a way that Windows
     vsnprintf doesn't like, and it returns -1.

  2. Our git_vsnprintf wrapper interprets this -1 as "you didn't give me
     enough space to store the result", and we grow our test-buffer to
     try again

  3. Eventually the test buffer gets unreasonably large, and realloc
     fails. We have no choice but to return -1 from our wrapper.

  4. strbuf_vaddf sees the -1 and thinks you are using a broken
     vsnprintf.

All of that would make sense to me, _except_ for your weird "if I add a
random function, the problem is more reproducible" bit. Which does seem
like something is invoking undefined behavior (of course, it could be
that undefined behavior or stack-smashing that is causing vsnprintf to
report an error). Lacking any better leads, it might be worth pursuing.

> I've bisected the issues down to 5e9637c (i18n: add infrastructure for
> translating Git with gettext). Trying to apply my unused-function
> patch on top of this commit starts giving the same "fatal: BUG: your
> vsnprintf is broken (returned -1)" error. It's ancestor, bc1bbe0(Git
> 1.7.8-rc2), does not yield any of the issues.

I've looked at 5e9637c, and it really doesn't do anything that looks
bad. I wonder if your gettext library is buggy. Does compiling with
NO_GETTEXT help?

-Peff

^ permalink raw reply	[relevance 0%]

* Breakage in master?
@ 2012-02-02 12:14  3% Erik Faye-Lund
  2012-02-02 17:46  0% ` Jeff King
  0 siblings, 1 reply; 5+ results
From: Erik Faye-Lund @ 2012-02-02 12:14 UTC (permalink / raw)
  To: Git Mailing List, msysGit, Ævar Arnfjörð Bjarmason

Something strange is going on in Junio's current 'master' branch
(f3fb075). "git show" has started to error out on Windows with a
complaint about our vsnprintf:
---8<---

$ git show
commit f3fb07509c2e0b21b12a598fcd0a19a92fc38a9d
Author: Junio C Hamano <gitster@pobox.com>
Date:   Tue Jan 31 22:31:35 2012 -0800

    Update draft release notes to 1.7.10

    Signed-off-by: Junio C Hamano <gitster@pobox.com>

fatal: BUG: your vsnprintf is broken (returned -1)
---8<---

"git status" is also behaving strange:
---8<---
$ git status
# On branch master
# Untracked files:
#   (use "git add <file>..." to include in what will be committed)
#
#       ←[31mcompat/vcbuild/include/sys/resource.h←[m
#       ←[31mtemp.patch←[m
#       ←[31mtest.c←[m
#       ←[31mtest.patch←[m
nothing added to commit but untracked files present (use "git add" to track)
---8<---

Yeah, the ANSI color codes are being printed verbatim, even though
compat/winansi.c is supposed to convert these. "git -p status" works
fine, as it pipes the ANSI codes directly through less.

But here's the REALLY puzzling part: If I add a simple, unused
function to diff-lib.c, like this:

---8<---
diff --git a/diff-lib.c b/diff-lib.c
index fc0dff3..914a224 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -82,6 +82,11 @@ static int match_stat_with_submodule(struct
diff_options *diffopt,
 	return changed;
 }

+static unsigned int foo(const char *a, unsigned int b)
+{
+	return b;
+}
+
 int run_diff_files(struct rev_info *revs, unsigned int option)
 {
 	int entries, i;
---8<---

"git status" starts to error out with that same vsnprintf complaint!

---8<---
$ git status
# On branch master
# Changes not staged for commit:
#   (use "git add <file>..." to update what will be committed)
fatal: BUG: your vsnprintf is broken (returned -1)
---8<---

Here's the stack-trace:
---8<---
Breakpoint 1, die (err=0x5268ec "BUG: your vsnprintf is broken (returned %d)")
    at usage.c:81
81      void NORETURN die(const char *err, ...)
(gdb) bt
#0  die (err=0x5268ec "BUG: your vsnprintf is broken (returned %d)")
    at usage.c:81
#1  0x00466314 in strbuf_vaddf (sb=0x28fb54,
    fmt=0x533ef4 "  (use \"git checkout -- <file>...\" to discard changes in wor
king directory)", ap=0x28fbac "+\025\026\002") at strbuf.c:221
#2  0x004cbb6f in status_vprintf (s=0x28fc38, at_bol=0,
    color=0x46c4f2 "\205└t\n\203─\020[^╔├\215v",
    fmt=0x533ef4 "  (use \"git checkout -- <file>...\" to discard changes in wor
king directory)", ap=0x28fbac "+\025\026\002", trail=0x533a89 "\n")
    at wt-status.c:44
#3  0x004cbe6b in status_printf_ln (s=0x28fc38, color=0x28fc70 "",
    fmt=0x533ef4 "  (use \"git checkout -- <file>...\" to discard changes in wor
king directory)") at wt-status.c:87
#4  0x004cced1 in wt_status_print (s=0x28fc38) at wt-status.c:176
#5  0x0041922e in cmd_status (argc=1, argv=0x319b4, prefix=0x0)
    at builtin/commit.c:1254
#6  0x004019d6 in handle_internal_command (argc=<value optimized out>,
    argv=<value optimized out>) at git.c:308
#7  0x00401c26 in main (argc=2, argv=0x319b0) at git.c:513
(gdb)
---8<---

This smells a bit like a smashed stack to me. Both issues happens in
roughly the same area of the call stack, and when adding an unused
function changes behavior, something really odd is going on ;)

I've bisected the issues down to 5e9637c (i18n: add infrastructure for
translating Git with gettext). Trying to apply my unused-function
patch on top of this commit starts giving the same "fatal: BUG: your
vsnprintf is broken (returned -1)" error. It's ancestor, bc1bbe0(Git
1.7.8-rc2), does not yield any of the issues.

I'm at a loss here. Does anyone have a hunch about what's going on?

^ permalink raw reply related	[relevance 3%]

* Re: [PATCH 00/28] Store references hierarchically in cache -- benchmark results
  @ 2011-11-16 12:51  5% ` Michael Haggerty
  0 siblings, 0 replies; 5+ results
From: Michael Haggerty @ 2011-11-16 12:51 UTC (permalink / raw)
  To: mhagger
  Cc: Junio C Hamano, git, Jeff King, Drew Northup, Jakub Narebski,
	Heiko Voigt, Johan Herland, Julian Phillips

On 10/28/2011 02:28 PM, mhagger@alum.mit.edu wrote:
> [...]
> This patch series changes how references are stored in the reference
> cache data structures (entirely internal to refs.c).  Previously, the
> packed refs were stored in one big array of pointers-to-struct, and
> the loose refs were stored in another.  [...]
> 
> Therefore, this patch series changes the data structure used to store
> the reference cache from a single array of pointers-to-struct into a
> tree-like structure in which each subdirectory of the reference
> namespace is stored as an array of pointers-to-entry and entries can
> be either references or subdirectories containing more references.
> Moreover, each subdirectory of loose references is only read from disk
> when a reference from that subdirectory (or one of its descendants) is
> needed.  This slightly slows down commands that need to iterate
> through all references (simply because the new data structures are
> more complicated), but it *dramatically* decreases the time needed for
> some common operations.  For example, in a test repository with 20000
> revisions and 10000 loose tags:

Due to the death of my old computer, I got distracted and never
published the benchmark results that I cited above.  (The numbers here
are different than the originals because they are done on a different
computer.)  The benchmarks are done using code from [1]; the test
repository consists of a linear series of 20000 commits (with very
little content) and 10000 tags (on every second commit); the tags are
unsharded.  The numbers are times in seconds; "cold" means that the disk
cache was flushed immediately before the test; "warm" usually means that
the same operation was done a second time.

Column #0 is Git 1.7.7.2; column #1 is Git 1.7.8-rc2; column #2 is a
recent git master plus my ref-related patch series (what Junio calls
mh/ref-api-take-2 plus the fix that I posted yesterday).

The main change between 1.7.7.2 and 1.7.8 is that the latter stores the
reference cache in a sorted array rather than a linked list, meaning
that it is possible to use bisection to locate a reference quickly by
name rather than having to search linearly through a linked list.  This
greatly helps some operations when most references are packed and can be
read from disk quickly.  It doesn't make much difference when most
references are loose, because the time required to read the loose
references from disk overwhelms the time for iterating through the array
in memory.

The main improvements between 1.7.8 and the new version are when the old
code would have read all of the loose references but the new code only
needs to read a couple of directories of them.  Given that almost all of
the references in the test repository are tags, they often don't need to
be read at all when branches are being manipulated.  By contrast, many
old code paths force *all* of the references to be read, for example
when they check for replacements in refs/replace/.

I've done other benchmarks with varying numbers of references.  The
results suggest that many operations go from O(N) in the number of loose
references to O(1) (e.g., if all they do is check refs/replace/) or some
other slow scaling that depends on how the reference namespace is organized.

Anything involving packed references necessarily scales at least like
O(N) because packed references are all read at once.  OTOH reading
packed refs is so much faster than reading loose references that with
10000 references, packing is still advantageous.  (For some number of
references, of course, the curves must cross and loose references will
be more efficient than packed references for some operations.)

The case of git-filter-branch is particularly dramatic; the old code
scales like O(N^2) whereas the new code scales like O(N) as expected.
Moreover, git-filter-branch creates lots of loose references while it
works, so it is not possible to evade the problem by packing the
references before invoking git-filter-branch.  I believe that
git-filter-branch is mostly slowed down be each subcommand's check for
replacements in refs/replace (even though there are none in my test
repository) because in the old code this check forces *all* loose
references to be read.  Versions 1.7.7 and 1.7.8 of git-filter-branch
runs much faster if the --no-replace-objects option is used.

With these changes, it becomes thinkable to work with repositories with
very large numbers of references (especially if the reference namespace
is sharded appropriately), whereas in 1.7.7 some operations were
annoyingly slow.

Michael

[1] branch "refperf" at git://github.com/mhagger/git.git

> ===================================  ========  ========  ========
> Test name                                 [0]       [1]       [2]
> ===================================  ========  ========  ========
> branch-loose-cold                        1.59      1.68      0.29
> branch-loose-warm                        0.04      0.04      0.00
> for-each-ref-loose-cold                  1.86      1.96      1.88
> for-each-ref-loose-warm                  0.10      0.10      0.11
> checkout-loose-cold                      1.66      1.86      0.39
> checkout-loose-warm                      0.04      0.05      0.01
> checkout-orphan-loose                    0.04      0.04      0.00
> checkout-from-detached-loose-cold        2.04      2.11      1.81
> checkout-from-detached-loose-warm        0.24      0.15      0.16
> branch-contains-loose-cold               1.79      1.86      1.87
> branch-contains-loose-warm               0.14      0.14      0.14
> pack-refs-loose                          0.49      0.53      0.53
> branch-packed-cold                       0.28      0.25      0.25
> branch-packed-warm                       0.02      0.00      0.00
> for-each-ref-packed-cold                 0.34      0.39      0.40
> for-each-ref-packed-warm                 0.07      0.07      0.07
> checkout-packed-cold                     2.81      0.50      0.55
> checkout-packed-warm                     0.01      0.00      0.01
> checkout-orphan-packed                   0.00      0.00      0.00
> checkout-from-detached-packed-cold       2.83      0.55      0.46
> checkout-from-detached-packed-warm       2.45      0.12      0.13
> branch-contains-packed-cold              0.38      0.32      0.43
> branch-contains-packed-warm              0.11      0.11      0.11
> clone-loose-cold                        30.16     30.31     30.51
> clone-loose-warm                         1.28      1.30      1.33
> fetch-nothing-loose                      0.21      0.39      0.38
> pack-refs                                0.14      0.12      0.14
> fetch-nothing-packed                     0.21      0.40      0.39
> clone-packed-cold                        1.07      1.24      1.18
> clone-packed-warm                        0.23      0.23      0.22
> fetch-everything-cold                   30.49     30.89     31.09
> fetch-everything-warm                    1.78      2.01      2.06
> filter-branch-warm                    2949.81   2891.51    440.60
> ===================================  ========  ========  ========
> 
> 
> [0] 8d19b44 (tag: v1.7.7.2) Git 1.7.7.2
>     Test repository created using: t/make-refperf-repo --commits 20000 --refs 10000
> [1] bc1bbe0 (tag: v1.7.8-rc2) Git 1.7.8-rc2
>     Test repository created using: t/make-refperf-repo --commits 20000 --refs 10000
> [2] 01494b4 (github/ref-api-D) repack_without_ref(): call clear_packed_ref_cache()
>     Test repository created using: t/make-refperf-repo --commits 20000 --refs 10000



-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply	[relevance 5%]

* [ANNOUNCE] Git 1.7.8.rc2
@ 2011-11-13  7:27 14% Junio C Hamano
  0 siblings, 0 replies; 5+ results
From: Junio C Hamano @ 2011-11-13  7:27 UTC (permalink / raw)
  To: git; +Cc: Linux Kernel

A release candidate Git 1.7.8.rc2 is available for testing.

Since rc0, we killed all known regressions. Because there won't be any
more new feature merged until the 1.7.8 final, it is a good time for the
coolest kids on the block to start using the upcoming release before
others do.

The release tarballs are found at:

    http://code.google.com/p/git-core/downloads/list

and their SHA-1 checksums are:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

b1cb030dee2b9ae024f4076fe5fadfea43edec4e  git-1.7.8.rc2.tar.gz
cd30ce92f9518920ff8f9cdde0a8da5c856f6193  git-htmldocs-1.7.8.rc2.tar.gz
97d72c0c56e557eb3f11b9a3dcdb971e38eaee49  git-manpages-1.7.8.rc2.tar.gz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQIcBAEBAgAGBQJOv287AAoJELC16IaWr+bLGJwP/1ZL+9NcBiIWshqxrYaBACF6
dgLsK1M7iJBn95ye6xBp57uPeLGK9iv5qLvo5Wxoog8raWgceR3qXjZL3snfYlLO
Hz2P8Zb9EF80QfVs7RjkQYAJRaT/WzbSxoQF/ZUXHyLk2BHpw6YfYA0Vj0JJZ72l
bCYMnppi18uxLuUyf6/0ftlkadxv3L58VIaNWAp8NwuLuskucx64LZgYzwkaRFfO
YNWgV0zwimK+9SF/gnQ5cw55GCurs69HWoDVLJbnQuJbjiU3Kl9jehYBwTo/rPSn
vo03Foh3mV6u3DTovZARLF54goJvvc+JKKkFAdeY+dNKLdZQRdO+hrldGTeGKxVk
XissJxmsHm/DkXc15yIPu+iV5wmAXVc8BpTzK1NgleuOyz2qnvVrmY+ZOVQfM1BA
4zS6PMp/sgEJa6ybmwXuYY/JpJlgmEBsDk6MJOJ8RKt+q0qovB+2ltMJbFpqVnQR
VEswTvhBOmUSfKhiY68UmqE4H6vmSO5yOmo1VQKgCKzN7glcG/3wpIHNK9+QW9yE
eOQMSDdBQGGgzz7Y+bVwpbOpSUsc49MYit8T9wx/haNNCA3Fud1UUKbT+060/zQw
Tgv2gi/6L/vCcDhj7oersNYfpgilggYBeiADKsI1fw6SHzBpbWblJZTtjr1yplaZ
YG+o9yPd2EwA2tkosG51
=hXu/
-----END PGP SIGNATURE-----

Also the following public repositories all have a copy of the v1.7.8.rc2
tag and the master branch that the tag points at:

  url = git://repo.or.cz/alt-git.git
  url = https://code.google.com/p/git-core/
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

----------------------------------------------------------------

Changes since v1.7.8-rc1 are as follows:

Felipe Contreras (1):
      remote: fix remote set-url usage

Jeff King (1):
      docs: don't mention --quiet or --exit-code in git-log(1)

Junio C Hamano (5):
      remote: fix set-branches usage
      docs: Update install-doc-quick
      Git 1.7.7.3
      Update draft release notes to 1.7.8
      Git 1.7.8-rc2

Liu Yuan (1):
      mktree: fix a memory leak in write_tree()

SZEDER Gábor (1):
      completion: don't leak variable from the prompt into environment

^ permalink raw reply	[relevance 14%]

Results 1-5 of 5 | reverse | options above
-- pct% links below jump to the message on this page, permalinks otherwise --
2011-10-28 12:28     [PATCH 00/28] Store references hierarchically in cache mhagger
2011-11-16 12:51  5% ` [PATCH 00/28] Store references hierarchically in cache -- benchmark results Michael Haggerty
2011-11-13  7:27 14% [ANNOUNCE] Git 1.7.8.rc2 Junio C Hamano
2012-02-02 12:14  3% Breakage in master? Erik Faye-Lund
2012-02-02 17:46  0% ` Jeff King
2012-02-03 12:28  0%   ` Erik Faye-Lund

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).