UB for passing uninitialized variables to "significant only at root" arguments

[jprotze]

Problem

The wording about "significant only at root" is a bit sloppy and at least one example is broken. Although the parameters will not be read by the function on non-root ranks, passing uninitialized variables to the function is UB according to the C standard. Example 6.3 does not initialize rbuf on non-root ranks and passes this variable to MPI_Gather, which is UB. As a result, we observed the if (myrank == root) to be optimized away by clang 14 or newer and icx 2022.1 or newer.

Proposal

• Fix example 6.3 and initialize the variable on declaration.
• Expand the description for significant only at root

Changes to the Text

Change

int root, myrank, *rbuf;

to

int root, myrank, *rbuf=NULL;

And possibly explain why this initialization is important?

In the introduction of Chapter 6:

Some arguments in the collective functions are specified as “significant only at root,” and are ignored for all participants except the root.

Add:

The C standard nevertheless requires variables passed to these arguments to be initialized.

Impact on Implementations

None

Impact on Users

Avoid running into non-obvious UB by misunderstanding “significant only at root”

References and Pull Requests

[devreal]

Curious whether the compiler removed the branch because rbuf is uninitialized of because root is uninitialized in this example? Agreed that both are UB and we should be careful not to promote that.

[jprotze]

In our concrete code, we had root fixed to 0 and more code executed next to the malloc. A printf would even print rank as 0 for all ranks:

#include <mpi.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int main(int argc, char **argv) {
  if (argc != 2) {
    printf("No valid size given\n");
    return 1;
  }

  int n = atoi(argv[1]);

  MPI_Init(&argc, &argv);
  int rank, wsize;
  MPI_Comm_rank(MPI_COMM_WORLD, &rank);
  MPI_Comm_size(MPI_COMM_WORLD, &wsize);
  n = n - n % wsize;
  int n_per_process = n / wsize;

  float *x;
  float *x_root;

  if (rank == 0) {
    printf("From Rank %d: Initializing vectors...\n", rank);
    fflush(stdout);
    x_root = (float *)malloc(n * sizeof(float));
    memset(x_root, 0, n * sizeof(float));
  }

  x = (float *)malloc(n_per_process * sizeof(float));
  memset(x, 0, n_per_process * sizeof(float));
  MPI_Scatter(x_root, n_per_process, MPI_FLOAT, x, n_per_process, MPI_FLOAT, 0,
              MPI_COMM_WORLD);

  MPI_Finalize();
  free(x);
  return 0;
}

[jeffhammond]

I don't believe any of the examples are intended to be complete working code. Do they even include all the necessary headers to compile?

[wgropp]

That is correct, most examples are code snippets. The code check uses LaTeX comments to describe things like headers and declarations needed to see if the code could be compiled - this test has caught many errors, such as misspellings, out of order arguments, and missing semicolons, in the code snippets.

However, we do want the code snippets to be valid and compileable with the additional info, so things like ensure rbuf is initialized is reasonable for the examples.

[mahermanns]

Discussion in Tools WG on Sep 30, 2024:

• We should add the suggested text
  • Potentially with "on all participating processes"
• We should also change the "are ignored" to "are ignored by MPI" (or something alike)
  • The MPI implementation is the only thing that we can mandate for
  • What about tools? Do we need additional text to make sure tools don't do "the wrong thing" :tm:
  • A potential memory checker might want to evaluate the "non-significant" argument anyway

[mahermanns]

Discussion on Tools WG on Oct 14, 2024:

• Chapter (see "MPI_Scatter") the text actually uses "ignored", argument descriptions (in LIS) are using "significant only at root".
  • We should change the "ignored" text to "significant only at" also in the text
  • Define "significant" some place in the text
    • "MPI does not change behavior, based on the value of that argument" ?
      • We should focus on the "behavior" part of the MPI library and not trying to define terms like "ignore" in the context MPI
      • Could be in the realm of both AtoU and AtoI
        • "Users, always initialize your variables!"
    • Unsure where this should be done? Terms? Collectives?
  • Should bring this up to the Forum (non-voting meeting) to ask how to move forward here
  • Affected chapters:
    • Collectives
    • RMA (MPI_Accumulate)
    • Tools (MPI_T)
    • Process Initialization (MPI_Comm_spawn)

[jprotze]

Here is a semantically equivalent reproducer without MPI to look at the compiler optimization observed above on godbolt. For clang the optimization seems to be introduced with version 14:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int test(int n, char *c) {
  if (n == 0)
    return 0;
  return atoi(c);
}

int main(int argc, char **argv) {
  char *buf;
  if (argc > 1) {
    buf = strdup(argv[1]);
  }
  int res = test(argc - 1, buf);
  return res;
}