Skip to content

Fix 3C definedType test failure on Windows X86 and re-enable the test #345

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

Closed
mgrang opened this issue Dec 5, 2020 · 5 comments · Fixed by #460
Closed

Fix 3C definedType test failure on Windows X86 and re-enable the test #345

mgrang opened this issue Dec 5, 2020 · 5 comments · Fixed by #460

Comments

@mgrang
Copy link

mgrang commented Dec 5, 2020

@mattmccutchen-cci

After checkedc#930 merged a number of 3C unit tests failed on Windows builds. These were temporarily disabled: checkedc#952 to unblock the builds. Need fix the underlying issues and enable these tests.

@mattmccutchen-cci
Copy link
Member

Thanks. We'll find the right person to look into this on Monday.

@mattmccutchen-cci mattmccutchen-cci changed the title Re-enable 3C unit tests Fix 3C regression test failures on Windows and re-enable the tests Dec 7, 2020
@mattmccutchen-cci
Copy link
Member

mattmccutchen-cci commented Dec 30, 2020

I'm keeping this issue open for us to fix the definedType test on Windows X86 after Microsoft fixes the problem that currently makes the tests difficult to run on Windows X86 (checkedc#963).

@mattmccutchen-cci mattmccutchen-cci changed the title Fix 3C regression test failures on Windows and re-enable the tests Fix 3C definedType test failure on Windows X86 and re-enable the test Dec 30, 2020
@mattmccutchen-cci mattmccutchen-cci added the Upstream Work item that has to deal with making changes to upstream. label Dec 30, 2020
@mattmccutchen-cci mattmccutchen-cci removed the Upstream Work item that has to deal with making changes to upstream. label Feb 25, 2021
@mattmccutchen-cci
Copy link
Member

Now that we're able to run the Windows X86 tests, I tracked down why the test was failing on X86 and not X64.

#define ulong unsigned long
ulong * TOP;
// CHECK_NOALL: ulong * TOP;
// CHECK_ALL: _Array_ptr<ulong> TOP = ((void *)0);
ulong channelColumns;
void
DescribeChannel(void)
{
ulong col;
TOP = (ulong *)malloc((channelColumns+1) * sizeof(ulong));
// CHECK_ALL: TOP = (_Array_ptr<unsigned long>)malloc<unsigned long>((channelColumns+1) * sizeof(ulong));
// CHECK_NOALL: TOP = (ulong *)malloc<unsigned long>((channelColumns+1) * sizeof(ulong));
TOP[col] = 0;
}

The malloc is generating "Unsafe call to allocator function" on X86 but not X64, which causes TOP to remain wild and fail the CHECK_ALL. Here are the AST dumps of the right side of the multiplication:

X86:

ImplicitCastExpr 0xa33ca0 'unsigned long' <IntegralCast>
`-UnaryExprOrTypeTraitExpr 0xa33c88 'unsigned int' sizeof 'unsigned long'

X64:

UnaryExprOrTypeTraitExpr 0x21c475cece8 'unsigned long long' sizeof 'unsigned long'

So sizeof appears to return unsigned int on X86 and unsigned long long on X64, which is consistent with the definition of size_t in the system headers. Multiplication automatically casts the argument of the "smaller" type to the "larger" type, and unsigned int is considered smaller than unsigned long for this purpose even though both types are 32 bits. So on X86, the sizeof gets casted to unsigned long, while on X64, the (channelColumns+1) gets casted to unsigned long long and there is no cast on the sizeof.

The ImplicitCastExpr is preventing this code from detecting the sizeof:

static bool getSizeOfArg(Expr *Arg, QualType &ArgTy) {
if (auto *SizeOf = dyn_cast<UnaryExprOrTypeTraitExpr>(Arg))
if (SizeOf->getKind() == UETT_SizeOf) {
ArgTy = SizeOf->getTypeOfArgument();
return true;
}
return false;
}

I guess getSizeOfArg should just do Arg = Arg->IgnoreParenImpCasts() like analyzeAllocExpr does here:
E = E->IgnoreParenImpCasts();

Next questions: Is there a reasonable way to test this that is independent of the OS and architecture? Are there any other obvious places in 3C that we should test for similar bugs while we're here?

@mattmccutchen-cci
Copy link
Member

Is there a reasonable way to test this that is independent of the OS and architecture?

The only straightforward testing approach I can see needs an integer type larger than size_t to force conversion of the sizeof. If we use unsigned __int128 if available and otherwise uintmax_t, that will work with Clang in many environments (including our typical one: Linux x86_64 with the GCC system headers, I think?). We could consider having another variant of the test that forces -m32 to get a second shot at it, though that may fail if the user doesn't have the 32-bit system headers installed. Does the unsigned __int128 / uintmax_t approach sound good enough? @john-h-kastner or @kyleheadley?

@john-h-kastner
Copy link
Collaborator

I'm satisfied as long as we can test this behavior on current 64-bit linux and don't cause test failures on other configurations.

mattmccutchen-cci added a commit that referenced this issue Feb 26, 2021
This was the cause of the `definedType.c` failure on Windows X86, so we
can now re-enable that test. We also have a dedicated test for the bug
that should work in our main Linux environment.

Fixes #345.
mattmccutchen-cci added a commit that referenced this issue Feb 26, 2021
This was the cause of the `definedType.c` failure on Windows X86, so we
can now re-enable that test. We also have a dedicated test for the bug
that should work in our main Linux environment.

Fixes #345.
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 a pull request may close this issue.

3 participants