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

fix Windows build errors #47350

Merged
merged 5 commits into from Mar 29, 2021

Conversation

jgehw
Copy link
Contributor

@jgehw jgehw commented Feb 23, 2021

fix some more build errors of the kind already addressed in #42676:

compiler doesn't get that LOG(FATAL) macro eventually calls std::abort() and complains with "method doesn't return a value", so call abort explicitely

@google-ml-butler google-ml-butler bot added the size:S CL Change Size: Small label Feb 23, 2021
@google-cla google-cla bot added the cla: yes label Feb 23, 2021
@gbaned gbaned self-assigned this Feb 24, 2021
@gbaned gbaned added the comp:xla XLA label Feb 24, 2021
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation Feb 24, 2021
@gbaned gbaned added the awaiting review Pull request awaiting review label Mar 1, 2021
@joker-eph
Copy link
Contributor

This looks redundant to me: can we find a solution to make LOG(FATAL) work with MSVC? Either adding the abort more prominently there, or use an attribute or any annotation that would appease MSVC?

Alternatively: can a flag be added to the MSVC configuration to disable this warning/error?

@jgehw
Copy link
Contributor Author

jgehw commented Mar 2, 2021

This looks redundant to me: can we find a solution to make LOG(FATAL) work with MSVC? Either adding the abort more prominently there, or use an attribute or any annotation that would appease MSVC?

Due to the fact that the LOG macro expands to a subclass of std::ostringstream, I don't see a way to place abort() anywhere else than in the destructor and at the same time keep the semantics of the LOG macro.

Alternatively: can a flag be added to the MSVC configuration to disable this warning/error?

It's an error (I wouldn't have submitted this PR just to silence a false warning ;-) ), or rather it's tons of error C4716, for example:

C:\tools\vcpkg\buildtrees\.bzl\_bazel_jgehweil\e7zqzfy3\execroot\org_tensorflow\tensorflow\compiler\xla\literal.cc(1391) : error C4716: 'xla::`anonymous namespace'::BitcastBetweenNativeTypes<short,signed char>': must return a value

@jgehw
Copy link
Contributor Author

jgehw commented Mar 3, 2021

In the documentation of the MSVC compiler, I only found switches to handle warnings (suppress them or raise them like errors), but not errors. Nevertheless, I gave it a try to see if the suppress warning switch would also suppress an error, and added the according switch /wd4716 to ./tensorflow/BUILD like this:

tf_cc_shared_object(
    name = "tensorflow_cc",
    linkopts = select({
        "//tensorflow:macos": [
            "-Wl,-exported_symbols_list,$(location //tensorflow:tf_exported_symbols.lds)",
        ],
        "//tensorflow:windows": [],
        "//conditions:default": [
            "-z defs",
            "-Wl,--version-script,$(location //tensorflow:tf_version_script.lds)",
        ],
    }) + select({
        "//tensorflow:msvc_cl_debug": [
            "/DEBUG:FASTLINK",
            "/wd4716",
        ],
        "//conditions:default": [],
    }),
    per_os_targets = True,

But it kept failing with the C4716 error. So, in the end I don't see any better solution than mine.

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 5, 2021
@gbaned gbaned requested a review from sanjoy March 19, 2021 17:49
@gbaned gbaned added the awaiting review Pull request awaiting review label Mar 19, 2021
@sanjoy
Copy link
Contributor

sanjoy commented Mar 19, 2021

The MSVC doc suggests using #pragma; WDYT about putting that in the header that defines the LOG macro?

@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Mar 21, 2021
Copy link
Contributor

@sanjoy sanjoy left a comment

Choose a reason for hiding this comment

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

As per previous comment to try using #pragma.

PR Queue automation moved this from Assigned Reviewer to Reviewer Requested Changes Mar 23, 2021
@jgehw
Copy link
Contributor Author

jgehw commented Mar 23, 2021

As per previous comment to try using #pragma.

I tried to build it with the suggested #pragma solution, and it actually worked (through it just disables a warning and the compiler was reporting an error previously).

I updated the PR accordingly.

PR Queue automation moved this from Reviewer Requested Changes to Approved by Reviewer Mar 25, 2021
@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Mar 25, 2021
@gbaned gbaned added kokoro:force-run Tests on submitted change and removed kokoro:force-run Tests on submitted change labels Mar 25, 2021
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Mar 25, 2021
@copybara-service copybara-service bot merged commit 7b7f3c5 into tensorflow:master Mar 29, 2021
PR Queue automation moved this from Approved by Reviewer to Merged Mar 29, 2021
@jgehw jgehw deleted the fix-windows-build-errors branch March 29, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes comp:xla XLA ready to pull PR ready for merge process size:S CL Change Size: Small
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants