Skip to content
This repository was archived by the owner on Mar 28, 2020. It is now read-only.

Conversation

@hughbe
Copy link
Collaborator

@hughbe hughbe commented Nov 18, 2016

Commit 1:

  • MSVC complains: specific_attr_begin is not a member of swift::Decl
  • The fix is to extract the default parameter and specifiy it for each call of the method

Commit 2:

MSVC implodes compiling Clang, owing to the definition of llvm::None:

namespace llvm {
/// \brief A simple null object to allow implicit construction of Optional<T>
/// and similar types without having to spell out the specialization's name.
enum class NoneType { None };
const NoneType None = None;
}

https://connect.microsoft.com/VisualStudio/feedback/details/3111599/

It throws the following strange, obfuscated error:

Error: Unhandled exception at 0x00007FFFF9669633 (ntdll.dll) in cl.exe: 0xC0000005: Access violation writing location 0x00000036C7A00E90.

[1/665] Building CXX object tools\clang\lib\APINotes\CMakeFiles\clangAPINotes.dir\APINotesYAMLCompiler.cpp.obj
FAILED: tools/clang/lib/APINotes/CMakeFiles/clangAPINotes.dir/APINotesYAMLCompiler.cpp.obj
C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe   /nologo /TP -DCLANG_ENABLE_ARCMT -DCLANG_ENABLE_OBJC_REWRITER -DCLANG_ENABLE_STATIC_ANALYZER -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GNU_SOURCE -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Itools\clang\lib\APINotes -IC:\Users\hbellamy\Documents\GitHub\llvm\tools\clang\lib\APINotes -IC:\Users\hbellamy\Documents\GitHub\llvm\tools\clang\include -Itools\clang\include -Iinclude -IC:\Users\hbellamy\Documents\GitHub\llvm\include /W4 -wd4141 -wd4146 -wd4180 -wd4244 -wd4258 -wd4267 -wd4291 -wd4345 -wd4351 -wd4355 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4800 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4324 -w14062 -we4238 /Zc:inline /Zc:strictStrings /Oi /Zc:rvalueCast -O2   -UNDEBUG  /EHs-c- /GR- /showIncludes /Fotools\clang\lib\APINotes\CMakeFiles\clangAPINotes.dir\APINotesYAMLCompiler.cpp.obj /Fdtools\clang\lib\APINotes\CMakeFiles\clangAPINotes.dir\ /FS -c C:\Users\hbellamy\Documents\GitHub\llvm\tools\clang\lib\APINotes\APINotesYAMLCompiler.cpp
Internal Compiler Error in C:\PROGRA~2\MICROS~1.0\VC\bin\amd64\cl.exe.  You will be prompted to send an error report to Microsoft later.

The fix is to give MSVC some more information. However, it still complains if we use llvm::None, so we have to use llvm::NoneType::None. I assume the problem is that MSVC gets horribly confused at this line: const NoneType None = None;

Commit 3

  • MSVC cannot resolve the operator.
  • The fix is to add a static cast to unsigned: this has been done in other places in the file, and was missing in this case, presumably as an oversight

/cc @DougGregor (this time, these files are swift-specific and no upstream version exists in Clang)

vedantk and others added 30 commits June 1, 2016 10:42
We have to handle file exits before and after visiting regions in the
switch body. Fixes PR27948.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271308 91177308-0d34-0410-b5e6-96231b3b80d8
(cherry picked from commit e0c001c)
I added this call in r271308. It's redundant because it's dominated by a
call to extendRegion().

Thanks to Justin Bogner for pointing this out!

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271331 91177308-0d34-0410-b5e6-96231b3b80d8
(cherry picked from commit a47c5dd)
* origin/swift-3.0-branch:
  [Coverage] Remove redundant handleFileExit() call (NFC)
  [Coverage] Fix crash on a switch partially covered by a macro (PR27948)
It's possible to have multiple local ObjCLifetime qualifiers. When there is
a conflict, we can't stop after we reach a type that is directly qualified.
We need to keep pulling sugar off and removing the ObjCLifetime qualifers.

rdar://25804796

Differential Revision: http://reviews.llvm.org/D20843


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271409 91177308-0d34-0410-b5e6-96231b3b80d8
…dentifiers.

When we import a module that defines a builtin identifier from prefix header and
precompile the prefix header, the macro information related to the identifier
is lost.

If we don't precompile the prefix header, the source file can still see the
macro information. The reason is that we write out the identifier in the pch
but not the macro information since the macro is not defined locally.

This is related to r251565. In that commit, if we read a builtin identifier from
a module that wasn't "interesting" to that module, we will still write it out to
a PCH that imports that module.

The fix is to write exported module macros for PCH as well.

rdar://24666630

Differential Revision: http://reviews.llvm.org/D20383


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271310 91177308-0d34-0410-b5e6-96231b3b80d8
* origin/swift-3.0-branch:
  PCH + module: make sure we write out macros associated with builtin identifiers.
  ObjC lifetime: pull sugar off when the qualifiers conflict.
