On Sat, Jun 13, 2020 at 08:53:39AM +0200, SZEDER Gábor wrote:
> On Sat, Jun 13, 2020 at 12:10:12AM +0530, Abhishek Kumar wrote:
> > diff --git a/commit-graph.c b/commit-graph.c
> > index 2ff042fbf4..91120ba3d3 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -87,6 +87,55 @@ static int commit_pos_cmp(const void *va, const void *vb)
> > commit_pos_at(&commit_pos, b);
> > }
> >
> > +define_commit_slab(commit_graph_data_slab, struct commit_graph_data);
> > +static struct commit_graph_data_slab commit_graph_data_slab =
> > + COMMIT_SLAB_INIT(1, commit_graph_data_slab);
> > +
> > +uint32_t commit_graph_position(const struct commit *c)
> > +{
> > + struct commit_graph_data *data =
> > + commit_graph_data_slab_peek(&commit_graph_data_slab, c);
> > +
> > + return data ? data->graph_pos : COMMIT_NOT_FROM_GRAPH;
> > +}
> > +
> > +uint32_t commit_graph_generation(const struct commit *c)
> > +{
> > + struct commit_graph_data *data =
> > + commit_graph_data_slab_peek(&commit_graph_data_slab, c);
> > +
> > + if (!data)
> > + return GENERATION_NUMBER_INFINITY;
> > + else if (data->graph_pos == COMMIT_NOT_FROM_GRAPH)
> > + return GENERATION_NUMBER_INFINITY;
> > +
> > + return data->generation;
> > +}
> > +
> > +static struct commit_graph_data *commit_graph_data_at(const struct commit *c)
>
> commit-graph.c: At top level:
> commit-graph.c:115:34: error: ‘commit_graph_data_at’ defined but not used [-Werror=unused-function]
> static struct commit_graph_data *commit_graph_data_at(const struct commit *c)
> ^
>
> Please make sure that each and every commit of your series can be
> built with DEVELOPER=1 and passes the test suite.
>
> > +{
> > + uint32_t i = commit_graph_data_slab.slab_count, j;
> > + uint32_t slab_size = commit_graph_data_slab.slab_size;
> > + struct commit_graph_data *data =
> > + commit_graph_data_slab_at(&commit_graph_data_slab, c);
> > +
> > + /*
> > + * commit-slab initializes elements with zero, overwrite this with
> > + * COMMIT_NOT_FROM_GRAPH for graph_pos.
> > + *
> > + * We avoid initializing generation with checking if graph position
> > + * is not COMMIT_NOT_FROM_GRAPH.
> > + */
> > + for (; i < commit_graph_data_slab.slab_count; i++) {
> > + for (j = 0; j < slab_size; j++) {
> > + commit_graph_data_slab[i][j].graph_pos =
>
> commit-graph.c: In function ‘commit_graph_data_at’:
> commit-graph.c:131:26: error: subscripted value is neither array nor pointer nor vector
> commit_graph_data_slab[i][j].graph_pos =
> ^
>
> Once the next patch is applied Git can be built again, but then:
>
> $ ~/src/git/git -C webkit.git commit-graph write
> Finding commits for commit graph among packed objects: 100% (3984415/3984415), done.
> Expanding reachable commits in commit graph: 219420, done.
> Segmentation fault
>
> Looking at it with a debugger:
>
> Program received signal SIGSEGV, Segmentation fault.
> 0x00000000005079b2 in commit_graph_data_at (c=0x1ad9aa0) at commit-graph.c:131
> 131 commit_graph_data_slab.slab[i][j].graph_pos =
> (gdb) bt
> #0 0x00000000005079b2 in commit_graph_data_at (c=0x1ad9aa0)
> at commit-graph.c:131
> #1 0x000000000050a6a7 in compute_generation_numbers (ctx=0x9d61a0)
> at commit-graph.c:1304
> #2 0x000000000050d37d in write_commit_graph (odb=0x9d3d80, pack_indexes=0x0,
> commits=0x0, flags=COMMIT_GRAPH_WRITE_PROGRESS,
> split_opts=0x98e730 <split_opts>) at commit-graph.c:2178
> #3 0x000000000042bb7e in graph_write (argc=0, argv=0x7fffffffe2f0)
> at builtin/commit-graph.c:242
> #4 0x000000000042bc8d in cmd_commit_graph (argc=1, argv=0x7fffffffe2f0,
> prefix=0x0) at builtin/commit-graph.c:278
> #5 0x00000000004061d1 in run_builtin (p=0x97b950 <commands+528>, argc=2,
> argv=0x7fffffffe2f0) at git.c:448
> #6 0x0000000000406521 in handle_builtin (argc=2, argv=0x7fffffffe2f0)
> at git.c:673
> #7 0x00000000004067c9 in run_argv (argcp=0x7fffffffe18c, argv=0x7fffffffe180)
> at git.c:740
> #8 0x0000000000406c3a in cmd_main (argc=2, argv=0x7fffffffe2f0) at git.c:871
> #9 0x00000000004cfbda in main (argc=5, argv=0x7fffffffe2d8)
> at common-main.c:52
> (gdb) p commit_graph_data_slab
> $1 = {
> slab_size = 65532,
> stride = 1,
> slab_count = 4,
> slab = 0x4f68690
> }
> (gdb) p i
> $2 = 0
> (gdb) p commit_graph_data_slab.slab[i]
> $3 = (struct commit_graph_data *) 0x0
> (gdb) p c->index
> $4 = 213506
> (gdb) up
> #1 0x000000000050a6a7 in compute_generation_numbers (ctx=0x9d61a0)
> at commit-graph.c:1304
> 1304 uint32_t generation = commit_graph_data_at(ctx->commits.list[i])->generation;
> (gdb) p i
> $5 = 0
>
>
> The way the loop variable 'i' is initialized and is used in the loop
> header suggests that you assume that all slabs with an index
> <slab_count are allocated. That's not the case, only those slabs are
> allocated that contain data associated with a commit that has already
> been looked at.
>
Wait, so assuming there are no slabs before and I call
`commit_graph_data_slab_at(&commit_graph_data_slab, c)` with an commit
index that would be in fourth slab, the slab would look like:
commit_graph_data_slab.slab[] = {NULL, NULL, NULL, <address>}
That's new to me! Thanks for sharing this.
Hopefully this is fixed in the v4 series here [1].
[1]: https://lore.kernel.org/git/20200617091411.14650-1-abhishekkumar8222@xxxxxxxxx/T/#u
>
> > + COMMIT_NOT_FROM_GRAPH;
> > + }
> > + }
> > +
> > + return data;
> > +}
> > +
> > static int commit_gen_cmp(const void *va, const void *vb)
> > {
> > const struct commit *a = *(const struct commit **)va;
> > diff --git a/commit-graph.h b/commit-graph.h
> > index 3ba0da1e5f..cc76757007 100644
> > --- a/commit-graph.h
> > +++ b/commit-graph.h
> > @@ -135,4 +135,14 @@ void free_commit_graph(struct commit_graph *);
> > */
> > void disable_commit_graph(struct repository *r);
> >
> > +struct commit_graph_data {
> > + uint32_t graph_pos;
> > + uint32_t generation;
> > +};
> > +
> > +/*
>
> This line adds a trailing space which is then removed in next patch.
> Please don't add it in the first place.
>
> > + * Commits should be parsed before accessing generation, graph positions.
> > + */
> > +uint32_t commit_graph_generation(const struct commit *);
> > +uint32_t commit_graph_position(const struct commit *);
> > #endif
> > --
> > 2.27.0
> >
Regards
Abhishek
