Fix Main - ignore warning 6101 by vhvb1989 · Pull Request #2471 · Azure/azure-sdk-for-cpp

vhvb1989 · 2021-06-23T00:07:26Z

fixes: #2354

vhvb1989 · 2021-06-23T00:08:42Z

/azp run cpp - core

azure-pipelines · 2021-06-23T00:08:57Z

Azure Pipelines successfully started running 1 pipeline(s).

RickWinter

This warning is a great one to keep as it checks for unitialized memory. Why do we need to disable it?

RickWinter · 2021-06-23T00:17:25Z

  # - C6285: (<non-zero constant> || <non-zero constant>) -> VBProject static analysis on Functional header:
  #          _Is_large, regression from VS version 19.28.29915.0 to 19.29.30037.0
-  target_compile_options(azure-core PUBLIC /wd6285)
+  target_compile_options(azure-core PUBLIC /wd6285 /wd6101)


Can you add a comment in the source that describes what /wd6101 does?

# warning C6101: Returning uninitialized memory # https://docs.microsoft.com/en-us/cpp/code-quality/c6101?view=msvc-160

ahsonkhan

I had the same question, where is this warning coming from?

vhvb1989 · 2021-06-23T00:37:48Z

@ahsonkhan

Windows Kits\10\include\10.0.19041.0\um\ws2tcpip.h(968): warning C6101: Returning uninitialized memory '*Mtu'. A successful path through the function does not set the named Out parameter. : Lines: 968, 973, 974, 975, 976, 978, 979, 994, 968

ahsonkhan · 2021-06-23T00:42:38Z

Windows Kits\10\include\10.0.19041.0\um\ws2tcpip.h(968): warning C6101: Returning uninitialized memory '*Mtu'. A successful path through the function does not set the named Out parameter. : Lines: 968, 973, 974, 975, 976, 978, 979, 994, 968

What code in our SDK source is triggering this? Were you able to find/drill into the specific source file that we own?

vhvb1989 · 2021-06-23T00:49:37Z

/azp run cpp - core

azure-pipelines · 2021-06-23T00:49:50Z

Azure Pipelines successfully started running 1 pipeline(s).

vhvb1989 · 2021-06-23T01:31:11Z

/azp run cpp - core

azure-pipelines · 2021-06-23T01:31:24Z

Azure Pipelines successfully started running 1 pipeline(s).

…in-build

ahsonkhan · 2021-06-23T18:43:46Z

-  uint8_t buffer[chunk];
-  uint8_t buffer2[chunk];


Out of curiosity, what was the bug here?

MSVC complains or throw a warning saying that the method is allocating memory in the stack beyond the limit. That's due to creating two arrays of 1M each, resulting in 2MB stack allocation.
Updating the arrays to a vector would use heap for the 2MB

ahsonkhan · 2021-06-23T18:44:45Z

+if (MSVC)
+  # Disable warnings
+  # - C6326: Google comparisons 
+  target_compile_options(azure-perf-unit-test PUBLIC /wd6326)


Can you point to an example of our source that triggered this warning?

We have this warning disabling in azure-core-tests as well and other test packages,
MSCV warns you when we do something like ASSERT_EQ(a, b) where a is a size_t const and b is size_t as well. MSVC will say that you are just comparing two constants as 4 == 2 or something like that. Because ASSER_EQ macro grom gtest ends up checking something like if(VALUE == OTHER_VALUE) => if(4 == 2).

antkmsft · 2021-06-23T18:53:11Z

-  #          _Is_large, regression from VS version 19.28.29915.0 to 19.29.30037.0
-  target_compile_options(azure-core PUBLIC /wd6285)
+  # - C6101: Returning uninitialized memory '*Mtu' -> libcurl calling WSAGetIPUserMtu from WS2tcpip.h
+  target_compile_options(azure-core PUBLIC /wd6101)


Is there a cpp file for which, when being compiled, this warning gets emitted? If so, it is better to disable the warning via "... ifdef _MSC_VER, pragma warning disable".

If there's a header which includes the 3rd party header file that generates the warning, it also is better to disable as "... ifdef _MSC_VER, pragma warnings push, pragma warning disable, include <3rd party>, ifdef MSC pragma pop"

@vhvb1989 Is this warning only coming from libcurl cpp file? Can you share where it is coming from (which line of code)?

ahsonkhan

Since this is to unblock main, it's fine to check this in as a stop gap for now.

It would be nicer to scope the warning to only where we know it is a false positive, rather than across the board, since it seems like a useful warning to keep.