Cherry-pick fix for swift_newtype attribute doc from upstream-with-swift
* origin/swift-3.0-branch:
  [swift_newtype] Add heading to doc
Instead of setting DeclSpec's range end to point to the next token
after the DeclSpec, we use getLocForEndOfToken to insert fix-it after a type
name.

Before this fix, fix-it will change
^(NSView view) to ^(*NSView view)

This commit correctly updates the source to ^(NSView* view).

rdar://21042144
Differential Revision: http://reviews.llvm.org/D20844


git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271448 91177308-0d34-0410-b5e6-96231b3b80d8
* origin/swift-3.0-branch:
  FixIt: use getLocForEndOfToken to insert fix-it after a type name.
Also use the next enum value for CXObjCPropertyAttr_class.

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271747 91177308-0d34-0410-b5e6-96231b3b80d8

 Conflicts:
	include/clang-c/Index.h
* origin/swift-3.0-branch:
  Bump libclang API minor version after r271351.
  Indexer: add CXObjCPropertyAttr_class for class properties.
Uses error message now provided by LockFileManager in LLVM r271755.

rdar://problem/26529101

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@271758 91177308-0d34-0410-b5e6-96231b3b80d8
(cherry picked from commit bdf3f27)
* origin/swift-3.0-branch:
  [Modules] Improve diagnostics for LockFileManager errors
These new builtins support a mechanism for logging OS events, using a
printf-like format string to specify the layout of data in a buffer.
The _buffer_size version of the builtin can be used to determine the size
of the buffer to allocate to hold the data, and then __builtin_os_log_format
can write data into that buffer. This implements format checking to report
mismatches between the format string and the data arguments. Most of this
code was written by Chris Willmore.
* origin/swift-3.0-branch:
  Clean up some whitespace and comments for the new logging builtins patch.
  Add support for __builtin_os_log_format[_buffer_size]
…triple is a macOS target triple.

See 01436117652b41eb389ddbad9c9fd8df0de9b4b2 in llvm.

rdar://26780128
(cherry picked from commit 466f3ab)
* origin/swift-3.0-branch:
  Use the new lit.util.isMacOSTriple function to determine if a target triple is a macOS target triple.
Add basic tests to ensure the analyzer has support for class properties. This
is a test-only change.

rdar://problem/25256807

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@268773 91177308-0d34-0410-b5e6-96231b3b80d8
(cherry picked from commit c6a175c)
* origin/swift-3.0-branch:
  [analyzer] Add tests for Objective-C class properties
Allow iOS and tvOS version numbers with 2-digit major version numbers.
* origin/swift-3.0-branch:
  Allow iOS and tvOS version numbers with 2-digit major version numbers.
Teach trackNullOrUndefValue() how to properly look through PseudoObjectExprs
to find the underlying semantic method call for property getters. This fixes a
crash when looking through class property getters that I introduced in r265839.

rdar://problem/26796666

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@273340 91177308-0d34-0410-b5e6-96231b3b80d8
(cherry picked from commit 83b8ce1)
Like with SenTestCase, subclasses of XCTestCase follow a "tear down" idiom to
release instance variables and so typically do not release ivars in -dealloc.
This commit applies the existing special casing for SenTestCase to XCTestCase
as well.

rdar://problem/25884696

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@273441 91177308-0d34-0410-b5e6-96231b3b80d8
(cherry picked from commit 5cafc35)
* origin/swift-3.0-branch:
  [analyzer] Teach ObjCDeallocChecker about XCTestCase
  [analyzer] Teach trackNullOrUndefValue() about class property accessors.
…ssors (swift-3.0-branch)

This assert, which was added in r273340, is too strong for swift-3.0-branch

I've weakened it to first strip casts before checking the isa<>().

This is a swift-3.0-branch fixup for rdar://problem/26796666 to make
bots pass when assertions are turned on.
swift-ci and others added 15 commits November 15, 2016 17:49
* origin/swift-3.1-branch:
  Add a method to get the list of registered static analyzer checkers.
* origin/swift-3.1-branch:
  [analyzer] Teach RetainCountChecker about VTCompressionSessionEncodeFrame()
  [analyzer] Provide Contains() on ImmutableMap program state partial trait.
* origin/swift-3.1-branch:
  [analyzer] Update 'Automated' to 'Automatic' from r286694.
  [analyzer] Improve misleading RetainCountChcker diagnostic under ARC
* origin/swift-3.1-branch:
  [analyzer] NumberObjectConversion: Workaround for a linker error with modules.
  [analyzer] NumberObjectConversion: support more types, misc updates.
  [analyzer] Add NumberObjectConversion checker.
  [analyzer] Fix crash in NullabilityChecker calling block with too few arguments
  [analyzer] Fix copy-pasta in NullableReturnedFromNonnullChecker checker name.
* origin/swift-3.1-branch:
  [analyzer] Rename assumeWithinInclusiveRange*()
  [analyzer] Minor optimization: avoid setting state if unchanged
