Skip to content

Commit

Permalink
Remove uses of FILENAME_MAX in the runtime (#26381)
Browse files Browse the repository at this point in the history
Replace uses of `FILENAME_MAX` for buffer sizes in the runtime,
replacing them with heap-allocated strings of the proper size that are
reallocated as needed.

Merges in and builds on the work by @rchinmay in
#17059.

Completes the second part of
#8757 (with
#26357).

[reviewed by @jabraham17 , thanks!]

Testing:
- [x] paratest
- [x] gasnet paratest
- [x] C backend paratest
- [x] GPU tests
- [x] manual run of a nightly config that covers (some) affected code
(`test-hpe-cray-ex-ofi.bash`)
  • Loading branch information
riftEmber authored Jan 7, 2025
2 parents 54cedd7 + dc7c518 commit 0d8be34
Show file tree
Hide file tree
Showing 7 changed files with 205 additions and 99 deletions.
1 change: 1 addition & 0 deletions runtime/include/chpl-mem-desc.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ extern "C" {
m(SET_WIDE_STRING, "set wide string", true ), \
m(GET_WIDE_STRING, "get wide string", true ), \
m(COMMAND_BUFFER, "command buffer", true ), \
m(FILENAME, "filename string", true ), \
m(COMM_UTIL, "comm layer utility space", false), \
m(COMM_XMIT_RCV_BUF, "comm layer transmit/receive buffer", false), \
m(COMM_FRK_SND_INFO, "comm layer sent remote fork info", false), \
Expand Down
3 changes: 3 additions & 0 deletions runtime/include/chpllaunch.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ typedef struct {
int chpl_doDryRun(void);
void chpl_append_to_largv(int* largc, const char*** largv, int* largv_len,
const char* arg);
void chpl_append_to_cmd(char** cmdBufPtr, int* charsWritten,
const char* format, ...)
__attribute__((format(printf, 3, 4)));
int chpl_run_utility1K(const char *command, char *const argv[],
char *outbuf, int outbuflen);
int chpl_run_cmdstr(const char *commandStr, char *outbuf, int outbuflen);
Expand Down
40 changes: 40 additions & 0 deletions runtime/src/chpl-launcher-common.c
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <sys/select.h>
#include <errno.h>
#include <string.h>
#include <assert.h>
#include "chplcgfns.h"
#include "chpl-comm-launch.h"
#include "chpl-comm-locales.h"
Expand Down Expand Up @@ -102,6 +103,45 @@ void chpl_append_to_largv(int* largc, const char*** largv, int* largv_len,
(*largv)[(*largc)++] = (arg);
}

// Helper for appending arguments to a variable-size command buffer.
// - Requires cmdBufPtr points to an uninitialized pointer on first call, and
// charsWritten is 0.
// - After exceeding an initial allocated size estimate, each call will allocate
// additional memory as needed.
void chpl_append_to_cmd(char** cmdBufPtr, int* charsWritten,
const char* format, ...) {
// Estimate of a buffer size that probably won't require extending, to avoid
// reallocations and copying.
static const int initialSize = 2048;

va_list argsForLen, argsForPrint;
va_start(argsForLen, format);
va_copy(argsForPrint, argsForLen);

// Allocate buffer to initial size on first call
if (*cmdBufPtr == NULL) {
assert(*charsWritten == 0);
*cmdBufPtr = (char*)chpl_mem_allocMany(initialSize, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
}

// Determine additional characters to be written
const int addedLen = vsnprintf(NULL, 0, format, argsForLen);
va_end(argsForLen);
int newLen = *charsWritten + addedLen;

// Allocate more memory if needed
if (newLen >= initialSize) {
*cmdBufPtr = (char*)chpl_mem_realloc(*cmdBufPtr, newLen * sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
}

// Write the new characters
vsnprintf(*cmdBufPtr + *charsWritten, addedLen + 1, format, argsForPrint);
va_end(argsForPrint);
*charsWritten = newLen;
}

//
// Use this function to run short utility programs that will return less
// than 1024 characters of output. The program must not expect any input.
Expand Down
21 changes: 15 additions & 6 deletions runtime/src/launch/pbs-aprun/launch-pbs-aprun.c
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ static char* walltime = NULL;
static char* queue = NULL;
static int generate_qsub_script = 0;

static char expectFilename[FILENAME_MAX];
static char* expectFilename = NULL;

extern int fileno(FILE *stream);

Expand Down Expand Up @@ -236,8 +236,12 @@ static char** chpl_launch_create_argv(int argc, char* argv[],
} else {
mypid = 0;
}
snprintf(expectFilename, FILENAME_MAX, "%s%d",
baseExpectFilename, (int)mypid);
int expectFilenameLen =
strlen(baseExpectFilename) + snprintf(NULL, 0, "%d", (int)mypid) + 1;
expectFilename = (char*)chpl_mem_allocMany(expectFilenameLen, sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
snprintf(expectFilename, expectFilenameLen, "%s%d", baseExpectFilename,
(int)mypid);

initAprunAttributes();
numCoresPerLocale = getCoresPerLocale();
Expand Down Expand Up @@ -386,10 +390,15 @@ static void genQsubScript(int argc, char *argv[], int numLocales) {
static void chpl_launch_cleanup(void) {
if (!chpl_doDryRun() && !debug) {
if (unlink(expectFilename)) {
char msg[FILENAME_MAX + 35];
snprintf(msg, FILENAME_MAX + 35, "Error removing temporary file '%s': %s",
char* format = "Error removing temporary file '%s': %s";
int msgLen =
strlen(format) + strlen(expectFilename) + strlen(strerror(errno));
char* msg = (char*)chpl_mem_allocMany(msgLen, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
snprintf(msg, msgLen, "Error removing temporary file '%s': %s",
expectFilename, strerror(errno));
chpl_warning(msg, 0, 0);
chpl_mem_free(msg, 0, 0);
}
}
}
Expand All @@ -414,7 +423,7 @@ int chpl_launch(int argc, char* argv[], int32_t numLocales,
argv[0]);
chpl_launch_cleanup();
}

chpl_mem_free(expectFilename, 0, 0);
return retcode;
}

Expand Down
19 changes: 14 additions & 5 deletions runtime/src/launch/pbs-gasnetrun_ibv/launch-pbs-gasnetrun_ibv.c
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ static char* walltime = NULL;
#define basePBSFilename ".chpl-pbs-qsub-"
#define baseExpectFilename ".chpl-expect-"

char pbsFilename[FILENAME_MAX];
char expectFilename[FILENAME_MAX];
char* pbsFilename = NULL;
char* expectFilename = NULL;

#define launcherAccountEnvvar "CHPL_LAUNCHER_ACCOUNT"

Expand Down Expand Up @@ -167,10 +167,17 @@ static char* chpl_launch_create_command(int argc, char* argv[],
#else
mypid = 0;
#endif
snprintf(expectFilename, sizeof(expectFilename), "%s%d", baseExpectFilename,
(int)mypid);
snprintf(pbsFilename, sizeof(pbsFilename), "%s%d", basePBSFilename,
int expectFilenameLen =
strlen(baseExpectFilename) + snprintf(NULL, 0, "%d", (int)mypid) + 1;
expectFilename = (char*)chpl_mem_allocMany(expectFilenameLen, sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
int pbsFilenameLen =
strlen(basePBSFilename) + snprintf(NULL, 0, "%d", (int)mypid) + 1;
pbsFilename = (char*)chpl_mem_allocMany(pbsFilenameLen, sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
snprintf(expectFilename, expectFilenameLen, "%s%d", baseExpectFilename,
(int)mypid);
snprintf(pbsFilename, pbsFilenameLen, "%s%d", basePBSFilename, (int)mypid);

pbsFile = fopen(pbsFilename, "w");
fprintf(pbsFile, "#!/bin/sh\n\n");
Expand Down Expand Up @@ -254,6 +261,8 @@ static void chpl_launch_cleanup(void) {
}
}
#endif
chpl_mem_free(pbsFilename, 0, 0);
chpl_mem_free(expectFilename, 0, 0);
}


Expand Down
97 changes: 53 additions & 44 deletions runtime/src/launch/slurm-gasnetrun_common/slurm-gasnetrun_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ static char* nodelist = NULL;
static char* partition = NULL;
static char* exclude = NULL;
static char* gpusPerNode = NULL;
char slurmFilename[FILENAME_MAX];
char* slurmFilename = NULL;

/* copies of binary to run per node */
#define procsPerNode 1
Expand Down Expand Up @@ -173,17 +173,21 @@ static void genNumLocalesOptions(FILE* slurmFile, sbatchVersion sbatch,
}
}

static int propagate_environment(char* buf, size_t size)
// Append environment variables using chpl_append_to_cmd.
// charsWritten may be NULL, in which case it is ignored.
static void propagate_environment(char** buf, int* charsWritten)
{
int len = 0;
int ignoredCharsWritten;
if (!charsWritten) {
charsWritten = &ignoredCharsWritten;
}

// Indiscriminately propagate all environment variables.
// We could do this more selectively, but we would be likely
// to leave out something important.
char *enviro_keys = chpl_get_enviro_keys(',');
if (enviro_keys)
len += snprintf(buf, size, " -E '%s'", enviro_keys);
return len;
chpl_append_to_cmd(buf, charsWritten, " -E '%s'", enviro_keys);
}

static char* chpl_launch_create_command(int argc, char* argv[],
Expand All @@ -192,8 +196,7 @@ static char* chpl_launch_create_command(int argc, char* argv[],

int i;
int size;
char baseCommand[2*FILENAME_MAX];
char envProp[2*FILENAME_MAX];
char* baseCommand = NULL;
char* command;
FILE* slurmFile;
char* projectString = getenv(launcherAccountEnvvar);
Expand Down Expand Up @@ -273,7 +276,12 @@ static char* chpl_launch_create_command(int argc, char* argv[],
} else {
mypid = getpid();
}
snprintf(slurmFilename, sizeof(slurmFilename), "%s%d", baseSBATCHFilename, (int)mypid);
int slurmFilenameLen =
strlen(baseSBATCHFilename) + snprintf(NULL, 0, "%d", (int)mypid) + 1;
slurmFilename = (char*)chpl_mem_allocMany(slurmFilenameLen, sizeof(char),
CHPL_RT_MD_FILENAME, -1, 0);
snprintf(slurmFilename, slurmFilenameLen, "%s%d", baseSBATCHFilename,
(int)mypid);

if (getenv("CHPL_LAUNCHER_USE_SBATCH") != NULL) {
slurmFile = fopen(slurmFilename, "w");
Expand All @@ -297,8 +305,10 @@ static char* chpl_launch_create_command(int argc, char* argv[],
CHPL_THIRD_PARTY, WRAP_TO_STR(LAUNCH_PATH), GASNETRUN_LAUNCHER,
numLocales, numLocales);

propagate_environment(envProp, sizeof(envProp));
char* envProp = NULL;
propagate_environment(&envProp, NULL);
fprintf(slurmFile, "%s", envProp);
chpl_mem_free(envProp, 0, 0);

fprintf(slurmFile, " %s %s", chpl_get_real_binary_wrapper(), chpl_get_real_binary_name());

Expand All @@ -309,56 +319,54 @@ static char* chpl_launch_create_command(int argc, char* argv[],

fclose(slurmFile);
chmod(slurmFilename, 0755);

snprintf(baseCommand, sizeof(baseCommand), "sbatch %s\n", slurmFilename);
char* format="sbatch %s\n";
int baseCommandLen = strlen(slurmFilename) + strlen(format);
baseCommand = (char*)chpl_mem_allocMany(baseCommandLen, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
snprintf(baseCommand, baseCommandLen, format, slurmFilename);
} else {
char iCom[2*FILENAME_MAX-10];
char* iCom = NULL;
int len = 0;

if (!getSlurmDebug()) {
len += snprintf(iCom+len, sizeof(iCom)-len, "--quiet ");
chpl_append_to_cmd(&iCom, &len, "--quiet ");
}
len += snprintf(iCom+len, sizeof(iCom)-len, "-J %s ", jobName);
len += snprintf(iCom+len, sizeof(iCom)-len, "-N %d ", numNodes);
len += snprintf(iCom+len, sizeof(iCom)-len, "--ntasks=%d ", numLocales);
if (nodeAccessStr != NULL)
len += snprintf(iCom+len, sizeof(iCom)-len, "--%s ", nodeAccessStr);
if (walltime)
len += snprintf(iCom+len, sizeof(iCom)-len, "--time=%s ", walltime);
if (nodelist)
len += snprintf(iCom+len, sizeof(iCom)-len, "--nodelist=%s ", nodelist);
if(partition)
len += snprintf(iCom+len, sizeof(iCom)-len, "--partition=%s ", partition);
if(exclude)
len += snprintf(iCom+len, sizeof(iCom)-len, "--exclude=%s ", exclude);
if(gpusPerNode)
len += snprintf(iCom+len, sizeof(iCom)-len, "--gpus-per-node=%s ", gpusPerNode);
if(projectString && strlen(projectString) > 0)
len += snprintf(iCom+len, sizeof(iCom)-len, "--account=%s ",
projectString);
if (constraint)
len += snprintf(iCom+len, sizeof(iCom)-len, " -C %s", constraint);
len += snprintf(iCom+len, sizeof(iCom)-len,
" %s/%s/%s -n %d -N %d -c 0",
chpl_append_to_cmd(&iCom, &len, "-J %s ", jobName);
chpl_append_to_cmd(&iCom, &len, "-N %d ", numNodes);
chpl_append_to_cmd(&iCom, &len, "--ntasks=%d ", numLocales);
if (nodeAccessStr != NULL) chpl_append_to_cmd(&iCom, &len, "--%s ", nodeAccessStr);
if (walltime) chpl_append_to_cmd(&iCom, &len, "--time=%s ", walltime);
if (nodelist) chpl_append_to_cmd(&iCom, &len, "--nodelist=%s ", nodelist);
if (partition) chpl_append_to_cmd(&iCom, &len, "--partition=%s ", partition);
if (exclude) chpl_append_to_cmd(&iCom, &len, "--exclude=%s ", exclude);
if (gpusPerNode) chpl_append_to_cmd(&iCom, &len, "--gpus-per-node=%s ", gpusPerNode);
if (projectString && strlen(projectString) > 0)
chpl_append_to_cmd(&iCom, &len, "--account=%s ", projectString);
if (constraint) chpl_append_to_cmd(&iCom, &len, "-C %s", constraint);
chpl_append_to_cmd(&iCom, &len, " %s/%s/%s -n %d -N %d -c 0",
CHPL_THIRD_PARTY, WRAP_TO_STR(LAUNCH_PATH),
GASNETRUN_LAUNCHER, numLocales, numNodes);
len += propagate_environment(iCom+len, sizeof(iCom) - len);
len += snprintf(iCom+len, sizeof(iCom)-len, " %s %s",
chpl_get_real_binary_wrapper(),
propagate_environment(&iCom, &len);
chpl_append_to_cmd(&iCom, &len, " %s %s", chpl_get_real_binary_wrapper(),
chpl_get_real_binary_name());
for (i=1; i<argc; i++) {
len += snprintf(iCom+len, sizeof(iCom)-len, " '%s'", argv[i]);
chpl_append_to_cmd(&iCom, &len, " '%s'", argv[i]);
}

snprintf(baseCommand, sizeof(baseCommand), "salloc %s", iCom);
char* format = "salloc %s";
int baseCommandLen = strlen(format) + len + 1;
baseCommand = (char*)chpl_mem_allocMany(baseCommandLen, sizeof(char),
CHPL_RT_MD_COMMAND_BUFFER, -1, 0);
snprintf(baseCommand, baseCommandLen, format, iCom);
chpl_mem_free(iCom, 0, 0);
}

size = strlen(baseCommand) + 1;

command = chpl_mem_allocMany(size, sizeof(char), CHPL_RT_MD_COMMAND_BUFFER, -1, 0);

snprintf(command, size * sizeof(char), "%s", baseCommand);
command =
chpl_mem_allocMany(size, sizeof(char), CHPL_RT_MD_COMMAND_BUFFER, -1, 0);

snprintf(command, size, "%s", baseCommand);
chpl_mem_free(baseCommand, 0, 0);
if (strlen(command)+1 > size) {
chpl_internal_error("buffer overflow");
}
Expand All @@ -385,6 +393,7 @@ int chpl_launch(int argc, char* argv[], int32_t numLocales,
numLocales, numLocalesPerNode),
argv[0]);
chpl_launch_cleanup();
chpl_mem_free(slurmFilename, 0, 0);
return retcode;
}

Expand Down
Loading

0 comments on commit 0d8be34

Please sign in to comment.