Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use C99 flexible arrays for struct pool_block in memory.c if possible #1764

Merged
merged 2 commits into from May 8, 2018

Conversation

Projects
None yet
3 participants
@xavierleroy
Copy link
Contributor

xavierleroy commented May 6, 2018

The original code causes alarms with Clang's address sanitizer, because the malloc-ed size of the object is less than its static size (as given by sizeof). Whether it is valid C99 code is up for debate.

The new code is patterned after struct caml_ba_array in <caml/bigarray.h>, which is a solution to a similar problem, see MPR#5516.

I'd like to integrate Clang sanitizers and maybe other static or dynamic checking tools in our CI. Getting rid of spurious alarms is a first step.

Use C99 flexible arrays for struct pool_block in memory.c if possible
The original code causes alarms with Clang's address sanitizer.

The new code is patterned after struct caml_ba_array in <caml/bigarray.h>,
which is a solution to a similar problem (see MPR#5516).
@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 6, 2018

Whether it is valid C99 code is up for debate.

The flexible array member at the end of the struct is valid C99 code.

However, whether this is the best way to handle an alignment issue is up for debate. I'm not quite sure I understand the problem fixed as malloc is, to my knowledge, required to return an aligned block. Quoting POSIX 2017's malloc description for example:

The pointer returned if the allocation succeeds shall be suitably aligned so that it may be assigned to a pointer to any type of object

The C standard has similar language but is unfortunately harder to find as a web page.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 6, 2018

Hrm. Reading further I realize I don't know that malloc is involved here.

Sadly, because of my academic work, I've become too informed about the C standard for my own good. (Then again, I would imagine that @xavierleroy is in the same position having written CompCert.) Anyway, if someone can explain how the allocation is being done, I can explain how needed alignment can be achieved per the standard. Regardless, the use of the flexible array member is completely permitted. Whether it achieves the desired effect in a standards compliant way is a different question.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 7, 2018

The question is not whether the new code with the flex array is legal C99 -- I know it is. The question was whether the old code it replaces is legal C99 or is undefined behavior anyway.

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 7, 2018

The old code is undefined behavior under not just one but several rules.

That said...

  1. What does having this member of the structure accomplish? It seems to be trying to assure that the structure is aligned in some manner? But I'm not sure that's the right way to accomplish that.
  2. I will note that, under a variety of old compilers, a terminal array member in a structure of zero length was allowed, as an extension. gcc permitted this first, and then many other compilers did. It is certainly the case that under gcc and anything that pretends to have gcc's extensions you may declare this as union max_align data[0];
  3. Your new code is of course the maximally portable way to do this, though I'm still not clear on what "this" is for.
@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 8, 2018

@murmour: this is originally your code, so could you please review? Thanks.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 8, 2018

@pmetzger: it's pretty clear what the code is doing. Consider: you want to wrap malloc() so as to put the blocks in a doubly-linked list. How do you do that in a way that preserves (as much as possible) the alignment properties of malloc() ?

@xavierleroy xavierleroy added this to the 4.07 milestone May 8, 2018

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 8, 2018

@pmetzger

What does having this member of the structure accomplish? It seems to be trying to assure that the structure is aligned in some manner? But I'm not sure that's the right way to accomplish that.

Anyway, if someone can explain how the allocation is being done, I can explain how needed alignment can be achieved per the standard.

The struct is a header of larger (arbitrary-sized) block allocated with malloc. I've explained the reason for alignment requirements in #71:

That trick with max_align is not needed at all for correctness (void* would do just fine as far as correctness goes); however, it is supposed to increase the chances of getting a more favourable alignment of data in terms of performance (it is important, since data is going to be accessed much more often than the header of the block).

For a bigger picture, see this comment near the code that this patch touches:

/* Global memory pool.

   The pool is structured as a ring of blocks, where each block's header
   contains two links: to the previous and to the next block. The data
   structure allows for insertions and removals of blocks in constant time,
   given that a pointer to the operated block is provided.

   Initially, the pool contains a single block -- a pivot with no data, the
   guaranteed existence of which makes for a more concise implementation.

   The API functions that operate on the pool receive not pointers to the
   block's header, but rather pointers to the block's "data" field. This
   behaviour is required to maintain compatibility with the interfaces of
   [malloc], [realloc], and [free] family of functions, as well as to hide
   the implementation from the user.
*/

Also, see docs for caml_stat_* functions in memory.h.

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 8, 2018

@xavierleroy

The question is not whether the new code with the flex array is legal C99 -- I know it is. The question was whether the old code it replaces is legal C99 or is undefined behavior anyway.

I've checked some references, but it's still unclear to me whether the behaviour is defined.

The standard claims (C99, 6.5.6p8): "If both the pointer operand and the result point to elements of the same array object, or one past the last element of the array object, the evaluation shall not produce an overflow; otherwise, the behavior is undefined."

I understand it as "accessing data[2] is UB".

Now, what our code does (a shortened version):

#define SIZEOF_POOL_BLOCK offsetof(struct pool_block, data)

void* stat_alloc(asize_t sz)
{
  struct pool_block *pb = malloc(sz + SIZEOF_POOL_BLOCK);
  if (pb == NULL) return NULL;

  /* Linking the block into the ring */
  pb->next = pool->next;
  pb->prev = pool;
  pool->next->prev = pb;
  pool->next = pb;

  return &(pb->data);
}

In this piece of code, there doesn't seem to be any UB, as referencing a field ( &(pb->data)) is a perfectly defined operation. On the other hand, the user of stat_alloc does perform out-of-bounds access indirectly, by reading and writing memory that corresponds to data[0]..data[sz-1].

In regards to UB, is this the same as accessing data[2] directly, without an intermediate void*-cast? I'm quite puzzled (and also quite glad that I don't have to write C compilers).

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 8, 2018

