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

Test the return value of omMMapBinaryFile function and terminate the main program elegantly #3002

Merged
merged 13 commits into from
Nov 15, 2024
27 changes: 13 additions & 14 deletions src/Runtime/OMExternalConstant.inc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ const int i = 1;
void checkEndianness(const char constPackIsLE) {
if (XOR(IS_SYSTEM_LE(), constPackIsLE)) {
fprintf(stderr, "Constant pack is stored in a byte order that is not "
"native to this current system.");
"native to this current system: %s", strerror(errno));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I probably didn't make it clear. errno is only meaningful after you make a library call such as strdup, open, malloc, etc. Here strerror(errno) is meaningless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thought. Will remove them.

exit(1);
}
}
Expand All @@ -67,15 +67,15 @@ bool omMMapBinaryFile(
// Convert the file name to EBCDIC for the open call.
char *tPath = strdup(fname);
if (!tPath) {
fprintf(stderr, "Error while strdup");
fprintf(stderr, "Error while strdup: %s", strerror(errno));
return false;
}
__a2e_s(tPath);
fname = tPath;
#endif

if (constAddr == NULL) {
perror("Error: null pointer");
fprintf(stderr, "Error: null pointer: %s", strerror(errno));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strerror(errno) is also meaningless.

#ifdef __MVS__
free(tPath);
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not a big deal but a bit of an eye sore to see the #ifdef __MVS__ everywhere. Maybe it's simpler to just define two macros:

#ifdef __MVS__
#define CONV_PATH(p) \
  char *p = strdup(fname); \
  if (!p) { \
    fprintf(stderr, "Error while strdup: %s", strerror(errno)); \
    return false; \
  } \
  __a2e_s(p); \
  fname = p
#define FREE_PATH(p) free(p)
#else
#define CONV_PATH(p)
#define FREE_PATH(p)
#endif

and then just use CONV_PATH(tPath) and FREE_PATH(tPath) everywhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gongsu832 all of these issues come from the fact that fname is not in EBCDIC. So I modify the code generation to convert fname to EBCDIC during compilation so that fname here would be alread in EBCDIC, then there is no need to handle it here anymore.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK that makes things simpler.

Expand All @@ -89,25 +89,22 @@ bool omMMapBinaryFile(
size_t baseLen = strlen(basePath);
size_t fnameLen = strlen(fname);
size_t sepLen = strlen(DIR_SEPARATOR);
size_t filePathLen = baseLen + sepLen + fnameLen;
size_t filePathLen = baseLen + sepLen + fnameLen + 1;
filePath = (char *)malloc(filePathLen);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to +1 to filePathLen to account for the \0 you add to the end later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I updated this.

if (!filePath) {
fprintf(stderr, "Error while malloc");
fprintf(stderr, "Error while malloc: %s", strerror(errno));
#ifdef __MVS__
free(tPath);
#endif
return false;
}
memcpy(filePath, basePath, baseLen);
memcpy(filePath + baseLen, DIR_SEPARATOR, sepLen);
memcpy(filePath + baseLen + sepLen, fname, fnameLen);
filePath[filePathLen] = '\0';
snprintf(filePath, filePathLen, "%s%s%s", basePath, DIR_SEPARATOR, fname);
} else {
filePath = (char *)fname;
}
int fd = open(filePath, O_RDONLY);
if (fd < 0) {
fprintf(stderr, "Error while opening %s\n", filePath);
fprintf(stderr, "Error while opening %s: %s\n", filePath, strerror(errno));
if (basePath)
free(filePath);
#ifdef __MVS__
Expand All @@ -123,7 +120,7 @@ bool omMMapBinaryFile(
#endif

if (tempAddr == MAP_FAILED) {
fprintf(stderr, "Error while mmapping %s\n", fname);
fprintf(stderr, "Error while mmapping %s: %s\n", fname, strerror(errno));
close(fd);
if (basePath)
free(filePath);
Expand Down Expand Up @@ -158,7 +155,9 @@ bool omMMapBinaryFile(
/* Make sure constAddr is the same as the mmap address.
*/
if (constAddr[0] != tempAddr) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is wrong. Only the first thread that successfully performs the compare-and-swap will have constAddr[0] == tempAddr. All other threads will have constAddr[0] != tempAddr. But it doesn't matter since by design they will throw away their tempAddr and use the constAddr[0] set by the first thread.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does tempAddr become NULL after munmap? If so, we do the test only when tempAddr != NULL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explicitly set tempAddr = NULL after munmap, and check constAddr[0] != tempAddr only when tempAddr != NULL (meaning for the first successful thread).

Copy link
Collaborator

@gongsu832 gongsu832 Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does tempAddr become NULL after munmap? If so, we do the test only when tempAddr != NULL.

Not sure what you mean. munmap cannot change tempAddr since C is passing by value (i.e., inside munmap it only has access to a copy of tempAddr). But even if tempAddr somehow becomes NULL after munmap, the check constAddr[0] != tempAddr is still wrong.

Copy link
Collaborator

@gongsu832 gongsu832 Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I explicitly set tempAddr = NULL after munmap, and check constAddr[0] != tempAddr only when tempAddr != NULL (meaning for the first successful thread).

Not sure why you want to do that. A successful compare-and-swap guarantees that constAddr[0] == tempAddr so the check would be redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are right. In the latest code, I set tempAddr = NULL for failed threads, and check if (tempAddr && (constAddr[0] != tempAddr)). What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to avoid the situation we encountered where cds was written wrongly. Perhaps, assert is better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I removed the check. I was overthinking about having a check to debug, but since we found the issue, and cds would work correctly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes you can use assert but it wouldn't help in this case. Because even if you write your cds wrong like the bug we just had, in the first thread after the cds you will have constAddr[0] == tempAddr. Because cds doesn't know/care if the tempAddr you supplied in the call is garbage. All it does is setting constAddr[0] to tempAddr atomically, regardless of what's in tempAddr.

fprintf(stderr, "Error while updating the buffer address for constants\n");
fprintf(stderr,
"Error while updating the buffer address for constants: %s\n",
strerror(errno));
return false;
}
return true;
Expand All @@ -181,11 +180,11 @@ bool omMMapBinaryFile(
void omGetExternalConstantAddr(
void **outputAddr, void **baseAddr, int64_t offset) {
if (outputAddr == NULL) {
perror("Error: null pointer");
fprintf(stderr, "Error: null pointer: %s", strerror(errno));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strerror(errno) is meaningless here.

return;
}
if (baseAddr == NULL) {
perror("Error: null pointer");
fprintf(stderr, "Error: null pointer: %s", strerror(errno));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strerror(errno) is meaningless here.

return;
}
// Constant is already loaded. Nothing to do.
Expand Down
Loading