Factor out code that should move to cxplat #82

dthaler · 2023-08-18T00:55:12Z

Move leak detection and fault injection to cxplat_winuser
Add tests for memory realloc
Add cxplat_test to CI/CD

Signed-off-by: Dave Thaler <[email protected]>

cxplat/inc/cxplat.h

cxplat/inc/cxplat/common.h

nibanks · 2023-08-18T11:41:23Z

cxplat/inc/cxplat/fault_injection.h

-    NTSTATUS
-    usersim_fault_injection_initialize(size_t stack_depth) noexcept;
+    int
+    cxplat_fault_injection_initialize(size_t stack_depth) NOEXCEPT;


You already extern "c" all these functions. Why do you have the NOEXCEPT?

See https://stackoverflow.com/questions/24362616/does-the-c-standard-mandate-that-c-linkage-functions-are-noexcept

"it's a good idea that user defined extern "C" functions be also specified with noexcept, unless it is handed a pointer to C++ function as callback argument like qsort and the likes"

nibanks · 2023-08-18T11:43:14Z

cxplat/inc/cxplat/common.h

+#endif
+
+#ifdef _DEBUG
+#define cxplat_assert(x) assert(x)


For MsQuic, we generally follow the pattern that #define macros are always all upper case: CXPLAT_ASSERT. Additionally, we have several types of asserts:

#define CXPLAT_DBG_ASSERT(_exp) #define CXPLAT_DBG_ASSERTMSG(_exp, _msg) #define CXPLAT_TEL_ASSERT(_exp) #define CXPLAT_TEL_ASSERTMSG(_exp, _msg) #define CXPLAT_TEL_ASSERTMSG_ARGS(_exp, _msg, _origin, _bucketArg1, _bucketArg2) #define CXPLAT_FRE_ASSERT(_exp) #define CXPLAT_FRE_ASSERTMSG(_exp, _msg)

FRE is super confusing and not cross-platform as a term. I propose using _RUNTIME_

I'm definitely open to other terms. Not sure how I feel about RUNTIME though.

I'm open to other terms too :)

cxplat/inc/cxplat.h

nibanks · 2023-08-18T11:46:54Z

cxplat/inc/cxplat/memory.h

+     * @param[in] size Size of memory to allocate.
+     * @returns Pointer to memory block allocated, or null on failure.
+     */
+    _Must_inspect_result_ _Ret_writes_maybenull_(size) void* cxplat_allocate(size_t size);


Super happy to see the SAL annotations, but you will need a stub file for posix platforms. Feel free to steal quic_sal_stub.h and rename to cxplat_sal_stub.h or whatever you'd like.

agreed. not needed for this PR but agree it will be needed eventually.

I propose we do this after moving code to the cxplat repo, since a stub file isn't needed for the usersim project which does not support posix platforms.

cxplat/inc/cxplat/common.h

Signed-off-by: Dave Thaler <[email protected]>

nibanks · 2023-08-18T17:20:14Z

cxplat/inc/winuser/cxplat_platform.h

+#pragma once
+// Do not add anything else to this file.
+// Actual platform-specific definitions go in the file below.
+#include "cxplat_winuser.h"


Suggested change

#include "cxplat_winuser.h"

#ifdef _KERNEL_MODE

#error "Windows kernel platform is unsupported"

#elif _WIN32

#include "cxplat_winuser.h"

#else

#error "Unknown unsupported Platform"

#endif

Maybe make this a bit explicit right off the bat?

I have a general distaste for ifdefs, they clutter the code and can easily leave dead code sections.

I don't feel strongly about this, so may adopt this suggestion in the next PR

We can discuss more in a follow up PR, but I don't want each consumer of cxplat to be required to have platforms specific logic to change the include path. They should include one path, and just #include <cxplat_platform.h> and be good to go.

Filed #83 to track

nibanks · 2023-08-18T17:22:09Z

cxplat/inc/winuser/cxplat_winuser.h

+// SPDX-License-Identifier: MIT
+#pragma once
+#include <winerror.h>
+


I'd also recommend:

#define CXPLAT_FAILED(X) FAILED(X) #define CXPLAT_SUCCEEDED(X) SUCCEEDED(X)

I dislike having two macros that do the same thing. How is FAILED different from !SUCCEEDED?

98% of the time, code uses the failed marco for error conditions, so if we did have to pick just 1, I'd prefer that. But I do like having two for the cases where you want the opposite. Having english words instead of just ! the other is nicer (IMO) to read.

