{"action":"created","issue":{"url":"https://api.github.com/repos/Microsoft/llvm/issues/86","labels_url":"https://api.github.com/repos/Microsoft/llvm/issues/86/labels{/name}","comments_url":"https://api.github.com/repos/Microsoft/llvm/issues/86/comments","events_url":"https://api.github.com/repos/Microsoft/llvm/issues/86/events","html_url":"https://github.com/Microsoft/llvm/pull/86","id":105897124,"number":86,"title":"Fix to remove Visual C++ warning C4319 from SmallBitVector.h","user":{"login":"richardlford","id":6829071,"avatar_url":"https://avatars.githubusercontent.com/u/6829071?v=3","gravatar_id":"","url":"https://api.github.com/users/richardlford","html_url":"https://github.com/richardlford","followers_url":"https://api.github.com/users/richardlford/followers","following_url":"https://api.github.com/users/richardlford/following{/other_user}","gists_url":"https://api.github.com/users/richardlford/gists{/gist_id}","starred_url":"https://api.github.com/users/richardlford/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/richardlford/subscriptions","organizations_url":"https://api.github.com/users/richardlford/orgs","repos_url":"https://api.github.com/users/richardlford/repos","events_url":"https://api.github.com/users/richardlford/events{/privacy}","received_events_url":"https://api.github.com/users/richardlford/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":3,"created_at":"2015-09-10T21:03:32Z","updated_at":"2015-09-10T21:48:12Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/Microsoft/llvm/pulls/86","html_url":"https://github.com/Microsoft/llvm/pull/86","diff_url":"https://github.com/Microsoft/llvm/pull/86.diff","patch_url":"https://github.com/Microsoft/llvm/pull/86.patch"},"body":"Closes #85.\r\n\r\nFile llvm/include/llvm/ADT/SmallBitVector.h has the following code:\r\n\r\n```\r\n  template<bool AddBits, bool InvertMask>\r\n  void applyMask(const uint32_t *Mask, unsigned MaskWords) {\r\n    if (NumBaseBits == 64 && MaskWords >= 2) {\r\n      uint64_t M = Mask[0] | (uint64_t(Mask[1]) << 32);\r\n      if (InvertMask) M = ~M;\r\n      if (AddBits) setSmallBits(getSmallBits() | M);\r\n      else         setSmallBits(getSmallBits() & ~M);\r\n    } else {\r\n      uint32_t M = Mask[0];\r\n      if (InvertMask) M = ~M;\r\n      if (AddBits) setSmallBits(getSmallBits() | M);\r\n      else         setSmallBits(getSmallBits() & ~M);\r\n    }\r\n  }\r\n```\r\n\r\nWhen compiled for a 64 bit target the Microsoft C++ compiler emits\r\na warning:\r\n\r\n```\r\n.../SmallBitVector.h(565): warning C4319: '~': zero extending 'uint32_t' to 'uintptr_t' of greater size\r\n```\r\n\r\nThe line referred to is the one containing the last occurrence of \"~M\" in the method.\r\n\r\nThe correct way of avoiding this method depends on the desired semantics of the\r\napplyMask method. This applyMask method is in the SmallBitVector class which\r\nis a specialization of the BitVectorClass. SmallBitVector is supposed to have the same\r\nsemantics as BitVectorClass but be more efficient for small bitvectors that will fit\r\nwithin a pointer-sized word.\r\n\r\nIf compiling the above for a 32 bit target the warning is not given because then\r\nuintptr_t and uint32_t are the same size.\r\n\r\nIf compiling for a 64 bit target the code giving the warning can only be executed\r\nif the number of mask words given is 1, because in this case NumBaseBits will\r\nbe 64. Since the mask words are 32 bits, this is a case where the mask\r\nwords do not cover the bitvector.\r\n\r\nSo what is the correct semantics when the mask does not cover the bitvector?\r\n\r\nOne possible interpretation would be that the mask is zero-extended to\r\nthe length of the bitvector before being applied. However a reading of the\r\nsource code of BitVector shows that this is not the case. In the BitVector\r\nimplementation the applyMask operation only affects the part of the\r\nBitVector that is covered by the mask. The remainder of the BitVector\r\nis unaffected. [In current LLVM usage the mask in fact always does\r\ncover the BitVector, except in the bitvector unit tests].\r\n\r\nSo the correct fix is to replace the offending line with\r\n\r\n```\r\n      else         setSmallBits(getSmallBits() & ~(uintptr_t )M);\r\n```\r\n\r\nThis zero-extends M before the \"~\" operation occurs, so the\r\nhigher-order bits become 1s which then have no effect when \"&\"\r\nis applied. So the mask does not affect the result where the\r\nmask is not covering.\r\n\r\nAn alternate fix I had considered but that would not be correct would be\r\nto change the declaration of the second M to\r\n\r\n```\r\n      uintptr_t M = (uintptr_t )Mask[0];\r\n```\r\n\r\nThis would give the wrong answer if InvertMask is true, as then\r\nbits not covered by the mask would be affected."},"comment":{"url":"https://api.github.com/repos/Microsoft/llvm/issues/comments/139388389","html_url":"https://github.com/Microsoft/llvm/pull/86#issuecomment-139388389","issue_url":"https://api.github.com/repos/Microsoft/llvm/issues/86","id":139388389,"user":{"login":"AndyAyersMS","id":10121823,"avatar_url":"https://avatars.githubusercontent.com/u/10121823?v=3","gravatar_id":"","url":"https://api.github.com/users/AndyAyersMS","html_url":"https://github.com/AndyAyersMS","followers_url":"https://api.github.com/users/AndyAyersMS/followers","following_url":"https://api.github.com/users/AndyAyersMS/following{/other_user}","gists_url":"https://api.github.com/users/AndyAyersMS/gists{/gist_id}","starred_url":"https://api.github.com/users/AndyAyersMS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AndyAyersMS/subscriptions","organizations_url":"https://api.github.com/users/AndyAyersMS/orgs","repos_url":"https://api.github.com/users/AndyAyersMS/repos","events_url":"https://api.github.com/users/AndyAyersMS/events{/privacy}","received_events_url":"https://api.github.com/users/AndyAyersMS/received_events","type":"User","site_admin":false},"created_at":"2015-09-10T21:48:12Z","updated_at":"2015-09-10T21:48:12Z","body":"This looks reasonable, but given that it's not LLILC specific, I'd really prefer that we fix it in a branch off master, upstream that, and then get the approved fix into MS via master.\r\n\r\nI realize that can take a while. If you want to commit this into MS to unblock folks in the meantime, please go ahead, but follow up with the upstreaming and be prepared to sort out any merge conflict that might arise when the change comes back in again via master."}}