* origin/swift-3.1-branch:
  [www] Update analyzer website for release of checker-279
  [analyzer] Add check for when block is called with too few arguments.
* origin/swift-3.1-branch:
  [Driver] Infer the correct option to ld64 for -fembed-bitcode
* origin/swift-3.1-branch:
  Remove some false positives when taking the address of packed members
  [Sema] Avoid -Wshadow warnings for shadowed variables that aren't captured by lambdas with a default capture specifier
* origin/swift-3.1-branch:
  Cherry-pick to fix rdar://29229698
* origin/swift-3.1-branch:
  Fix PR31029 by attaching an artificial debug location to msabi thunks. This was a latent bug that was recently uncovered by r286400.
* origin/swift-3.1-branch:
  [modules] Mark deleted functions as implicitly inline to allow  merging
  [modules] PR28812: Modules can return duplicate field decls.
  [modules] Fix assert if multiple update records provide a definition for a class template specialization and that specialization has attributes.
* origin/swift-3.1-branch:
  Relax testcase. This removes checks that are irrelevant for what is being tested.
  Add the missing FileCheck invocation to this testcase.
* origin/swift-3.1-branch:
  Improve handling of __FUNCTION__ and other predefined expression for Objective-C Blocks
// See: https://connect.microsoft.com/VisualStudio/feedback/details/3111599/
// This is a workaround that issue until the problem gets fixed.
#if defined(_MSC_VER)
static llvm::Optional<NullabilityKind> AbsentNullability = llvm::NoneType::None;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think llvm::Optional is initialised to none by default, so shouldn't something like this be sufficient: llvm::Optional< NullabilityKind > AbsentNullability {};? This should allow you to get rid of #if checks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @hyp, I should've tagged you, sorry.
I'll take a look. I do recall that MSVC complained about initializing Optional<T> with {} in a different code base, but maybe this will work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This worked, thanks Alex.

Curious, the following: llvm::Optional< NullabilityKind > AbsentNullability = {}; didn't work: I presumed you made a typo msising the =. Clearly you didn't :D. Do you know why?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the form with the = has different semantics. Not sure how they differ, but I think the compiler might treat it as initializer list initialization.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's possible, cheers

@hughbe
Copy link
Collaborator Author

hughbe commented Nov 18, 2016

I just added another commit that fixes another MSVC error. This is the 3rd commit - however, unlike the others, it modifies code that already exists in Clang. However, the good news is that the Clang folks must've fixed this, as the Clang code: https://github.com/llvm-mirror/clang/blob/8f035b6f8c06db3477406d9da4b88ccdc4fca880/lib/Sema/SemaType.cpp#L3529 has the fix

Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

Thanks! Some process notes: nothing new should actually be committed to the 'stable' branch. Instead, changes should go to the 'upstream-with-swift' branch and then cherry-picked to the latest release branch if necessary—in this case 'swift-3.1-branch'. This will then get automerged into 'stable'.

As we get further into a release, we'll start locking down more heavily on cherry-picks to the release branch, requiring them to follow a review template and be approved by a release manager (not just a code owner). But for now we're still early enough in Swift 3.1 that a regular review should suffice.

CopyString(S.Context,
info.UnavailableMsg));
});
}, [](Decl *decl) { return decl->specific_attr_begin<UnavailableAttr>(); });
Copy link
Contributor

Choose a reason for hiding this comment

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

Really not happy with this duplication. If MSVC really can't handle the default argument, maybe you can make an overload instead?

} else {
S.Diag(fileNullability.PointerLoc, diag::warn_nullability_missing)
<< fileNullability.PointerKind;
<< static_cast<unsigned>(fileNullability.PointerKind);
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been fixed upstream; I just hadn't cherry-picked it. Can you take the real commit instead?

@hughbe
Copy link
Collaborator Author

hughbe commented Nov 18, 2016

Some process notes: nothing new should actually be committed to the 'stable' branch.

Sorry, there's not much documentation about the contribution process to these more niche repos. Would you like me to change the target branch? I think that might involve starting a new PR (so maybe the benefits don't outweigh the costs)

@hyp
Copy link
Contributor

hyp commented Nov 18, 2016

@hughbe I think you can change the target branch without creating a new PR using the 'edit' button (at least I can).

@jrose-apple
Copy link
Contributor

We/I keep meaning to write down the branch structure somewhere convenient. I'll take this as a reminder to do that after (American) Thanksgiving.

@hughbe hughbe changed the base branch from stable to swift-3.1-branch November 18, 2016 17:21
@hughbe
Copy link
Collaborator Author

hughbe commented Nov 18, 2016

@hyp good in theory, in practice GitHub just destroyed this PR :D

@hughbe
Copy link
Collaborator Author

hughbe commented Nov 18, 2016

I'm gonna have to close this PR, and I screwed up again - sorry all for the double notifications. First one was GitHub's fault, second was mine :D

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.