Signed-off-by: Dave Thaler <[email protected]>

nibanks · 2023-08-20T00:23:27Z

cxplat/inc/cxplat.h

+    cxplat_status_t
+    cxplat_initialize();


Would you mind not having the spaces before all these lines? IMHO, it's a lot of wasted space.

I think clang-format did it, the formatting isn't supposed to be manual.
We can see if there's a config option to do what you want.

nibanks · 2023-08-20T00:24:47Z

cxplat/inc/cxplat.h

+#ifdef __cplusplus
+extern "C"
+{
+#endif


To simplify this stuff, we can define EXTERN_C (or EXTERN_C_START) and EXTERN_C_END in cxplat_common.h for this. It would reduce the duplication and line count a bit. It would also hide the { and } to eliminate any visible/explicit need to tab things over.

nibanks · 2023-08-20T00:27:27Z

cxplat/inc/cxplat_common.h

+{
+    CXPLAT_STATUS_SUCCESS = CXPLAT_PLATFORM_STATUS_SUCCESS,
+    CXPLAT_STATUS_NO_MEMORY = CXPLAT_PLATFORM_STATUS_NO_MEMORY,
+} cxplat_status_t;


There is a problem with defining cxplat_status_t as an enum instead of the platform type (HRESULT or NTSTATUS): you lose the "success" SAL annotation of those types that is then used for evaluation of a function being successful or not.

typedef _Return_type_success_(return >= 0) long HRESULT; typedef _Return_type_success_(return >= 0) LONG NTSTATUS;

I'm not sure if we can get the same thing on this enum.

I believe yes we can get it on an enum.

nibanks · 2023-08-20T00:30:05Z

cxplat/inc/cxplat_common.h

+// Include platform-specific definitions of common defines.
+#include "cxplat_platform.h"
+
+#include <assert.h>


FYI, in MsQuic's platform abstraction, the definition of assert is platform specific, and does not use assert.h. On Windows we have:

#define CXPLAT_ASSERT_LOG(_exp, _msg) \ (CXPLAT_ANALYSIS_ASSUME(_exp), \ ((!(_exp)) ? (CxPlatLogAssert(__FILE__, __LINE__, #_exp), FALSE) : TRUE)) #define CXPLAT_ASSERT_CRASH(_exp, _msg) \ (CXPLAT_ANALYSIS_ASSUME(_exp), \ ((!(_exp)) ? \ (CxPlatLogAssert(__FILE__, __LINE__, #_exp), \ __annotation(L"Debug", L"AssertFail", _msg), \ DbgRaiseAssertionFailure(), FALSE) : \ TRUE))

nibanks · 2023-08-20T00:34:55Z

cxplat/src/cxplat_winkernel/cxplat_winkernel.vcxproj

+  Copyright (c) Microsoft Corporation
+  SPDX-License-Identifier: MIT
+-->
+<Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">


We can do this in a follow up PR, but this project file needs some work. It won't load on xbox for instance.

Filed #85 to track

nibanks · 2023-08-20T00:36:43Z

cxplat/src/cxplat_winuser/cxplat_winuser.cpp

+cxplat_initialize()
+{
+    try {
+        auto fault_injection_stack_depth =


Eventually, this all needs to be DEBUG only.

nibanks

Great work so far.

Signed-off-by: Dave Thaler <[email protected]>

dthaler requested review from Alan-Jowett and mtfriesen as code owners August 18, 2023 00:55

dthaler marked this pull request as draft August 18, 2023 00:55

Add cxplat projects

0bb2086

Signed-off-by: Dave Thaler <[email protected]>

nibanks reviewed Aug 18, 2023

View reviewed changes

cxplat/inc/cxplat.h Outdated Show resolved Hide resolved

nibanks reviewed Aug 18, 2023

View reviewed changes

cxplat/inc/cxplat/common.h Outdated Show resolved Hide resolved

nibanks reviewed Aug 18, 2023

View reviewed changes

cxplat/inc/cxplat.h Outdated Show resolved Hide resolved

nibanks reviewed Aug 18, 2023

View reviewed changes

cxplat/inc/cxplat/common.h Outdated Show resolved Hide resolved

dthaler added 3 commits August 18, 2023 08:40

Move fault injection and leak detection to be cxplat apis

79105fd

Signed-off-by: Dave Thaler <[email protected]>

More fixes

3f3550b

Signed-off-by: Dave Thaler <[email protected]>

PR feedback

2dd996d

Signed-off-by: Dave Thaler <[email protected]>

dthaler force-pushed the cxplat branch from 73a6e30 to 2dd996d Compare August 18, 2023 16:36

Add cxplat_status_t

803bc5d

Signed-off-by: Dave Thaler <[email protected]>

nibanks reviewed Aug 18, 2023

View reviewed changes

dthaler added 5 commits August 18, 2023 10:30

More use of cxplat_status_t

e6bade4

Signed-off-by: Dave Thaler <[email protected]>

Fix cxplat_reallocate

de95d42

Signed-off-by: Dave Thaler <[email protected]>

Add cxplat memory API tests

71288e0

Signed-off-by: Dave Thaler <[email protected]>

Fix cmake build and add cxplat_test to CI/CD

a79c32d

Signed-off-by: Dave Thaler <[email protected]>

Fix release build

cf34537

Signed-off-by: Dave Thaler <[email protected]>

dthaler marked this pull request as ready for review August 18, 2023 21:29

dthaler changed the title ~~WIP: factor out code that should move to cxplat~~ Factor out code that should move to cxplat Aug 18, 2023

Cleanup

ea0d796

Signed-off-by: Dave Thaler <[email protected]>

dthaler force-pushed the cxplat branch 3 times, most recently from ec55537 to 6c6a136 Compare August 18, 2023 22:09

Update cxplat environment variable names

ae11f10

Signed-off-by: Dave Thaler <[email protected]>

dthaler force-pushed the cxplat branch from 6c6a136 to ae11f10 Compare August 18, 2023 23:18

Update winkernel headers

bb99536

Signed-off-by: Dave Thaler <[email protected]>

dthaler force-pushed the cxplat branch from b8852cd to b731cb7 Compare August 19, 2023 17:53

Move string allocation APIs to cxplat library

3c4098f

Signed-off-by: Dave Thaler <[email protected]>

dthaler force-pushed the cxplat branch from b731cb7 to 3c4098f Compare August 19, 2023 18:25

nibanks reviewed Aug 20, 2023

View reviewed changes

nibanks approved these changes Aug 20, 2023

View reviewed changes

Fix compiler warnings

677975d

Signed-off-by: Dave Thaler <[email protected]>

This was referenced Aug 20, 2023

ifdefs per platform #83

Open

Define extern C macros #84

Closed

vcxproj file won't load on xbox #85

Open

dthaler merged commit 0c9cc82 into main Aug 20, 2023

dthaler deleted the cxplat branch August 20, 2023 15:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out code that should move to cxplat #82

Factor out code that should move to cxplat #82

dthaler commented Aug 18, 2023 •

edited

Loading

nibanks Aug 18, 2023

dthaler Aug 18, 2023

nibanks Aug 18, 2023 •

edited

Loading

dthaler Aug 18, 2023

nibanks Aug 18, 2023

dthaler Aug 18, 2023

nibanks Aug 18, 2023

dthaler Aug 19, 2023

dthaler Aug 20, 2023

nibanks Aug 18, 2023

dthaler Aug 18, 2023

dthaler Aug 19, 2023

nibanks Aug 20, 2023

dthaler Aug 20, 2023

nibanks Aug 18, 2023

dthaler Aug 18, 2023

nibanks Aug 20, 2023

nibanks Aug 20, 2023

dthaler Aug 20, 2023

nibanks Aug 20, 2023

dthaler Aug 20, 2023 •

edited

Loading

nibanks Aug 20, 2023

dthaler Aug 20, 2023

dthaler Aug 20, 2023

nibanks Aug 20, 2023

nibanks Aug 20, 2023

dthaler Aug 20, 2023

nibanks Aug 20, 2023

dthaler Aug 20, 2023

nibanks left a comment

-#include "cxplat_winuser.h"
+#ifdef _KERNEL_MODE
+#error "Windows kernel platform is unsupported"
+#elif _WIN32
+#include "cxplat_winuser.h"
+#else
+#error "Unknown unsupported Platform"
+#endif

Factor out code that should move to cxplat #82

Factor out code that should move to cxplat #82

Conversation

dthaler commented Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibanks Aug 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dthaler Aug 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibanks left a comment

Choose a reason for hiding this comment

dthaler commented Aug 18, 2023 •

edited

Loading

nibanks Aug 18, 2023 •

edited

Loading

dthaler Aug 20, 2023 •

edited

Loading