Now, even if it is UB, do we have to fix code for older compilers?

Probably not. The idiom is pretty common, and lots of software would break if compilers bugged on it. AFAIK, there is no better solution in C-pre-99 (and that's why the feature was added).

union max_align data[1]; /* not allocated, used for alignment purposes */
/* Use C99's flexible array types if possible */
#if (__STDC_VERSION__ >= 199901L)
union max_align data[]; /* not allocated, used for alignment purposes */

This comment has been minimized.

@murmour

murmour May 8, 2018

Contributor

The "no allocated" comment is not specific to the C99 path.

@murmour

This comment has been minimized.

Copy link
Contributor

murmour commented May 8, 2018

Regarding the PR itself: looks good, I see no issues.

@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 8, 2018

Now, even if it is UB, do we have to fix code for older compilers? [...] AFAIK, there is no better solution in C-pre-99 (and that's why the feature was added).

Agreed. The only pre-C99 compiler we care about is Microsoft's (MSVC), and it doesn't optimize aggressively based on the absence of u.b.

@xavierleroy xavierleroy merged commit 4dae83a into ocaml:trunk May 8, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

xavierleroy added a commit that referenced this pull request May 8, 2018

Use C99 flexible arrays for struct pool_block in memory.c if possible (
…#1764)

The original code may not be quite ISO C conformant,
and causes alarms with Clang's address sanitizer.

The new code is patterned after struct caml_ba_array in <caml/bigarray.h>,
which is a solution to a similar problem (see MPR#5516).
@xavierleroy

This comment has been minimized.

Copy link
Contributor Author

xavierleroy commented May 8, 2018

Merged in trunk (4dae83a) and in 4.07 (17c4224)

@xavierleroy xavierleroy deleted the xavierleroy:pool_block_flex_array branch May 8, 2018

@pmetzger

This comment has been minimized.

Copy link
Member

pmetzger commented May 8, 2018

The struct is a header of larger (arbitrary-sized) block allocated with malloc. I've explained the reason for alignment requirements in #71:

Ah, so I think I understand. The goal is to make sure that the padding at the end of the header is appropriate for data that follows it.

I've checked some references, but it's still unclear to me whether the behaviour is defined.

The issue is you're putting a struct in a malloc'ed block of memory that is smaller than the struct. Things can then happen like passing the struct in a function call — if you made such a call, it would then accesses stuff past the end of the struct. It's thus undefined behavior. The reason for the modern notation is to avoid that. However, as has been noted, any compiler that really cares does C99.

Anyway, it's merged now. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.