Re: [PATCH v1] Travis: also test on 32-bit Linux
Am 11.03.2017 um 00:33 schrieb Junio C Hamano: > René Scharfe writes: > >> @ depends on r @ >> expression E; >> @@ >> - *& >>E > > I guess my source of the confusion is that the tool that understands > the semantics of the C language still needs to be told about that. Understanding that E and *&E have the same type does not imply (for a machine) that E alone is preferable. > I was hoping that something that understands C only needs to be told > only a single rule: > > type T > T src, dst > > -memcpy(&dst, &src, sizeof(dst)); > +dst = src; > > and then can apply that rule to this code in four ways: > > struct foo A, *Bp; > > memcpy(Bp, &A, sizeof(*Bp)); > memcpy(Bp, &A, sizeof(A)); > memcpy(&src, dstp, sizeof(A)); > memcpy(&src, dstp, sizeof(*Bp)); I guess src is A and dstp is Bp? Coccinelle does not know that the sizeof expressions are equivalent, but you could normalize them with an additional rule and then use a single memcpy transformation like this: @ normalize_sizeof @ type T; T var; @@ - sizeof(var) + sizeof(T) @ r @ type T; T *dst; T *src; @@ - memcpy(dst, src, sizeof(T)); + *dst = *src; @ depends on r @ expression E; @@ - *& E That would give you what you expected here: > to obtain its rewrite: > > struct foo A, *Bp; > > *Bp = A; > *Bp = A; > A = *Bp; > A = *Bp; But that normalization rule would be applied everywhere because it's so broad, and that's probably not what we want. You could replace it with an isomorphism like this one: Expression @@ type T; T E; @@ sizeof(T) => sizeof E In your example it allows you to specify sizeof(struct foo) (or a generic sizeof(T) as in rule r above) in the semantic patch and Coccinelle would let that match sizeof(A) and sizeof(*Bp) in the C file as well. Isomorphisms are kept in a separate file, the default one is /usr/lib/coccinelle/standard.iso on my machine and you can chose a different one with --iso-file. Perhaps we want to have our own for git, but we'd need to synchronize it with upstream from time to time. There is already a very similar isomorphism rule in the default file, but I don't think I understand it: Expression @ sizeof_type_expr @ pure type T; // pure because we drop a metavar T E; @@ sizeof(T) => sizeof E In particular I'm a bit worried about the lack of documentation for "pure", and I don't understand the comment. There's another comment at the top of another rule in the same file: // pure means that either the whole field reference expression is dropped, // or E is context code and has no attached + code // not really... pure means matches a unitary unplussed metavariable // but this rule doesn't work anyway Hmm. Here's an isomorphism that allows you to use "sizeof *src" (the third part for T is not strictly needed for your example): Expression @ sizeof_equiv @ type T; T E1, E2; @@ sizeof E1 <=> sizeof E2 <=> sizeof(T) That's the kind of bidirectional equivalence you expected to be part of Coccinelle's standard set of rules, right? With that rule in place the following semantic patch matches your four cases: @ r @ type T; T *dst; T *src; @@ - memcpy(dst, src, sizeof *dst); + *dst = *src; @ depends on r @ expression E; @@ - *& E But I'd be careful with that as long as "pure" is present in that standard rule and not fully understood. The isomorphism sizeof_equiv doesn't seem to cause any trouble with our existing semantic patches and the code in master, though. René
Re: [PATCH v1] Travis: also test on 32-bit Linux
René Scharfe writes: > @ depends on r @ > expression E; > @@ > - *& > E I guess my source of the confusion is that the tool that understands the semantics of the C language still needs to be told about that. I was hoping that something that understands C only needs to be told only a single rule: type T T src, dst -memcpy(&dst, &src, sizeof(dst)); +dst = src; and then can apply that rule to this code in four ways: struct foo A, *Bp; memcpy(Bp, &A, sizeof(*Bp)); memcpy(Bp, &A, sizeof(A)); memcpy(&src, dstp, sizeof(A)); memcpy(&src, dstp, sizeof(*Bp)); to obtain its rewrite: struct foo A, *Bp; *Bp = A; *Bp = A; A = *Bp; A = *Bp; by knowing that (*Bp) is of type "struct foo" (even though Bp is of type "struct foo *") and sizeof(dst) and sizeof(src) are the same thing in the rule because src and dst are both of type T.
Re: [PATCH v1] Travis: also test on 32-bit Linux
Am 10.03.2017 um 21:13 schrieb Junio C Hamano: René Scharfe writes: I think this misses the other two cases: (*dst, src) and (*dst, *src). ... and that's why I left them out. You can't get dst vs. *dst wrong with structs (at least not without the compiler complaining); only safe transformations are included in this round. I haven't followed this discussion to the end, but the omission of 2 out of obvious 4 did pique my curiosity when I saw it, too, and made me wonder if the omission was deliberate. If so, it would be nice to state why in the log message (or in copy.cocci file itself as a comment). It also made me wonder if we would be helped with a further combinatorial explosion from "T **dstp, **srcp" and somesuch (in other words, I am wondering why a rule for 'T *src' that uses '*src' need to be spelled out separately when there already is a good rule for 'T src' that uses 'src'---is that an inherent restriction of the tool?). There are redundancies. This semantic patch here: @@ type T; T dst; T *src; @@ - memcpy(&dst, src, sizeof(dst)); + dst = *src; would match e.g. this (from convert.c): memcpy(&new_stats, &stats, sizeof(new_stats)); and transform it to: new_stats = *&stats; We'd need just one more rule to remove the "*&" part and could then get rid of two of the "T src" variants, to arrive at something like this: @@ type T; T dst, src; @@ - memcpy(&dst, &src, sizeof(src)); + dst = src; @ r @ type T; T dst; T *src; @@ ( - memcpy(&dst, src, sizeof(dst)); + dst = *src; | - memcpy(&dst, src, sizeof(*src)); + dst = *src; | - memcpy(&dst, src, sizeof(T)); + dst = *src; ) @ depends on r @ expression E; @@ - *& E René
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Fri, Mar 10, 2017 at 12:13:11PM -0800, Junio C Hamano wrote: > René Scharfe writes: > > >> I think this misses the other two cases: (*dst, src) and (*dst, *src). > > > > ... and that's why I left them out. You can't get dst vs. *dst wrong > > with structs (at least not without the compiler complaining); only > > safe transformations are included in this round. > > I haven't followed this discussion to the end, but the omission of 2 > out of obvious 4 did pique my curiosity when I saw it, too, and made > me wonder if the omission was deliberate. If so, it would be nice > to state why in the log message (or in copy.cocci file itself as a > comment). Yeah, it definitely would be worth mentioning. I'm still undecided on whether we want to be endorsing struct assignment more fully. > It also made me wonder if we would be helped with a further > combinatorial explosion from "T **dstp, **srcp" and somesuch (in > other words, I am wondering why a rule for 'T *src' that uses '*src' > need to be spelled out separately when there already is a good rule > for 'T src' that uses 'src'---is that an inherent restriction of the > tool?). I had that thought, too, but I think the 4-way rules are necessary, because the transformations aren't the same in each case. E.g., for the four cases, the resulting assignments are: (dst, src): dst = src; (dst, *src): dst = *src; (*dst, src): *dst = src; (*dst, *src): *dst = *src; For pointer-to-pointer, I assumed the tool would handle that automatically by matching "T" as "T*". Though if that is the case, I think "(dst, src)" and "(*dst, *src)" would be equivalent (though of course our rule matches are different, as you do not memcpy the raw structs). -Peff
Re: [PATCH v1] Travis: also test on 32-bit Linux
René Scharfe writes: >> I think this misses the other two cases: (*dst, src) and (*dst, *src). > > ... and that's why I left them out. You can't get dst vs. *dst wrong > with structs (at least not without the compiler complaining); only > safe transformations are included in this round. I haven't followed this discussion to the end, but the omission of 2 out of obvious 4 did pique my curiosity when I saw it, too, and made me wonder if the omission was deliberate. If so, it would be nice to state why in the log message (or in copy.cocci file itself as a comment). It also made me wonder if we would be helped with a further combinatorial explosion from "T **dstp, **srcp" and somesuch (in other words, I am wondering why a rule for 'T *src' that uses '*src' need to be spelled out separately when there already is a good rule for 'T src' that uses 'src'---is that an inherent restriction of the tool?).
Re: [PATCH v1] Travis: also test on 32-bit Linux
Am 10.03.2017 um 18:57 schrieb Jeff King: On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote: I think this misses the other two cases: (*dst, src) and (*dst, *src). ... and that's why I left them out. You can't get dst vs. *dst wrong with structs (at least not without the compiler complaining); only safe transformations are included in this round. I don't think the transformation could be wrong without the original code being wrong. Avoiding to introduce bugs with automatic transformations is essential, indeed -- if we're not careful here we'd be better off keeping the original code. I'm also not sure why mine would be unsafe and yours would be safe. It seems like the other way around, because mine will do: *dst = ... which would fail unless "dst" is a pointer. Whereas in yours, we end up with: dst = ... and somebody mistaking pointer assignment for a struct copy would not notice. If dst is a struct then having *dst on the left-hand side results in a compiler error -- you can't get that one wrong. If it's a pointer then both dst and *dst can be valid (pointer assignment vs. content copy), so there is the possibility of making a mistake without the compiler noticing. But either way, I don't think we can have a rule like "you can use struct assignment only if you don't have a pointer for the destination". That's too arcane to expect developers and reviewers to follow. We should either allow struct assignment or not. With an automatic transformation in place it's more like "you can't use memcpy() in this case any more as it gets turned into an assignment with the next cocci patch". I think we shouldn't be that restrictive for pointers as destinations (yet?). René
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Fri, Mar 10, 2017 at 05:20:13PM +0100, René Scharfe wrote: > > I think this misses the other two cases: (*dst, src) and (*dst, *src). > > ... and that's why I left them out. You can't get dst vs. *dst wrong with > structs (at least not without the compiler complaining); only safe > transformations are included in this round. I don't think the transformation could be wrong without the original code being wrong. I'm also not sure why mine would be unsafe and yours would be safe. It seems like the other way around, because mine will do: *dst = ... which would fail unless "dst" is a pointer. Whereas in yours, we end up with: dst = ... and somebody mistaking pointer assignment for a struct copy would not notice. But either way, I don't think we can have a rule like "you can use struct assignment only if you don't have a pointer for the destination". That's too arcane to expect developers and reviewers to follow. We should either allow struct assignment or not. Or have I totally misunderstood your point? -Peff
Re: [PATCH v1] Travis: also test on 32-bit Linux
Am 10.03.2017 um 09:18 schrieb Jeff King: On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote: 2. Ones which just copy a single object, like: memcpy(&dst, &src, sizeof(dst)); Perhaps we should be using struct assignment like: dst = src; here. It's safer and it should give the compiler more room to optimize. The only downside is that if you have pointers, it is easy to write "dst = src" when you meant "*dst = *src". Compilers can usually inline memcpy(3) calls, but assignments are shorter and more pleasing to the eye, and we get a type check for free. How about this? Yeah, I mostly wasn't sure how people felt about "shorter and more pleasing". It _is_ shorter and there's less to get wrong. But the memcpy() screams "hey, I am making a copy" and is idiomatic to at least a certain generation of C programmers. I guess something like COPY(dst, src) removes the part that you can get wrong, while still screaming copy. It's not idiomatic either, but at least it stands out. I dunno. Yes ... I think this misses the other two cases: (*dst, src) and (*dst, *src). ... and that's why I left them out. You can't get dst vs. *dst wrong with structs (at least not without the compiler complaining); only safe transformations are included in this round. René
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Fri, Mar 10, 2017 at 01:14:16AM +0100, René Scharfe wrote:
> > 2. Ones which just copy a single object, like:
> >
> >memcpy(&dst, &src, sizeof(dst));
> >
> > Perhaps we should be using struct assignment like:
> >
> >dst = src;
> >
> > here. It's safer and it should give the compiler more room to
> > optimize. The only downside is that if you have pointers, it is
> > easy to write "dst = src" when you meant "*dst = *src".
>
> Compilers can usually inline memcpy(3) calls, but assignments are
> shorter and more pleasing to the eye, and we get a type check for
> free. How about this?
Yeah, I mostly wasn't sure how people felt about "shorter and more
pleasing". It _is_ shorter and there's less to get wrong. But the
memcpy() screams "hey, I am making a copy" and is idiomatic to at least
a certain generation of C programmers.
I guess something like COPY(dst, src) removes the part that you can get
wrong, while still screaming copy. It's not idiomatic either, but at
least it stands out. I dunno.
> diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
> new file mode 100644
> index 00..f0d883932a
> --- /dev/null
> +++ b/contrib/coccinelle/copy.cocci
> @@ -0,0 +1,31 @@
> +@@
> +type T;
> +T dst;
> +T src;
> [...]
> +type T;
> +T dst;
> +T *src;
I think this misses the other two cases: (*dst, src) and (*dst, *src).
Perhaps squash this in?
---
builtin/blame.c | 2 +-
builtin/pack-redundant.c | 4 ++--
contrib/coccinelle/copy.cocci | 32
diff.c| 4 ++--
dir.c | 2 +-
fast-import.c | 6 +++---
line-log.c| 2 +-
7 files changed, 42 insertions(+), 10 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..d0d917b37 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -680,7 +680,7 @@ static void dup_entry(struct blame_entry ***queue,
{
origin_incref(src->suspect);
origin_decref(dst->suspect);
- memcpy(dst, src, sizeof(*src));
+ *dst = *src;
dst->next = **queue;
**queue = dst;
*queue = &dst->next;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844..ac1d8111e 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -207,7 +207,7 @@ static inline struct pack_list * pack_list_insert(struct
pack_list **pl,
struct pack_list *entry)
{
struct pack_list *p = xmalloc(sizeof(struct pack_list));
- memcpy(p, entry, sizeof(struct pack_list));
+ *p = *entry;
p->next = *pl;
*pl = p;
return p;
@@ -451,7 +451,7 @@ static void minimize(struct pack_list **min)
while (perm) {
if (is_superset(perm->pl, missing)) {
new_perm = xmalloc(sizeof(struct pll));
- memcpy(new_perm, perm, sizeof(struct pll));
+ *new_perm = *perm;
new_perm->next = perm_ok;
perm_ok = new_perm;
}
diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
index f0d883932..da9969c84 100644
--- a/contrib/coccinelle/copy.cocci
+++ b/contrib/coccinelle/copy.cocci
@@ -29,3 +29,35 @@ T *src;
- memcpy(&dst, src, sizeof(T));
+ dst = *src;
)
+
+@@
+type T;
+T *dst;
+T src;
+@@
+(
+- memcpy(dst, &src, sizeof(*dst));
++ *dst = src;
+|
+- memcpy(dst, &src, sizeof(src));
++ *dst = src;
+|
+- memcpy(dst, &src, sizeof(T));
++ *dst = src;
+)
+
+@@
+type T;
+T *dst;
+T *src;
+@@
+(
+- memcpy(dst, src, sizeof(*dst));
++ *dst = *src;
+|
+- memcpy(dst, src, sizeof(*src));
++ *dst = *src;
+|
+- memcpy(dst, src, sizeof(T));
++ *dst = *src;
+)
diff --git a/diff.c b/diff.c
index 051761be4..fbd68ecd0 100644
--- a/diff.c
+++ b/diff.c
@@ -1169,7 +1169,7 @@ static void init_diff_words_data(struct emit_callback
*ecbdata,
{
int i;
struct diff_options *o = xmalloc(sizeof(struct diff_options));
- memcpy(o, orig_opts, sizeof(struct diff_options));
+ *o = *orig_opts;
ecbdata->diff_words =
xcalloc(1, sizeof(struct diff_words_data));
@@ -3359,7 +3359,7 @@ static void run_checkdiff(struct diff_filepair *p, struct
diff_options *o)
void diff_setup(struct diff_options *options)
{
- memcpy(options, &default_diff_options, sizeof(*options));
+ *options = default_diff_options;
options->file = stdout;
diff --git a/dir.c b/dir.c
index 4541f9e14..d3d0039bf 100644
--- a/dir.c
+++ b/dir.c
@@ -2499,7 +2499,7 @@ static int read_one_dir(struct untracked_cache_dir
**untracked_,
if (next > rd->end)
return -1;
*untracked_ = untracked = xmalloc(st_add(sizeof(*untracked), len));
- memcpy(untracked, &ud, sizeof(ud));
+ *untracked = ud;
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Fri, Mar 10, 2017 at 01:14:45AM +0100, René Scharfe wrote:
> > 3. There were a number of alloc-and-copy instances. The copy part is
> > the same as (2) above, but you have to repeat the size, which is
> > potentially error-prone. I wonder if we would want something like:
> >
> >#define ALLOC_COPY(dst, src) do { \
> > (dst) = xmalloc(sizeof(*(dst))); \
> > COPY_ARRAY(dst, src, 1); \
> >while(0)
> >
> > That avoids having to specify the size at all, and triggers a
> > compile-time error if "src" and "dst" point to objects of different
> > sizes.
>
> Or you could call it DUP or similar. And you could use ALLOC_ARRAY in
> its definition and let it infer the size implicitly (don't worry too
> much about the multiplication with one):
>
> #define DUPLICATE_ARRAY(dst, src, n) do { \
> ALLOC_ARRAY((dst), (n));\
> COPY_ARRAY((dst), (src), (n)); \
> } while (0)
> #define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)
>
> But do we even want such a thing? Duplicating objects should be rare, and
> keeping allocation and assignment/copying separate makes for more flexible
> building blocks. Adding ALLOC (and CALLOC) for single objects could be more
> widely useful, I think.
There's no reason you can't have both the building blocks and the thing
that is built for them. I think there are 5 spots that would use
DUPLICATE(), but I agree that there are more which could use ALLOC().
I'd be more worried that we're slowly drifting away from idiomatic C.
If it's safer, that's good. But if it makes it hard for people new to
the project to figure out what the hell is going on, that may be not so
good.
Here's the list of DUPLICATE spots, for reference.
---
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4b..f0ac1c511 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -661,8 +661,8 @@ static struct origin *find_rename(struct scoreboard *sb,
static void add_blame_entry(struct blame_entry ***queue,
const struct blame_entry *src)
{
- struct blame_entry *e = xmalloc(sizeof(*e));
- memcpy(e, src, sizeof(*e));
+ struct blame_entry *e;
+ DUPLICATE(e, src);
origin_incref(e->suspect);
e->next = **queue;
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index 72c815844..d6cb893cf 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -206,8 +206,8 @@ static void llist_sorted_difference_inplace(struct llist *A,
static inline struct pack_list * pack_list_insert(struct pack_list **pl,
struct pack_list *entry)
{
- struct pack_list *p = xmalloc(sizeof(struct pack_list));
- memcpy(p, entry, sizeof(struct pack_list));
+ struct pack_list *p;
+ DUPLICATE(p, entry);
p->next = *pl;
*pl = p;
return p;
@@ -238,8 +238,7 @@ static struct pack_list * pack_list_difference(const struct
pack_list *A,
return pack_list_difference(A->next, B);
pl = pl->next;
}
- ret = xmalloc(sizeof(struct pack_list));
- memcpy(ret, A, sizeof(struct pack_list));
+ DUPLICATE(ret, A);
ret->next = pack_list_difference(A->next, B);
return ret;
}
@@ -450,8 +449,7 @@ static void minimize(struct pack_list **min)
perm_all = perm = get_permutations(non_unique, n);
while (perm) {
if (is_superset(perm->pl, missing)) {
- new_perm = xmalloc(sizeof(struct pll));
- memcpy(new_perm, perm, sizeof(struct pll));
+ DUPLICATE(new_perm, perm);
new_perm->next = perm_ok;
perm_ok = new_perm;
}
diff --git a/diff.c b/diff.c
index 051761be4..dfe02f403 100644
--- a/diff.c
+++ b/diff.c
@@ -1168,8 +1168,8 @@ static void init_diff_words_data(struct emit_callback
*ecbdata,
struct diff_filespec *two)
{
int i;
- struct diff_options *o = xmalloc(sizeof(struct diff_options));
- memcpy(o, orig_opts, sizeof(struct diff_options));
+ struct diff_options *o;
+ DUPLICATE(o, orig_opts);
ecbdata->diff_words =
xcalloc(1, sizeof(struct diff_words_data));
Re: [PATCH v1] Travis: also test on 32-bit Linux
Am 05.03.2017 um 12:36 schrieb Jeff King:
> I grepped for 'memcpy.*sizeof' and found one other case that's not a
> bug, but is questionable.
>
> Of the "good" cases, I think most of them could be converted into
> something more obviously-correct, which would make auditing easier. The
> three main cases I saw were:
> 2. Ones which just copy a single object, like:
>
>memcpy(&dst, &src, sizeof(dst));
>
> Perhaps we should be using struct assignment like:
>
>dst = src;
>
> here. It's safer and it should give the compiler more room to
> optimize. The only downside is that if you have pointers, it is
> easy to write "dst = src" when you meant "*dst = *src".
Compilers can usually inline memcpy(3) calls, but assignments are
shorter and more pleasing to the eye, and we get a type check for
free. How about this?
-- >8 --
Subject: [PATCH] cocci: use assignment operator to copy structs
Add a semantic patch for converting memcpy(3) calls targeting
addresses of variables (i.e., variables preceded by &) -- which are
basically always structs -- to simple assignments, and apply it to
the current tree. The resulting code is shorter, simpler and its
type safety is checked by the compiler.
Signed-off-by: Rene Scharfe
---
builtin/log.c | 2 +-
contrib/coccinelle/copy.cocci | 31 +++
convert.c | 2 +-
credential-cache--daemon.c| 2 +-
daemon.c | 2 +-
line-log.c| 2 +-
revision.c| 2 +-
7 files changed, 37 insertions(+), 6 deletions(-)
create mode 100644 contrib/coccinelle/copy.cocci
diff --git a/builtin/log.c b/builtin/log.c
index 55d20cc2d8..23bb9a9e76 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1030,7 +1030,7 @@ static void make_cover_letter(struct rev_info *rev, int
use_stdout,
if (!origin)
return;
- memcpy(&opts, &rev->diffopt, sizeof(opts));
+ opts = rev->diffopt;
opts.output_format = DIFF_FORMAT_SUMMARY | DIFF_FORMAT_DIFFSTAT;
diff_setup_done(&opts);
diff --git a/contrib/coccinelle/copy.cocci b/contrib/coccinelle/copy.cocci
new file mode 100644
index 00..f0d883932a
--- /dev/null
+++ b/contrib/coccinelle/copy.cocci
@@ -0,0 +1,31 @@
+@@
+type T;
+T dst;
+T src;
+@@
+(
+- memcpy(&dst, &src, sizeof(dst));
++ dst = src;
+|
+- memcpy(&dst, &src, sizeof(src));
++ dst = src;
+|
+- memcpy(&dst, &src, sizeof(T));
++ dst = src;
+)
+
+@@
+type T;
+T dst;
+T *src;
+@@
+(
+- memcpy(&dst, src, sizeof(dst));
++ dst = *src;
+|
+- memcpy(&dst, src, sizeof(*src));
++ dst = *src;
+|
+- memcpy(&dst, src, sizeof(T));
++ dst = *src;
+)
diff --git a/convert.c b/convert.c
index 8d652bf27c..4bae12be6b 100644
--- a/convert.c
+++ b/convert.c
@@ -290,7 +290,7 @@ static int crlf_to_git(const char *path, const char *src,
size_t len,
if ((checksafe == SAFE_CRLF_WARN ||
(checksafe == SAFE_CRLF_FAIL)) && len) {
struct text_stat new_stats;
- memcpy(&new_stats, &stats, sizeof(new_stats));
+ new_stats = stats;
/* simulate "git add" */
if (convert_crlf_into_lf) {
new_stats.lonelf += new_stats.crlf;
diff --git a/credential-cache--daemon.c b/credential-cache--daemon.c
index 46c5937526..798cf33c3a 100644
--- a/credential-cache--daemon.c
+++ b/credential-cache--daemon.c
@@ -22,7 +22,7 @@ static void cache_credential(struct credential *c, int
timeout)
e = &entries[entries_nr++];
/* take ownership of pointers */
- memcpy(&e->item, c, sizeof(*c));
+ e->item = *c;
memset(c, 0, sizeof(*c));
e->expiration = time(NULL) + timeout;
}
diff --git a/daemon.c b/daemon.c
index 473e6b6b63..f891398aad 100644
--- a/daemon.c
+++ b/daemon.c
@@ -785,7 +785,7 @@ static void add_child(struct child_process *cld, struct
sockaddr *addr, socklen_
newborn = xcalloc(1, sizeof(*newborn));
live_children++;
- memcpy(&newborn->cld, cld, sizeof(*cld));
+ newborn->cld = *cld;
memcpy(&newborn->address, addr, addrlen);
for (cradle = &firstborn; *cradle; cradle = &(*cradle)->next)
if (!addrcmp(&(*cradle)->address, &newborn->address))
diff --git a/line-log.c b/line-log.c
index 65f3558b3b..64f141e200 100644
--- a/line-log.c
+++ b/line-log.c
@@ -1093,7 +1093,7 @@ static int process_all_files(struct line_log_data
**range_out,
rg = rg->next;
assert(rg);
rg->pair = diff_filepair_dup(queue->queue[i]);
- memcpy(&rg->diff, pairdiff, sizeof(struct diff_ranges));
+ rg->diff = *pairdiff;
}
free(pairdiff);
}
diff --git a/revision.c b/revision.c
index b37dbec378..289977c796 100644
--- a/revision.c
+++ b/revision.c
@@ -2738,7 +2738,7 @@ int prepare_revision_walk(struc
Re: [PATCH v1] Travis: also test on 32-bit Linux
Am 05.03.2017 um 12:36 schrieb Jeff King:
I grepped for 'memcpy.*sizeof' and found one other case that's not a
bug, but is questionable.
Of the "good" cases, I think most of them could be converted into
something more obviously-correct, which would make auditing easier. The
three main cases I saw were:
3. There were a number of alloc-and-copy instances. The copy part is
the same as (2) above, but you have to repeat the size, which is
potentially error-prone. I wonder if we would want something like:
#define ALLOC_COPY(dst, src) do { \
(dst) = xmalloc(sizeof(*(dst))); \
COPY_ARRAY(dst, src, 1); \
while(0)
That avoids having to specify the size at all, and triggers a
compile-time error if "src" and "dst" point to objects of different
sizes.
Or you could call it DUP or similar. And you could use ALLOC_ARRAY in
its definition and let it infer the size implicitly (don't worry too
much about the multiplication with one):
#define DUPLICATE_ARRAY(dst, src, n) do { \
ALLOC_ARRAY((dst), (n));\
COPY_ARRAY((dst), (src), (n)); \
} while (0)
#define DUPLICATE(dst, src) DUPLICATE_ARRAY((dst), (src), 1)
But do we even want such a thing? Duplicating objects should be rare,
and keeping allocation and assignment/copying separate makes for more
flexible building blocks. Adding ALLOC (and CALLOC) for single objects
could be more widely useful, I think.
René
Re: [PATCH v1] Travis: also test on 32-bit Linux
On 05/03/17 17:38, Lars Schneider wrote: >> On 02 Mar 2017, at 16:17, Ramsay Jones wrote: >> On 02/03/17 11:24, Johannes Schindelin wrote: >>> On Thu, 2 Mar 2017, Lars Schneider wrote: >>> >> [snip] One thing that still bugs me: In the Linux32 environment prove adds the CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 csys ... Has anyone an idea why that happens and how we can disable it? >>> >>> I have no idea. >> >> I have no idea either, but it is not unique to this 32bit Linux, but >> rather the version of prove. For example, I am seeing this on Linux >> Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do >> this. (They used different Ubuntu LTS releases). >> >> [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1] > > I think I found it. It was introduced in TAP::Harness v3.34: > https://github.com/Perl-Toolchain-Gang/Test-Harness/commit/66cbf6355928b4828db517a99f1099b7fed35e90 > > ... and it is enabled with the "--timer" switch. Yep, that looks like it. When I updated to Mint 18, this broke a perl script of mine, so I had a quick look to see what I could do to suppress it. The man page seemed to imply that you could replace the output formatter, but that didn't take me too far (search CPAN for TAP::Parser::Formatter: ;-) ). I suppose you could replace Tap::Formatter::Base, or some such, but I didn't need to go that far - I simply changed a couple of regex-es to ignore the excess output! :-P Do you really need to suppress that timing information or, like me, can you simply ignore it? ATB, Ramsay Jones
Re: [PATCH v1] Travis: also test on 32-bit Linux
> On 02 Mar 2017, at 16:17, Ramsay Jones wrote: > > > > On 02/03/17 11:24, Johannes Schindelin wrote: >> Hi Lars, >> >> On Thu, 2 Mar 2017, Lars Schneider wrote: >> > [snip] >>> One thing that still bugs me: In the Linux32 environment prove adds the >>> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 >>> csys ... Has anyone an idea why that happens and how we can disable it? >> >> I have no idea. > > I have no idea either, but it is not unique to this 32bit Linux, but > rather the version of prove. For example, I am seeing this on Linux > Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do > this. (They used different Ubuntu LTS releases). > > [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1] I think I found it. It was introduced in TAP::Harness v3.34: https://github.com/Perl-Toolchain-Gang/Test-Harness/commit/66cbf6355928b4828db517a99f1099b7fed35e90 ... and it is enabled with the "--timer" switch. - Lars
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Sun, Mar 05, 2017 at 06:36:19AM -0500, Jeff King wrote: > I grepped for 'memcpy.*sizeof' and found one other case that's not a > bug, but is questionable. And here's the fix for that case. It can be applied separately from the other patch if need be. -- >8 -- Subject: [PATCH] ewah: fix eword_t/uint64_t confusion The ewah subsystem typedefs eword_t to be uint64_t, but some code uses a bare uint64_t. This isn't a bug now, but it's a potential maintenance problem if the definition of eword_t ever changes. Let's use the correct type. Note that we can't use COPY_ARRAY() here because the source and destination point to objects of different sizes. For that reason we'll also skip the usual "sizeof(*dst)" and use the real type, which should make it more clear that there's something tricky going on. Signed-off-by: Jeff King --- ewah/ewah_io.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c index 61f6a4357..f73210973 100644 --- a/ewah/ewah_io.c +++ b/ewah/ewah_io.c @@ -142,8 +142,8 @@ int ewah_read_mmap(struct ewah_bitmap *self, const void *map, size_t len) * the endianness conversion in a separate pass to ensure * we're loading 8-byte aligned words. */ - memcpy(self->buffer, ptr, self->buffer_size * sizeof(uint64_t)); - ptr += self->buffer_size * sizeof(uint64_t); + memcpy(self->buffer, ptr, self->buffer_size * sizeof(eword_t)); + ptr += self->buffer_size * sizeof(eword_t); for (i = 0; i < self->buffer_size; ++i) self->buffer[i] = ntohll(self->buffer[i]); -- 2.12.0.426.g9d5d0eeae
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Sat, Mar 04, 2017 at 09:08:40PM +0100, Vegard Nossum wrote:
> > At a glance, looks like range_set_copy() is using
> > sizeof(struct range_set) == 12, but
> > range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using
> > sizeof(rs->range) == 8.
>
> Attached patch seems to fix it -- basically, range_set_copy() is trying
> to copy more than it should. It was uncovered with the test case from
> Allan's commit because it's creating enough ranges to overflow the
> initial allocation on 32-bit.
Ugh, yeah, that is definitely a bug.
> diff --git a/line-log.c b/line-log.c
> index 951029665..cb0dc1110 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -43,7 +43,7 @@ void range_set_release(struct range_set *rs)
> static void range_set_copy(struct range_set *dst, struct range_set *src)
> {
> range_set_init(dst, src->nr);
> - memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
> + memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));
I think "sizeof(*dst->ranges)" is probably an even better fix, as it
infers the type of "dst". But these days we have COPY_ARRAY() to make it
even harder to get this kind of thing wrong.
I grepped for 'memcpy.*sizeof' and found one other case that's not a
bug, but is questionable.
Of the "good" cases, I think most of them could be converted into
something more obviously-correct, which would make auditing easier. The
three main cases I saw were:
1. Ones which can probably be converted to COPY_ARRAY().
2. Ones which just copy a single object, like:
memcpy(&dst, &src, sizeof(dst));
Perhaps we should be using struct assignment like:
dst = src;
here. It's safer and it should give the compiler more room to
optimize. The only downside is that if you have pointers, it is
easy to write "dst = src" when you meant "*dst = *src".
3. There were a number of alloc-and-copy instances. The copy part is
the same as (2) above, but you have to repeat the size, which is
potentially error-prone. I wonder if we would want something like:
#define ALLOC_COPY(dst, src) do { \
(dst) = xmalloc(sizeof(*(dst))); \
COPY_ARRAY(dst, src, 1); \
while(0)
That avoids having to specify the size at all, and triggers a
compile-time error if "src" and "dst" point to objects of different
sizes.
I suspect our friendly neighborhood coccinelle wizards could cook
up a conversion.
-Peff
Re: [PATCH v1] Travis: also test on 32-bit Linux
On 04/03/2017 20:49, Vegard Nossum wrote:
On 04/03/2017 19:08, Vegard Nossum wrote:
On 04/03/2017 18:23, Lars Schneider wrote:
Did Travis find our first 32bit bug? I briefly looked into it
and the following new topic on pu seems to cause the issue:
http://public-inbox.org/git/20170302172902.16850-1-allan.x.xav...@oracle.com/
https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd
The "git log" call in the new 4211 test fails with:
*** Error in `/usr/src/git/git': malloc: top chunk is corrupt:
0x09ff4a78 ***
More output here:
https://travis-ci.org/larsxschneider/git/builds/207715343
[...]
At a glance, looks like range_set_copy() is using
sizeof(struct range_set) == 12, but
range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using
sizeof(rs->range) == 8.
Attached patch seems to fix it -- basically, range_set_copy() is trying
to copy more than it should. It was uncovered with the test case from
Allan's commit because it's creating enough ranges to overflow the
initial allocation on 32-bit.
Vegard
diff --git a/line-log.c b/line-log.c
index 951029665..cb0dc1110 100644
--- a/line-log.c
+++ b/line-log.c
@@ -43,7 +43,7 @@ void range_set_release(struct range_set *rs)
static void range_set_copy(struct range_set *dst, struct range_set *src)
{
range_set_init(dst, src->nr);
- memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
+ memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range));
dst->nr = src->nr;
}
static void range_set_move(struct range_set *dst, struct range_set *src)
Re: [PATCH v1] Travis: also test on 32-bit Linux
On 04/03/2017 19:08, Vegard Nossum wrote: On 04/03/2017 18:23, Lars Schneider wrote: Did Travis find our first 32bit bug? I briefly looked into it and the following new topic on pu seems to cause the issue: http://public-inbox.org/git/20170302172902.16850-1-allan.x.xav...@oracle.com/ https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd The "git log" call in the new 4211 test fails with: *** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 *** More output here: https://travis-ci.org/larsxschneider/git/builds/207715343 It looks like it's hitting the bug the patch is supposed to fix. Are you quite sure it's running the "git" binary that was just built (as opposed to e.g. the system binary installed inside the container)? Nevermind, I can reproduce it locally. ==10205== Invalid write of size 4 ==10205==at 0x4031ED2: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==10205==by 0x811C4B0: memcpy (string3.h:53) ==10205==by 0x811C4B0: range_set_copy.isra.7 (line-log.c:46) ==10205==by 0x811C51B: line_log_data_copy_one (line-log.c:628) ==10205==by 0x811C55D: line_log_data_copy (line-log.c:642) ==10205==by 0x811C5E3: add_line_range (line-log.c:708) ==10205==by 0x811D767: line_log_init (line-log.c:745) ==10205==by 0x808B1CD: cmd_log_init_finish (log.c:204) ==10205==by 0x808C9C8: cmd_log_init (log.c:213) ==10205==by 0x808C9C8: cmd_log (log.c:681) ==10205==by 0x804CF14: run_builtin (git.c:373) ==10205==by 0x804CF14: handle_builtin (git.c:574) ==10205==by 0x804D264: run_argv (git.c:626) ==10205==by 0x804D264: cmd_main (git.c:703) ==10205==by 0x804C448: main (common-main.c:43) ==10205== Address 0x4cde9c8 is 0 bytes after a block of size 1,600 alloc'd ==10205==at 0x402D17C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==10205==by 0x402F370: realloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==10205==by 0x819CC0F: xrealloc (wrapper.c:137) ==10205==by 0x811C1C8: range_set_grow (line-log.c:21) ==10205==by 0x811C499: range_set_init (line-log.c:32) ==10205==by 0x811C499: range_set_copy.isra.7 (line-log.c:45) ==10205==by 0x811C51B: line_log_data_copy_one (line-log.c:628) ==10205==by 0x811C55D: line_log_data_copy (line-log.c:642) ==10205==by 0x811C5E3: add_line_range (line-log.c:708) ==10205==by 0x811D767: line_log_init (line-log.c:745) ==10205==by 0x808B1CD: cmd_log_init_finish (log.c:204) ==10205==by 0x808C9C8: cmd_log_init (log.c:213) ==10205==by 0x808C9C8: cmd_log (log.c:681) ==10205==by 0x804CF14: run_builtin (git.c:373) ==10205==by 0x804CF14: handle_builtin (git.c:574) At a glance, looks like range_set_copy() is using sizeof(struct range_set) == 12, but range_set_init/range_set_grow/ALLOC_GROW/REALLOC_ARRAY is using sizeof(rs->range) == 8. Vegard
Re: [PATCH v1] Travis: also test on 32-bit Linux
On 04/03/2017 18:23, Lars Schneider wrote: Did Travis find our first 32bit bug? I briefly looked into it and the following new topic on pu seems to cause the issue: http://public-inbox.org/git/20170302172902.16850-1-allan.x.xav...@oracle.com/ https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd The "git log" call in the new 4211 test fails with: *** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 *** More output here: https://travis-ci.org/larsxschneider/git/builds/207715343 It looks like it's hitting the bug the patch is supposed to fix. Are you quite sure it's running the "git" binary that was just built (as opposed to e.g. the system binary installed inside the container)? Vegard
Re: [PATCH v1] Travis: also test on 32-bit Linux
> On 04 Mar 2017, at 05:11, Junio C Hamano wrote: > > On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano wrote: >> >> I only recently started looking at Travis logs, so I cannot tell if >> it is just a usual flake (e.g. some builds a few days ago seems to >> have failed due to not being able to check out the tree being >> tested, which I do not think is our fault) that we shouldn't worry >> about, or if it is a sign of a real problem. > > Tonight's pushout also seems to stall the same way. Dscho's > unversioned one didn't exhibit the problem? > https://travis-ci.org/git/git/jobs/206811396 Did Travis find our first 32bit bug? I briefly looked into it and the following new topic on pu seems to cause the issue: http://public-inbox.org/git/20170302172902.16850-1-allan.x.xav...@oracle.com/ https://github.com/git/git/commit/aaae0bf787f09ba102f69c3cf85d37e6554ab9fd The "git log" call in the new 4211 test fails with: *** Error in `/usr/src/git/git': malloc: top chunk is corrupt: 0x09ff4a78 *** More output here: https://travis-ci.org/larsxschneider/git/builds/207715343 >> Unrelated to linux-32, the same build has hard failure with Apple >> clang in t0021 with the rot13-filter.pl thing, by the way. > > This one may be a Heisenbug which may indicate some raciness in the > clean/smudge filter protocol. > The latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to > have passed OK. I agree, there seems to be some kind of race condition in "t0021.15 - required process filter should filter data". I try to look into it. - Lars
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Fri, Mar 3, 2017 at 4:03 PM, Junio C Hamano wrote: > > I only recently started looking at Travis logs, so I cannot tell if > it is just a usual flake (e.g. some builds a few days ago seems to > have failed due to not being able to check out the tree being > tested, which I do not think is our fault) that we shouldn't worry > about, or if it is a sign of a real problem. Tonight's pushout also seems to stall the same way. Dscho's unversioned one didn't exhibit the problem? https://travis-ci.org/git/git/jobs/206811396 > Unrelated to linux-32, the same build has hard failure with Apple > clang in t0021 with the rot13-filter.pl thing, by the way. This one may be a Heisenbug which may indicate some raciness in the clean/smudge filter protocol. The latest build of 'pu' https://travis-ci.org/git/git/jobs/207550171 seems to have passed OK.
Re: [PATCH v1] Travis: also test on 32-bit Linux
Junio C Hamano writes: > I see he did v2 which you Acked in a different thread. Will replace > what's been on 'pu' and running with Travis the past few days with > it. Let's wait for one or more Travis cycles and then merge it to > 'next'. https://travis-ci.org/git/git/jobs/207517043 is an output from 'pu' with the v2 patch; it seems to have fell into a funny state. The output ends like so: No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself. I only recently started looking at Travis logs, so I cannot tell if it is just a usual flake (e.g. some builds a few days ago seems to have failed due to not being able to check out the tree being tested, which I do not think is our fault) that we shouldn't worry about, or if it is a sign of a real problem. Unrelated to linux-32, the same build has hard failure with Apple clang in t0021 with the rot13-filter.pl thing, by the way.
Re: [PATCH v1] Travis: also test on 32-bit Linux
Johannes Schindelin writes: > On Thu, 2 Mar 2017, Junio C Hamano wrote: > >> Another question is which v3 people mean in the discussion, when you >> and Dscho work on improvements at the same time and each post the >> "next" version marked as "v3", and they comment on one of them? > > But then, Lars & I communicate in a more direct way than the mailing list > when discussing teeny tiny details such as: "could you have a look at my > mail" or "how would you change .travis.yml to do XYZ". > > With that level of communication, there is almost no danger of two v3s. Sure, that is true between two (or more) people who communicate privately. The issue you raised on Lars's "v1" and your original unversioned one can be seen either (1) as similar to having two v1's or (2) having (an implicit) v0 and v1 that won't cause confusion. As I said already, because the v0 and v1 (or v1 and v1) came from two different people, it's not such a big deal if Lars named his first one v1 or v2, just like it won't cause problem if his reroll were done as v2 or v3. I see he did v2 which you Acked in a different thread. Will replace what's been on 'pu' and running with Travis the past few days with it. Let's wait for one or more Travis cycles and then merge it to 'next'. Thanks.
Re: [PATCH v1] Travis: also test on 32-bit Linux
Hi Junio, On Thu, 2 Mar 2017, Junio C Hamano wrote: > Another question is which v3 people mean in the discussion, when you > and Dscho work on improvements at the same time and each post the > "next" version marked as "v3", and they comment on one of them? But then, Lars & I communicate in a more direct way than the mailing list when discussing teeny tiny details such as: "could you have a look at my mail" or "how would you change .travis.yml to do XYZ". With that level of communication, there is almost no danger of two v3s. Ciao, Johannes
Re: [PATCH v1] Travis: also test on 32-bit Linux
Lars Schneider writes: >> On 02 Mar 2017, at 12:24, Johannes Schindelin >> wrote: >> >> Hi Lars, >> >> On Thu, 2 Mar 2017, Lars Schneider wrote: >> >>> The patch looks good to me in general but I want to propose the following >>> changes: >> >> I know you are using your script to generate this mail, but I would have >> liked to see v2 in the subject ;-) > > Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in > my email client. Now I am wondering... is the next version v2 or v3 :D Another question is which v3 people mean in the discussion, when you and Dscho work on improvements at the same time and each post the "next" version marked as "v3", and they comment on one of them? Between "v2" and "v3", it would not make too much difference to the readership, when it is clear that two people are working to produce competing improvements without much coordination (i.e. lack of "ok, I'll send a reroll marked as vX in a minite"--"ok, I'll wait and comment on it" exchange). People watching from the sideline know "ah this is v3 from Lars which is the highest numbered one from him on this topic". As long as you do not mark your next one "v1", you'd be OK ;-).
Re: [PATCH v1] Travis: also test on 32-bit Linux
On Thu, Mar 2, 2017 at 3:22 PM, Johannes Schindelin
wrote:
>
>> >> +set -e
>> >
>> > Is this really necessary? I really like to avoid `set -e`, in
>> > particular when we do pretty much everything in && chains anyway.
>>
>> Agreed, not really necessary here as we just invoke one command. Out of
>> curiosity: Why do you try to avoid it? I set it by default in all my
>> scripts.
>
> I try to avoid it because it encourages a style that omits helpful error
> messages.
Yeah, we prefer to define and use a die() function like this:
die () {
printf >&2 '%s\n' "$*"
exit 1
}
do_something || die "meaningful error message"
Re: [PATCH v1] Travis: also test on 32-bit Linux
On 02/03/17 11:24, Johannes Schindelin wrote: > Hi Lars, > > On Thu, 2 Mar 2017, Lars Schneider wrote: > [snip] >> One thing that still bugs me: In the Linux32 environment prove adds the >> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 >> csys ... Has anyone an idea why that happens and how we can disable it? > > I have no idea. I have no idea either, but it is not unique to this 32bit Linux, but rather the version of prove. For example, I am seeing this on Linux Mint 18.1 (64bit _and_ 32bit), whereas Linux Mint 17.x did not do this. (They used different Ubuntu LTS releases). [Mint 18.1 'prove --version' says: TAP::Harness v3.35 and Perl v5.22.1] ATB, Ramsay Jones
Re: [PATCH v1] Travis: also test on 32-bit Linux
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
> > On 02 Mar 2017, at 12:24, Johannes Schindelin
> > wrote:
> >
> > On Thu, 2 Mar 2017, Lars Schneider wrote:
> >
> >> The patch looks good to me in general but I want to propose the
> >> following changes:
> >
> > I know you are using your script to generate this mail, but I would
> > have liked to see v2 in the subject ;-)
>
> Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in my
> email client. Now I am wondering... is the next version v2 or v3 :D
Since there was no v2, the next one should *definitely* be v2... ;-)
> >> (1) Move all the docker magic into a dedicated file
> >> "ci/run-linux-32-build.sh" This way people should be able to run this
> >> build on their local machines without TravisCI. However, I haven't
> >> tested this.
> >
> > I considered this, but there is serious overlap: the `docker pull`
> > call and the `docker run` call *have* to refer to the same image. It's
> > very easy for them to get out of sync if you have that information in
> > two files. Maybe make that an option of the script, defaulting to
> > daald/ubuntu32:xenial?
>
> Right. I missed that. How about something like that?
>
> before_install:
> - ci/run-linux32-build.sh --pull-container
> before_script:
> script: ci/run-linux32-build.sh
I'd prefer
before_install:
- docker pull daald/ubuntu32:xenial
before_script:
script: ci/run-linux32-build.sh daald/ubuntu32:xenial
> > BTW speaking of Docker: it would be nicer if there was a Docker image
> > that already had the build-essentials installed, to save on startup
> > time. But I did not find any that was reasonably up-to-date.
>
> True. But installing everything just takes a minute and we don't need to
> maintain anything...
And when there are network problems (like there were on Tuesday, right
when I developed the first v1 of this patch) then we have another set of
problems that make Travis fail. Even if the code in the PR or branch is
actually good. I'd like to avoid false positives, if possible.
> >> +set -e
> >
> > Is this really necessary? I really like to avoid `set -e`, in
> > particular when we do pretty much everything in && chains anyway.
>
> Agreed, not really necessary here as we just invoke one command. Out of
> curiosity: Why do you try to avoid it? I set it by default in all my
> scripts.
I try to avoid it because it encourages a style that omits helpful error
messages.
> >> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> >> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> >> +
> >> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> >> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> >> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> >> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> >> +
> >> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> >> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> >> +
> >> +sudo docker run \
> >> +--interactive --volume "${PWD}:/usr/src/git" \
> >> +daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
> >
> > Hmm. Since it is a script now, it would be more readable this way, I
> > think:
> >
> > sudo docker run --volume "${PWD}:/usr/src/git"
> > "${1:-daald/ubuntu32:xenial}" \
> > linux32 --32bit i386 sh -c '
> > : update packages first &&
> > apt update >/dev/null &&
> > apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> > libexpat-dev gettext python >/dev/null &&
> >
> > : now build and test &&
> > cd /usr/src/git &&
> > DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> > GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> > GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> > GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> > make -j2 test
> > '
>
> That looks better! I'll try it!
Thanks!
Dscho
Re: [PATCH v1] Travis: also test on 32-bit Linux
> On 02 Mar 2017, at 12:24, Johannes Schindelin
> wrote:
>
> Hi Lars,
>
> On Thu, 2 Mar 2017, Lars Schneider wrote:
>
>> The patch looks good to me in general but I want to propose the following
>> changes:
>
> I know you are using your script to generate this mail, but I would have
> liked to see v2 in the subject ;-)
Yeah, sorry. I already had a "D'oh" moment *after* I saw the email in
my email client. Now I am wondering... is the next version v2 or v3 :D
>> (1) Move all the docker magic into a dedicated file
>> "ci/run-linux-32-build.sh" This way people should be able to run this
>> build on their local machines without TravisCI. However, I haven't
>> tested this.
>
> I considered this, but there is serious overlap: the `docker pull` call
> and the `docker run` call *have* to refer to the same image. It's very
> easy for them to get out of sync if you have that information in two
> files. Maybe make that an option of the script, defaulting to
> daald/ubuntu32:xenial?
Right. I missed that. How about something like that?
before_install:
- ci/run-linux32-build.sh --pull-container
before_script:
script: ci/run-linux32-build.sh
> BTW speaking of Docker: it would be nicer if there was a Docker image that
> already had the build-essentials installed, to save on startup time. But I
> did not find any that was reasonably up-to-date.
True. But installing everything just takes a minute and we don't need to
maintain anything...
>> +set -e
>
> Is this really necessary? I really like to avoid `set -e`, in particular
> when we do pretty much everything in && chains anyway.
Agreed, not really necessary here as we just invoke one command.
Out of curiosity: Why do you try to avoid it? I set it by default in all
my scripts.
>> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
>> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
>> +
>> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
>> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
>> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
>> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
>> +
>> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
>> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
>> +
>> +sudo docker run \
>> +--interactive --volume "${PWD}:/usr/src/git" \
>> +daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
>
> Hmm. Since it is a script now, it would be more readable this way, I
> think:
>
> sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
> linux32 --32bit i386 sh -c '
> : update packages first &&
> apt update >/dev/null &&
> apt install -y build-essential libcurl4-openssl-dev libssl-dev \
> libexpat-dev gettext python >/dev/null &&
>
> : now build and test &&
> cd /usr/src/git &&
> DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
> GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
> GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
> GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
> make -j2 test
> '
That looks better! I'll try it!
- Lars
Re: [PATCH v1] Travis: also test on 32-bit Linux
Hi Lars,
On Thu, 2 Mar 2017, Lars Schneider wrote:
> The patch looks good to me in general but I want to propose the following
> changes:
I know you are using your script to generate this mail, but I would have
liked to see v2 in the subject ;-)
> (1) Move all the docker magic into a dedicated file
> "ci/run-linux-32-build.sh" This way people should be able to run this
> build on their local machines without TravisCI. However, I haven't
> tested this.
I considered this, but there is serious overlap: the `docker pull` call
and the `docker run` call *have* to refer to the same image. It's very
easy for them to get out of sync if you have that information in two
files. Maybe make that an option of the script, defaulting to
daald/ubuntu32:xenial?
BTW speaking of Docker: it would be nicer if there was a Docker image that
already had the build-essentials installed, to save on startup time. But I
did not find any that was reasonably up-to-date.
> (2) The docker build command inherits the Git test environment
> variables. This way we use the same environment variables as in all
> other TravisCI builds (plus it would use *your* variables if you run it
> locally).
Good!
> (3) Silence the apt update/git output as is it clutters the log. I did
> not silence stderr output!
Also good!
> (4) Remove (to my knowledge) superfluous "compiler: clang" in the
> Linux32 job.
I copied that from one of your experimental .travis.yml patches ;-)
> I added my sign-off. I hope this is the right thing to do in this "I
> took your patch and changed it to suggest an improvement" situation.
Absolutely. Thank you for taking it from here.
> One thing that still bugs me: In the Linux32 environment prove adds the
> CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00
> csys ... Has anyone an idea why that happens and how we can disable it?
I have no idea.
> diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
> new file mode 100755
> index 00..b892fbdc9e
> --- /dev/null
> +++ b/ci/run-linux32-build.sh
> @@ -0,0 +1,21 @@
> +#!/bin/sh
> +#
> +# Build and test Git in a docker container running a 32-bit Ubuntu Linux
> +#
> +
> +set -e
Is this really necessary? I really like to avoid `set -e`, in particular
when we do pretty much everything in && chains anyway.
> +APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
> +"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
> +
> +TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
> +"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
> +"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
> +"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
> +
> +TEST_GIT_CMD="linux32 --32bit i386 sh -c '"\
> +"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
> +
> +sudo docker run \
> +--interactive --volume "${PWD}:/usr/src/git" \
> +daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
Hmm. Since it is a script now, it would be more readable this way, I
think:
sudo docker run --volume "${PWD}:/usr/src/git" "${1:-daald/ubuntu32:xenial}" \
linux32 --32bit i386 sh -c '
: update packages first &&
apt update >/dev/null &&
apt install -y build-essential libcurl4-openssl-dev libssl-dev \
libexpat-dev gettext python >/dev/null &&
: now build and test &&
cd /usr/src/git &&
DEFAULT_TEST_TARGET='"$DEFAULT_TEST_TARGET"' \
GIT_PROVE_OPTS='"$GIT_PROVE_OPTS"' \
GIT_TEST_OPTS='"$GIT_TEST_OPTS"' \
GIT_TEST_CLONE_2GB='"$GIT_TEST_CLONE_2GB"' \
make -j2 test
'
This is completely untested (pun intended ;-))...
Ciao,
Dscho
[PATCH v1] Travis: also test on 32-bit Linux
From: Johannes Schindelin
When Git v2.9.1 was released, it had a bug that showed only on Windows
and on 32-bit systems: our assumption that `unsigned long` can hold
64-bit values turned out to be wrong.
This could have been caught earlier if we had a Continuous Testing
set up that includes a build and test run on 32-bit Linux.
Let's do this (and take care of the Windows build later). This patch
asks Travis CI to install a Docker image with 32-bit libraries and then
goes on to build and test Git using this 32-bit setup.
Signed-off-by: Johannes Schindelin
Signed-off-by: Lars Schneider
---
Thanks for the patch Dscho!
The patch looks good to me in general but I want to propose the following
changes:
(1) Move all the docker magic into a dedicated file "ci/run-linux-32-build.sh"
This way people should be able to run this build on their local machines
without TravisCI. However, I haven't tested this.
(2) The docker build command inherits the Git test environment variables.
This way we use the same environment variables as in all other TravisCI
builds (plus it would use *your* variables if you run it locally).
(3) Silence the apt update/git output as is it clutters the log.
I did not silence stderr output!
(4) Remove (to my knowledge) superfluous "compiler: clang" in the Linux32 job.
I added my sign-off. I hope this is the right thing to do in this "I took your
patch and changed it to suggest an improvement" situation.
You can see a successful run here:
https://travis-ci.org/larsxschneider/git/jobs/206945950
One thing that still bugs me: In the Linux32 environment prove adds the
CPU times to every test run: ( 0.02 usr 0.00 sys + 0.00 cusr 0.00 csys ...
Has anyone an idea why that happens and how we can disable it?
Cheers,
Lars
Notes:
Base Ref:
Web-Diff: https://github.com/larsxschneider/git/commit/82995ed59c
Checkout: git fetch https://github.com/larsxschneider/git
travisci/linux32-v1 && git checkout 82995ed59c
.travis.yml | 9 +
ci/run-linux32-build.sh | 21 +
2 files changed, 30 insertions(+)
create mode 100755 ci/run-linux32-build.sh
diff --git a/.travis.yml b/.travis.yml
index 9c63c8c3f6..c8c789c437 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -39,6 +39,15 @@ env:
matrix:
include:
+- env: Linux32
+ os: linux
+ sudo: required
+ services:
+- docker
+ before_install:
+- docker pull daald/ubuntu32:xenial
+ before_script:
+ script: ci/run-linux32-build.sh
- env: Documentation
os: linux
compiler: clang
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
new file mode 100755
index 00..b892fbdc9e
--- /dev/null
+++ b/ci/run-linux32-build.sh
@@ -0,0 +1,21 @@
+#!/bin/sh
+#
+# Build and test Git in a docker container running a 32-bit Ubuntu Linux
+#
+
+set -e
+
+APT_INSTALL="apt update >/dev/null && apt install -y build-essential "\
+"libcurl4-openssl-dev libssl-dev libexpat-dev gettext python >/dev/null"
+
+TEST_GIT_ENV="DEFAULT_TEST_TARGET=$DEFAULT_TEST_TARGET "\
+"GIT_PROVE_OPTS=\"$GIT_PROVE_OPTS\" "\
+"GIT_TEST_OPTS=\"$GIT_TEST_OPTS\" "\
+"GIT_TEST_CLONE_2GB=$GIT_TEST_CLONE_2GB"
+
+TEST_GIT_CMD="linux32 --32bit i386 sh -c "\
+"'$APT_INSTALL && cd /usr/src/git && $TEST_GIT_ENV make -j2 test'"
+
+sudo docker run \
+--interactive --volume "${PWD}:/usr/src/git" \
+daald/ubuntu32:xenial /bin/bash -c "$TEST_GIT_CMD"
base-commit: 3bc53220cb2dcf709f7a027a3f526befd021d858
--
2.11.1