vhvb1989 · 2021-06-23T23:18:26Z

/azp run cpp - core

azure-pipelines · 2021-06-23T23:18:39Z

Azure Pipelines successfully started running 1 pipeline(s).

ahsonkhan · 2021-06-24T00:54:27Z

  - template: /eng/pipelines/templates/steps/cmake-build.yml
    parameters:
      GenerateArgs: $(CmakeArgs)
+      BuildArgs: "$(BuildArgs)"


@danieljurek - I am not the expert here, can you review/speak to this?

This was not causing Main to be broken, but it was missing to add the build args from the test matrix ( -v and --parallel )

This looks good to me. In the case of the Ubuntu and Windows 2019 machines, the VMs have 2 cores. For I/O bound operations, like building, the process can complete faster with a degree of parallelism that is greater than the number of cores.

In the case of cmake-build.yml this parameter is inserted into the build command line: https://github.com/Azure/azure-sdk-for-cpp/blob/main/eng/pipelines/templates/steps/cmake-build.yml#L20

BuildArgs is a variable and its value is set in the matrix above (if the value is not set somewhere then it is an empty string).

The template syntax:

cmake --build . ${{ parameters.BuildArgs }}

After template substitution becomes:

cmake --build . $(BuildArgs)

In the case of MacOS_x64_with_unit_test in the matrix:

cmake --build . -j 4

According to cmake documentation for the -j parameter:

The CMAKE_BUILD_PARALLEL_LEVEL environment variable, if set, specifies a default parallel level when this option is not given.

Another way to get this outcome is to use the CMAKE_BUILD_PARALLEL_LEVEL.

Either approach will get the same effects. It might be easier to use the command line approach taken in this PR as, if the command line is logged, one can copy and paste the logged command line locally. Otherwise one would have to dig through and reassemble the state of the environment variables at build time starting at the matrix and working up to the job level (in the event that there are some global variables defined)

Ignoring msvc warning 6101

0c58dd1

vhvb1989 requested review from RickWinter, ahsonkhan, antkmsft and schaabs as code owners June 23, 2021 00:07

vhvb1989 changed the title ~~Fix Main - ignore warning~~ Fix Main - ignore warning 6101 Jun 23, 2021

vhvb1989 self-assigned this Jun 23, 2021

vhvb1989 added Azure.Core Client This issue points to a problem in the data-plane of the library. KeyVault labels Jun 23, 2021

vhvb1989 added this to the [2021] July milestone Jun 23, 2021

RickWinter reviewed Jun 23, 2021

View reviewed changes

ahsonkhan reviewed Jun 23, 2021

View reviewed changes

remove not required

a1ea850

not required

01f9a2f

perf issues

5ece837

Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into fix-ma…

090af7b

…in-build

ahsonkhan reviewed Jun 23, 2021

View reviewed changes

antkmsft approved these changes Jun 23, 2021

View reviewed changes

ahsonkhan approved these changes Jun 23, 2021

View reviewed changes

fix livetests build args

a46a58b

vhvb1989 requested review from a team and danieljurek as code owners June 23, 2021 23:16

vhvb1989 mentioned this pull request Jun 23, 2021

Is there a cpp file for which, when being compiled, this warning gets emitted? If so, it is better to disable the warning via "... ifdef _MSC_VER, pragma warning disable". #2474

Closed

vhvb1989 enabled auto-merge (squash) June 23, 2021 23:47

vhvb1989 merged commit 7a31ca5 into Azure:main Jun 23, 2021

vhvb1989 deleted the fix-main-build branch June 23, 2021 23:50

ahsonkhan reviewed Jun 24, 2021

View reviewed changes

Conversation

vhvb1989 commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vhvb1989 commented Jun 23, 2021

Uh oh!

azure-pipelines Bot commented Jun 23, 2021

Uh oh!

RickWinter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

vhvb1989 commented Jun 23, 2021

Uh oh!

ahsonkhan commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vhvb1989 commented Jun 23, 2021

Uh oh!

azure-pipelines Bot commented Jun 23, 2021

Uh oh!

vhvb1989 commented Jun 23, 2021

Uh oh!

azure-pipelines Bot commented Jun 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

vhvb1989 commented Jun 23, 2021

Uh oh!

azure-pipelines Bot commented Jun 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vhvb1989 commented Jun 23, 2021 •

edited

Loading

ahsonkhan commented Jun 23, 2021 •

edited

Loading

ahsonkhan Jun 23, 2021 •

edited

Loading