Skip to content

CDRIVER-5996 Address EVG task failure due to possible VS 2017 compiler bug #2041

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

Merged
merged 2 commits into from
Jun 18, 2025

Conversation

eramongodb
Copy link
Contributor

@eramongodb eramongodb commented Jun 17, 2025

Followup to #1999. Addresses ongoing VS 2017 test task failures due to what seems to be a VS 2017 compiler bug. Narrowed the issue down to what seems to be a poor interaction between the conditional operator and compound literals, which manifests in the expansion of the mlib_upsize_integer macro:

#include <assert.h>
#include <stdio.h>
#include <stdbool.h>

typedef struct S { int v; } S;

int main(void) {
  int x = 0;

  int y = true ? (S){++x}.v : (S){++x}.v;

  assert(y == 1); // OK
  assert(x == 1); // failure (!?!?)
}

I am unable to reproduce this behavior on any other currently tested or supported compiler (GCC, Clang, MSVC). I could not find any Visual Studio bug reports relating to this behavior. This behavior does not seem to be present with VS 2019 and newer. Quoting the C99 draft:

The first operand is evaluated; there is a sequence point after its evaluation. The second operand is evaluated only if the first compares unequal to 0; the third operand is evaluated only if the first compares equal to 0; the result is the value of the second or third operand (whichever is evaluated), [...]

Similarly quoting the C++11 draft:

The first expression is contextually converted to bool. It is evaluated and if it is true, the result of the conditional expression is the value of the second expression, otherwise that of the third expression. Only one of the second and third expressions is evaluated.

The behavior observed above with VS 2017 clearly violates both of these specifications. I suspect it is an unfortunate consequence of partial C language support in VS 2017. The behavior is as expected if the compound literals are removed:

#include <assert.h>
#include <stdio.h>
#include <stdbool.h>

typedef struct S { int v; } S;

int q(int x, int i) { printf(" - %d: %d\n", i, x); return x; }

int main(void) {
  {
    printf("Compound Initializers:\n");
    int x = 0;
    int y = true ? (S){q(++x, 0)}.v : (S){q(++x, 1)}.v;
    printf(" - x: %d\n - y: %d\n", x, y);
    printf("\n");
  }

  {
    printf("Normal:\n");
    int x = 0;
    int y = true ? q(++x, 0) : q(++x, 1);
    printf(" - x: %d\n - y: %d\n", x, y);
    printf("\n");
  }
}
Compound Initializers:
 - 0: 1
 - 1: 2
 - x: 2
 - y: 1

Normal:
 - 0: 1
 - x: 1
 - y: 1

Rather than complicating the definition of the mlib_* macros any further, this PR proposes simply skipping these tests when compiled with VS 2017 or older. We are unlikely to be (and should be avoiding) using these macros with expressions containing side-effects anyways.

@eramongodb eramongodb self-assigned this Jun 17, 2025
@eramongodb eramongodb requested a review from a team as a code owner June 17, 2025 19:09
@eramongodb eramongodb changed the title CDRIVER-5996 Address EVG task failure due to possible VS 2017 CDRIVER-5996 Address EVG task failure due to possible VS 2017 compiler bug Jun 17, 2025
Copy link
Contributor

@vector-of-bool vector-of-bool left a comment

Choose a reason for hiding this comment

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

LGTM

I suspect this has a similar root cause as the loop-init bug. Probably hoisting the initialization of the anonymous object out of the full expression.

Could you add a backlog Jira ticket for this one, and possibly include a mention of the loop-related bug also? I want to address this properly by changing the macro definition so it doesn't happen when we least expect it.

@eramongodb
Copy link
Contributor Author

Could you add a backlog Jira ticket for this one, and possibly include a mention of the loop-related bug also?

Done: CDRIVER-6043.

Copy link
Collaborator

@kevinAlbs kevinAlbs left a comment

Choose a reason for hiding this comment

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

Interesting, thank you for the investigation and test fix.

@eramongodb eramongodb merged commit 504cba9 into mongodb:master Jun 18, 2025
38 of 41 checks passed
@eramongodb eramongodb deleted the cdriver-vs2017 branch June 18, 2025 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants