Re: r314235 - Allow IUnknown/IInterface types to come from extern C++

[Friedman, Eli via cfe-commits]

On 9/26/2017 11:55 AM, Erich Keane via cfe-commits wrote:

Author: erichkeane
Date: Tue Sep 26 11:55:16 2017
New Revision: 314235

URL: http://llvm.org/viewvc/llvm-project?rev=314235&view=rev
Log:
Allow IUnknown/IInterface types to come from extern C++

It was brought up in response to my last implementation for
this struct-as-interface features that at least 1 header in
the MS SDK uses "extern C++" around an IUnknown declaration.

The previous implementation demanded that this type exist
in the TranslationUnit DeclContext.  This small change simply
also allows in the situation where we're extern "C++".

Modified:
 cfe/trunk/lib/AST/DeclCXX.cpp
 cfe/trunk/test/SemaCXX/ms-iunknown.cpp

Modified: cfe/trunk/lib/AST/DeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/DeclCXX.cpp?rev=314235&r1=314234&r2=314235&view=diff
==
--- cfe/trunk/lib/AST/DeclCXX.cpp (original)
+++ cfe/trunk/lib/AST/DeclCXX.cpp Tue Sep 26 11:55:16 2017
@@ -1491,7 +1491,8 @@ bool CXXRecordDecl::isInterfaceLike() co
  
// Check "Special" types.

const auto *Uuid = getAttr();
-  if (Uuid && isStruct() && getDeclContext()->isTranslationUnit() &&
+  if (Uuid && isStruct() && (getDeclContext()->isTranslationUnit() ||
+ getDeclContext()->isExternCXXContext()) &&


Do you need to check that the "extern C++" is actually a direct child of 
the translation unit?  Consider, e.g. `namespace X { extern "C++"`...


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: Patch bug 27628 (clang-tidy should exit with a failure code when there are errors)

[Friedman, Eli via cfe-commits]

When you're submitting a patch, please make sure you're sending it to 
the right list, and the "subject" line actually describes what the patch 
changes; otherwise, reviewers won't notice the patch.  Optionally, you 
can submit a patch using Phabricator, a tool which which makes patch 
reviews a little easier.  See 
http://llvm.org/docs/DeveloperPolicy.html#code-reviews


-Eli

On 9/28/2017 10:22 AM, Antoni Boucher via cfe-commits wrote:

Any news on this?


Aaand the patch itself...

-K

On 9/8/2017 10:32 AM, Krzysztof Parzyszek via cfe-commits wrote:

This should to to cfe-commits. Redirecting.

-Krzysztof

On 9/8/2017 10:25 AM, Antoni Boucher via llvm-commits wrote:

Hello.
I've fixed the bug 27628:
https://bugs.llvm.org/show_bug.cgi?id=27628

I attached the patch.

Thanks.


___
llvm-commits mailing list
llvm-commits at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits





--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, 
hosted by The Linux Foundation

-- next part --
An embedded and charset-unspecified text was scrubbed...
Name: clang-tidy-error-code.diff
URL: 


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r314262 - Emit section information for extern variables.

[Friedman, Eli via cfe-commits]

Oh, that's easy to explain; sorry, I didn't think of it when I was 
reviewing.


DefinitionKind has three possible values: DeclarationOnly, 
TentativeDefinition, and Definition.  (Tentative definitions are C 
weirdness that allows you to write "int x; int x = 10;".)


For the purpose of this warning, a TentativeDefinition should count as a 
definition.


-Eli

On 10/4/2017 1:38 PM, Andrews, Elizabeth wrote:


Hello,

I just spoke to Erich. The warning isn’t supposed to be emitted when 
the section attribute is specified on a definition. I’m not sure why 
struct r_debug _r_debug __attribute__((nocommon, 
section(".r_debug"))); failed the isThisDeclarationADefiniton check. I 
need to look into it.


Thanks,

Elizabeth

*From:*tha...@google.com [mailto:tha...@google.com] *On Behalf Of 
*Nico Weber

*Sent:* Wednesday, October 4, 2017 4:29 PM
*To:* Keane, Erich 
*Cc:* Andrews, Elizabeth ; Friedman, Eli 
; cfe-commits 

*Subject:* Re: r314262 - Emit section information for extern variables.

All I know about this code that it used to build (and still builds 
with gcc) and now it doesn't, sorry :-) What that code does seems 
somewhat questionable, but also somewhat useful, and it feels like the 
behavior change from this change here for that code might have been 
unintentional.


Your suggestion sounds reasonable to me.

On Wed, Oct 4, 2017 at 4:18 PM, Keane, Erich > wrote:


I saw that… I don’t see where it prevents the same change from
being made in link.h.

As I mentioned, there is a solution to make this Warning less
aggressive (in the lib/Sema/SemaDecl.cpp changes, add a condition
that Old->isUsed() before the warning.  I’m wondering if that
solves your issue.

It would result in

extern struct r_debug _r_debug;
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));

Compiling, but :

extern struct r_debug _r_debug;
r_debug = something();
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));

NOT compiling (which is almost definitely a bug).

*From:*tha...@google.com
[mailto:tha...@google.com
] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 1:14 PM
*To:* Keane, Erich mailto:erich.ke...@intel.com>>
*Cc:* Andrews, Elizabeth mailto:elizabeth.andr...@intel.com>>; Friedman, Eli
mailto:efrie...@codeaurora.org>>;
cfe-commits mailto:cfe-commits@lists.llvm.org>>


*Subject:* Re: r314262 - Emit section information for extern
variables.

For why, here's the comment from the code I linked to:

/*

 * GDB looks for this symbol name when it cannot find
PT_DYNAMIC->DT_DEBUG.

 * We don't have a PT_DYNAMIC, so it will find this.  Now all we
have to do

 * is arrange for this space to be filled in with the dynamic linker's

 * _r_debug contents after they're initialized.  That way,
attaching GDB to

 * this process or examining its core file will find the PIE we
loaded, the

 * dynamic linker, and all the shared libraries, making debugging
pleasant.

 */

On Wed, Oct 4, 2017 at 4:11 PM, Keane, Erich
mailto:erich.ke...@intel.com>> wrote:

I’ve added Elizabeth, who is the original patch author. 
Hopefully she can help out here. Additionally, Eli did the
original review, so hopefully he can chime in as well.


I believe the necessity for this warning came out of the
discussion on fixing the section behavior.  The issue here is
that redeclaring with a different ‘section’ can cause some
pretty nasty issues, since it isn’t terribly clear what
happens if the variable is used in the meantime.

There is 1 change that I can think of that Elizabeth and I
discussed, which was to only warn in the case where there was
a USAGE between these two redeclarations.  I’m not sure that
will allow na_cl to compile, however it is my belief that if
there IS a usage between link.h:64 and nacl_bootstrap.c:434,
that this is a bug in nacl that is simply being uncovered
thanks to this new warning.

Is there a good/reasonable reason the source of nacl wants to
redeclare this with a different ‘section’?

*From:*tha...@google.com
[mailto:tha...@google.com
] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 12:59 PM
*To:* Keane, Erich mailto:erich.ke...@intel.com>>
*Cc:* cfe-commits mailto:cfe-commits@lists.llvm.org>>
*Subject:* Re: r314262 - Emit section information for extern
variables.

Hi Erich,

this breaks existing code. NaCl does this:

#include 

struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));

(There's a lengthy-ish comment for

Re: r314262 - Emit section information for extern variables.

[Friedman, Eli via cfe-commits]

On 10/4/2017 1:48 PM, Keane, Erich wrote:


Ah, cool!  I didn’t realize that, and that is actually exactly what 
Elizabeth is looking into now.


Elizabeth, if you send me a diff, I’ll commit it as 
review-after-commit if Eli is alright with this.  It should be 
something like changing:


+      if (VD->isThisDeclarationADefinition() != VarDecl::Definition) {

To

+      if (VD->isThisDeclarationADefinition() != VarDecl::Definition 
&& VD->isThsDeclarationADefinition() != VarDecl::TentativeDefinition) {


Right?



I'd probably just write it "if (VD->isThisDeclarationADefinition() == 
VarDecl::DeclarationOnly)", but yes.  (And don't forget a testcase.)


-Eli


*From:*Friedman, Eli [mailto:efrie...@codeaurora.org]
*Sent:* Wednesday, October 4, 2017 1:46 PM
*To:* Andrews, Elizabeth ; Nico Weber 

*Cc:* cfe-commits ; Keane, Erich 


*Subject:* Re: r314262 - Emit section information for extern variables.

Oh, that's easy to explain; sorry, I didn't think of it when I was 
reviewing.


DefinitionKind has three possible values: DeclarationOnly, 
TentativeDefinition, and Definition.  (Tentative definitions are C 
weirdness that allows you to write "int x; int x = 10;".)


For the purpose of this warning, a TentativeDefinition should count as 
a definition.


-Eli

On 10/4/2017 1:38 PM, Andrews, Elizabeth wrote:

Hello,

I just spoke to Erich. The warning isn’t supposed to be emitted
when the section attribute is specified on a definition. I’m not
sure why struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug"))); failed the isThisDeclarationADefiniton
check. I need to look into it.

Thanks,

Elizabeth

*From:*tha...@google.com 
[mailto:tha...@google.com] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 4:29 PM
*To:* Keane, Erich 

*Cc:* Andrews, Elizabeth 
; Friedman, Eli
 ;
cfe-commits 

*Subject:* Re: r314262 - Emit section information for extern
variables.

All I know about this code that it used to build (and still builds
with gcc) and now it doesn't, sorry :-) What that code does seems
somewhat questionable, but also somewhat useful, and it feels like
the behavior change from this change here for that code might have
been unintentional.

Your suggestion sounds reasonable to me.

On Wed, Oct 4, 2017 at 4:18 PM, Keane, Erich
mailto:erich.ke...@intel.com>> wrote:

I saw that… I don’t see where it prevents the same change from
being made in link.h.

As I mentioned, there is a solution to make this Warning less
aggressive (in the lib/Sema/SemaDecl.cpp changes, add a
condition that Old->isUsed() before the warning.  I’m
wondering if that solves your issue.

It would result in

extern struct r_debug _r_debug;
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));

Compiling, but :

extern struct r_debug _r_debug;
r_debug = something();
struct r_debug _r_debug __attribute__((nocommon,
section(".r_debug")));

NOT compiling (which is almost definitely a bug).

*From:*tha...@google.com
[mailto:tha...@google.com
] *On Behalf Of *Nico Weber
*Sent:* Wednesday, October 4, 2017 1:14 PM
*To:* Keane, Erich mailto:erich.ke...@intel.com>>
*Cc:* Andrews, Elizabeth mailto:elizabeth.andr...@intel.com>>; Friedman, Eli
mailto:efrie...@codeaurora.org>>;
cfe-commits mailto:cfe-commits@lists.llvm.org>>


*Subject:* Re: r314262 - Emit section information for extern
variables.

For why, here's the comment from the code I linked to:

/*

 * GDB looks for this symbol name when it cannot find
PT_DYNAMIC->DT_DEBUG.

 * We don't have a PT_DYNAMIC, so it will find this.  Now all
we have to do

 * is arrange for this space to be filled in with the dynamic
linker's

 * _r_debug contents after they're initialized.  That way,
attaching GDB to

 * this process or examining its core file will find the PIE
we loaded, the

 * dynamic linker, and all the shared libraries, making
debugging pleasant.

 */

On Wed, Oct 4, 2017 at 4:11 PM, Keane, Erich
mailto:erich.ke...@intel.com>> wrote:

I’ve added Elizabeth, who is the original patch author.
Hopefully she can help out here.  Additionally, Eli did
the original review, so hopefully he can chime in as well.


I believe the necessity for this warning came out of the
discussion on fixing the section behavior. The issue here
is that redecla

Re: Patch bug 27628 (clang-tidy should exit with a failure code when there are errors)

[Friedman, Eli via cfe-commits]

Sometimes people just lose track; "pinging" is normal (see 
http://llvm.org/docs/DeveloperPolicy.html#code-reviews).  And sometimes 
it's helpful to CC reviewers in the area; adding Alexander Kornienko.


-Eli

On 10/17/2017 12:03 PM, Antoni Boucher wrote:

Since the patch was redirected to the right mailing list and the subject
was edited, I believe everything is okay, now?
Is there any news?
Thanks.

On Thu, Sep 28, 2017 at 06:56:57PM +, Friedman, Eli wrote:

When you're submitting a patch, please make sure you're sending it to 
the right list, and the "subject" line actually describes what the 
patch changes; otherwise, reviewers won't notice the patch.  
Optionally, you can submit a patch using Phabricator, a tool which 
which makes patch reviews a little easier.  See 
http://llvm.org/docs/DeveloperPolicy.html#code-reviews


-Eli

On 9/28/2017 10:22 AM, Antoni Boucher via cfe-commits wrote:


Any news on this?


Aaand the patch itself...

-K

On 9/8/2017 10:32 AM, Krzysztof Parzyszek via cfe-commits wrote:


This should to to cfe-commits. Redirecting.

-Krzysztof

On 9/8/2017 10:25 AM, Antoni Boucher via llvm-commits wrote:

Hello. I've fixed the bug 27628: 
https://bugs.llvm.org/show_bug.cgi?id=27628


I attached the patch.

Thanks.


___ llvm-commits 
mailing list llvm-commits at lists.llvm.org 
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits





-- Qualcomm Innovation Center, Inc. is a member of Code Aurora 
Forum, hosted by The Linux Foundation -- next part 
-- An embedded and charset-unspecified text was 
scrubbed... Name: clang-tidy-error-code.diff URL: 



___ cfe-commits mailing 
list cfe-commits@lists.llvm.org 
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



-- Employee of Qualcomm Innovation Center, Inc. Qualcomm Innovation 
Center, Inc. is a member of Code Aurora Forum, a Linux Foundation 
Collaborative Project




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r316046 - Basic: fix __{,U}INTPTR_TYPE__ on ARM

[Friedman, Eli via cfe-commits]

On 10/17/2017 5:00 PM, Saleem Abdulrasool via cfe-commits wrote:

Author: compnerd
Date: Tue Oct 17 17:00:50 2017
New Revision: 316046

URL: http://llvm.org/viewvc/llvm-project?rev=316046&view=rev
Log:
Basic: fix __{,U}INTPTR_TYPE__ on ARM

Darwin and OpenBSD are the only platforms which use `long int` for
`__INTPTR_TYPE__`.  The other platforms use `int` in 32-bit, and `long
int` on 64-bit (except for VMS and Windows which are LLP64).  Adjust the
type definitions to match the platform definitions.  We now generate the
same definition as GCC on all the targets.

Modified:
 cfe/trunk/lib/Basic/Targets/ARM.cpp
 cfe/trunk/test/Preprocessor/init.c
 cfe/trunk/test/Preprocessor/stdint.c

Modified: cfe/trunk/lib/Basic/Targets/ARM.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets/ARM.cpp?rev=316046&r1=316045&r2=316046&view=diff
==
--- cfe/trunk/lib/Basic/Targets/ARM.cpp (original)
+++ cfe/trunk/lib/Basic/Targets/ARM.cpp Tue Oct 17 17:00:50 2017
@@ -236,6 +236,10 @@ ARMTargetInfo::ARMTargetInfo(const llvm:
  break;
}
  
+  IntPtrType = (Triple.isOSDarwin() || Triple.getOS() == llvm::Triple::OpenBSD)

+   ? SignedLong
+   : SignedInt;
+
// Cache arch related info.
setArchInfo();
  
@@ -923,7 +927,6 @@ WindowsARMTargetInfo::WindowsARMTargetIn

 const TargetOptions &Opts)
  : WindowsTargetInfo(Triple, Opts), Triple(Triple) {
SizeType = UnsignedInt;
-  IntPtrType = SignedInt;
  }
  


Generally, PtrDiffType, IntPtrType, and SizeType are all the same 
(ignoring signedness).  Please change the code to set all of these 
together.  With the code scattered like this, it isn't obvious your 
changes are consistent.  (Actually, I'm pretty sure they aren't consistent.)


Also, in the future, please don't commit any change which affects ABI 
definitions without pre-commit review; this is a tricky area, even if a 
change seems simple.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r338239 - [mips64][clang] Provide the signext attribute for i32 return values

[Friedman, Eli via cfe-commits]

On 7/30/2018 3:44 AM, Stefan Maksimovic via cfe-commits wrote:

Author: smaksimovic
Date: Mon Jul 30 03:44:46 2018
New Revision: 338239

URL: http://llvm.org/viewvc/llvm-project?rev=338239&view=rev
Log:
[mips64][clang] Provide the signext attribute for i32 return values

Additional info: see r338019.

Differential Revision: https://reviews.llvm.org/D49289

Modified:
 cfe/trunk/lib/CodeGen/TargetInfo.cpp


I'd like to see some test coverage for this.

-El

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r344199 - Add a flag to remap manglings when reading profile data information.

[Friedman, Eli via cfe-commits]

On 10/10/2018 4:13 PM, Richard Smith via cfe-commits wrote:

Author: rsmith
Date: Wed Oct 10 16:13:35 2018
New Revision: 344199

URL: http://llvm.org/viewvc/llvm-project?rev=344199&view=rev
Log:
Add a flag to remap manglings when reading profile data information.

This can be used to preserve profiling information across codebase
changes that have widespread impact on mangled names, but across which
most profiling data should still be usable. For example, when switching
from libstdc++ to libc++, or from the old libstdc++ ABI to the new ABI,
or even from a 32-bit to a 64-bit build.

The user can provide a remapping file specifying parts of mangled names
that should be treated as equivalent (eg, std::__1 should be treated as
equivalent to std::__cxx11), and profile data will be treated as
applying to a particular function if its name is equivalent to the name
of a function in the profile data under the provided equivalences. See
the documentation change for a description of how this is configured.

Remapping is supported for both sample-based profiling and instruction
profiling. We do not support remapping indirect branch target
information, but all other profile data should be remapped
appropriately.

Support is only added for the new pass manager. If someone wants to also
add support for this for the old pass manager, doing so should be
straightforward.


Should the documentation mention this limitation?

-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r345660 - [clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

[Friedman, Eli via cfe-commits]

On 10/30/2018 2:58 PM, Roman Lebedev via cfe-commits wrote:

Author: lebedevri
Date: Tue Oct 30 14:58:56 2018
New Revision: 345660

URL: http://llvm.org/viewvc/llvm-project?rev=345660&view=rev
Log:
[clang][ubsan] Implicit Conversion Sanitizer - integer sign change - clang part

This is the second half of Implicit Integer Conversion Sanitizer.
It completes the first half, and finally makes the sanitizer
fully functional! Only the bitfield handling is missing.


This is causing failures on the polly-aosp buildbot 
(http://lab.llvm.org:8011/builders/aosp-O3-polly-before-vectorizer-unprofitable/builds/690/steps/build-aosp/logs/stdio 
).  I haven't tried to reduce the issue yet; let me know if you need help.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r312750 - [Sema] -Wtautological-compare: handle comparison of unsigned with 0S.

[Friedman, Eli via cfe-commits]

On 9/8/2017 2:18 PM, Hans Wennborg via cfe-commits wrote:

On Fri, Sep 8, 2017 at 2:09 PM, Roman Lebedev  wrote:


Interesting. My first thought was to explicitly specify enum as signed:

enum MediaDeviceType : signed int {
MEDIA_DEVICE_TYPE_AUDIO_INPUT = 0,
MEDIA_DEVICE_TYPE_VIDEO_INPUT,
MEDIA_DEVICE_TYPE_AUDIO_OUTPUT,
NUM_MEDIA_DEVICE_TYPES,
};

inline bool IsValidMediaDeviceType(MediaDeviceType type) {
return type >= 0U && type < NUM_MEDIA_DEVICE_TYPES;
}

But it still seem to warn, and *that* sounds like a bug.
Please open a new bugreport.

I'm reporting it here :-)


As for different default signedness, i'm not sure what is there to do. Does
not sound like a problem for this diagnostic to intentionally avoid to
be honest.

I think it is a problem for this warning. If a user sees the warning
and removes the "type >= 0" check, thinking that it was unnecessary
because the compiler told them so, they have now introduced a bug in
the code.


Even if you declare the enum type as signed, it still isn't allowed to 
contain a negative number; that's undefined behavior.  You can check for 
this using ubsan's -fsanitize=enum.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r313907 - Suppress Wsign-conversion for enums with matching underlying type

[Friedman, Eli via cfe-commits]

On 9/21/2017 12:58 PM, Erich Keane via cfe-commits wrote:

Author: erichkeane
Date: Thu Sep 21 12:58:55 2017
New Revision: 313907

URL: http://llvm.org/viewvc/llvm-project?rev=313907&view=rev
Log:
Suppress Wsign-conversion for enums with matching underlying type

As reported here: https://bugs.llvm.org/show_bug.cgi?id=34692

A non-defined enum with a backing type was always defaulting to
being treated as a signed type. IN the case where it IS defined,
the signed-ness of the actual items is used.

This patch uses the underlying type's signed-ness in the non-defined
case to test signed-comparision.

Differential Revision: https://reviews.llvm.org/D38145

Modified:
 cfe/trunk/lib/Sema/SemaChecking.cpp


Missing testcase?

-El

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45109: Remove -cc1 option "-backend-option"

[Friedman, Eli via cfe-commits]

The test wasn't checking anything useful; the clang driver never passes 
"-arm-long-calls" to clang -cc1.  See https://reviews.llvm.org/rL241565 .


-Eli

On 4/12/2018 3:55 PM, George Karpenkov wrote:

Hi Eli,

The commit makes sense, but I’m not sure about your change to the 
`test/Driver/apple-kext-mkernel.c 
` 
file:

instead of changing the run lines to use -mllvm you have removed them.
Was there a reason for the removal?

Regards,
George

On Apr 12, 2018, at 3:25 PM, Eli Friedman via Phabricator via 
llvm-commits > wrote:


This revision was automatically updated to reflect the committed changes.
Closed by commit rL329965: Remove -cc1 option 
"-backend-option". (authored by efriedma, committed by ).

Herald added a subscriber: llvm-commits.

Changed prior to commit:
https://reviews.llvm.org/D45109?vs=140484&id=142277#toc

Repository:
 rL LLVM

https://reviews.llvm.org/D45109

Files:
 cfe/trunk/include/clang/Driver/CC1Options.td
 cfe/trunk/include/clang/Frontend/CodeGenOptions.h
 cfe/trunk/lib/CodeGen/BackendUtil.cpp
 cfe/trunk/lib/Driver/ToolChains/Clang.cpp
 cfe/trunk/lib/Frontend/CompilerInvocation.cpp
 cfe/trunk/test/CodeGen/thinlto-backend-option.ll
 cfe/trunk/test/CodeGenCUDA/link-device-bitcode.cu
 cfe/trunk/test/Driver/aarch64-fix-cortex-a53-835769.c
 cfe/trunk/test/Driver/apple-kext-mkernel.c
 cfe/trunk/test/Driver/arm-restrict-it.c
 cfe/trunk/test/Driver/debug-options.c
 cfe/trunk/test/Driver/mglobal-merge.c
 cfe/trunk/test/Driver/woa-restrict-it.c
 cfe/trunk/test/Frontend/backend-option.c

___
llvm-commits mailing list
llvm-comm...@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r331370 - Add -foutline option to enable the MachineOutliner in AArch64

[Friedman, Eli via cfe-commits]

On 5/2/2018 9:42 AM, Jessica Paquette via cfe-commits wrote:

Author: paquette
Date: Wed May  2 09:42:51 2018
New Revision: 331370

URL: http://llvm.org/viewvc/llvm-project?rev=331370&view=rev
Log:
Add -foutline option to enable the MachineOutliner in AArch64


It probably makes sense to also have a -fno-outline option.

-Eli



Since we've been working on productizing the MachineOutliner in AArch64, it
makes sense to provide a more user-friendly way to enable it.

This allows users of AArch64 to enable the outliner using -foutline instead
of -mllvm -enable-machine-outliner. Other, less mature implementations (e.g,
x86-64) can still enable the pass using the -mllvm option.

Also add a test to make sure it works.

Added:
 cfe/trunk/test/Driver/aarch64-outliner.c
Modified:
 cfe/trunk/include/clang/Driver/Options.td
 cfe/trunk/lib/Driver/ToolChains/Clang.cpp

Modified: cfe/trunk/include/clang/Driver/Options.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Driver/Options.td?rev=331370&r1=331369&r2=331370&view=diff
==
--- cfe/trunk/include/clang/Driver/Options.td (original)
+++ cfe/trunk/include/clang/Driver/Options.td Wed May  2 09:42:51 2018
@@ -1317,6 +1317,8 @@ def fno_exceptions : Flag<["-"], "fno-ex
  def fno_gnu_keywords : Flag<["-"], "fno-gnu-keywords">, Group, 
Flags<[CC1Option]>;
  def fno_inline_functions : Flag<["-"], "fno-inline-functions">, 
Group, Flags<[CC1Option]>;
  def fno_inline : Flag<["-"], "fno-inline">, Group, 
Flags<[CC1Option]>;
+def foutline : Flag<["-"], "foutline">, Group, 
Flags<[CC1Option]>,
+HelpText<"Enable function outlining (AArch64 only)">;
  def fno_experimental_isel : Flag<["-"], "fno-experimental-isel">, 
Group,
HelpText<"Disables the experimental global instruction selector">;
  def fno_experimental_new_pass_manager : Flag<["-"], 
"fno-experimental-new-pass-manager">,

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=331370&r1=331369&r2=331370&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Wed May  2 09:42:51 2018
@@ -1484,6 +1484,11 @@ void Clang::AddAArch64TargetArgs(const A
  else
CmdArgs.push_back("-aarch64-enable-global-merge=true");
}
+
+  if (Arg *A = Args.getLastArg(options::OPT_foutline)) {
+CmdArgs.push_back("-mllvm");
+CmdArgs.push_back("-enable-machine-outliner");
+  }
  }
  
  void Clang::AddMIPSTargetArgs(const ArgList &Args,


Added: cfe/trunk/test/Driver/aarch64-outliner.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/aarch64-outliner.c?rev=331370&view=auto
==
--- cfe/trunk/test/Driver/aarch64-outliner.c (added)
+++ cfe/trunk/test/Driver/aarch64-outliner.c Wed May  2 09:42:51 2018
@@ -0,0 +1,4 @@
+// REQUIRES: aarch64-registered-target
+
+// RUN: %clang -target aarch64 -foutline -S %s -### 2>&1 | FileCheck %s
+// CHECK: "-mllvm" "-enable-machine-outliner"


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r315126 - Driver: hoist the `wchar_t` handling to the driver

[Friedman, Eli via cfe-commits]

On 10/6/2017 4:09 PM, Saleem Abdulrasool via cfe-commits wrote:

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=315126&r1=315125&r2=315126&view=diff
==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Fri Oct  6 16:09:55 2017
@@ -2601,6 +2601,33 @@ static void RenderModulesOptions(Compila
Args.AddLastArg(CmdArgs, 
options::OPT_fmodules_disable_diagnostic_validation);
  }
  
+static void RenderCharacterOptions(const ArgList &Args, const llvm::Triple &T,

+   ArgStringList &CmdArgs) {
+  // -fsigned-char is default.
+  if (const Arg *A = Args.getLastArg(options::OPT_fsigned_char,
+ options::OPT_fno_signed_char,
+ options::OPT_funsigned_char,
+ options::OPT_fno_unsigned_char)) {
+if (A->getOption().matches(options::OPT_funsigned_char) ||
+A->getOption().matches(options::OPT_fno_signed_char)) {
+  CmdArgs.push_back("-fno-signed-char");
+}
+  } else if (!isSignedCharDefault(T)) {
+CmdArgs.push_back("-fno-signed-char");
+  }
+
+  if (const Arg *A = Args.getLastArg(options::OPT_fshort_wchar,
+ options::OPT_fno_short_wchar)) {
+if (A->getOption().matches(options::OPT_fshort_wchar)) {
+  CmdArgs.push_back("-fwchar-type=short");
+  CmdArgs.push_back("-fno-signed-wchar");
+} else {
+  CmdArgs.push_back("-fwchar-type=int");
+  CmdArgs.push_back("-fsigned-wchar");
+}
+  }
+}


It looks like this changes the behavior of "-fno-short-wchar". 
Specifically, on targets where the default wchar_t type is "unsigned 
int" (like ARM AAPCS), "-fno-short-wchar" used to be a no-op, but now it 
changes the sign of wchar_t.  Why are we overriding the default here?


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r315126 - Driver: hoist the `wchar_t` handling to the driver

[Friedman, Eli via cfe-commits]

On 10/25/2017 11:54 AM, Reid Kleckner wrote:
On Wed, Oct 25, 2017 at 10:56 AM, Friedman, Eli 
mailto:efrie...@codeaurora.org>> wrote:


On 10/6/2017 4:09 PM, Saleem Abdulrasool via cfe-commits wrote:

Modified: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
URL:

http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Clang.cpp?rev=315126&r1=315125&r2=315126&view=diff



==
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp Fri Oct  6
16:09:55 2017
@@ -2601,6 +2601,33 @@ static void RenderModulesOptions(Compila
    Args.AddLastArg(CmdArgs,
options::OPT_fmodules_disable_diagnostic_validation);
  }
  +static void RenderCharacterOptions(const ArgList &Args,
const llvm::Triple &T,
+                                   ArgStringList &CmdArgs) {
+  // -fsigned-char is default.
+  if (const Arg *A = Args.getLastArg(options::OPT_fsigned_char,
+  options::OPT_fno_signed_char,
+  options::OPT_funsigned_char,
+  options::OPT_fno_unsigned_char)) {
+    if (A->getOption().matches(options::OPT_funsigned_char) ||
+        A->getOption().matches(options::OPT_fno_signed_char)) {
+      CmdArgs.push_back("-fno-signed-char");
+    }
+  } else if (!isSignedCharDefault(T)) {
+    CmdArgs.push_back("-fno-signed-char");
+  }
+
+  if (const Arg *A = Args.getLastArg(options::OPT_fshort_wchar,
+  options::OPT_fno_short_wchar)) {
+    if (A->getOption().matches(options::OPT_fshort_wchar)) {
+      CmdArgs.push_back("-fwchar-type=short");
+      CmdArgs.push_back("-fno-signed-wchar");
+    } else {
+      CmdArgs.push_back("-fwchar-type=int");
+      CmdArgs.push_back("-fsigned-wchar");
+    }
+  }
+}


It looks like this changes the behavior of "-fno-short-wchar".
Specifically, on targets where the default wchar_t type is
"unsigned int" (like ARM AAPCS), "-fno-short-wchar" used to be a
no-op, but now it changes the sign of wchar_t.  Why are we
overriding the default here?


Overriding the default was more or less the intention, but it was for 
bitwidth, not signedness. The idea was that if you pass 
-fno-short-wchar, then you want the 4 byte one.


Users probably don't care about signedness when using 4 byte wchars, 
so we could probably remove the -fsigned-wchar flag from the else 
block and take the original sign of wchar_t when overriding the width 
in Basic. They probably want an 'unsigned short' when -fshort-wchar is 
passed, though.


Maybe we should just ignore -fno-short-wchar, instead?  I think that's 
what gcc and released versions of clang do (that means -fno-short-wchar 
doesn't do anything for Windows targets, but that's probably fine).


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r315126 - Driver: hoist the `wchar_t` handling to the driver

[Friedman, Eli via cfe-commits]

On 10/25/2017 12:49 PM, Reid Kleckner wrote:
On Wed, Oct 25, 2017 at 12:14 PM, Friedman, Eli 
mailto:efrie...@codeaurora.org>> wrote:


Maybe we should just ignore -fno-short-wchar, instead?  I think
that's what gcc and released versions of clang do (that means
-fno-short-wchar doesn't do anything for Windows targets, but
that's probably fine).


No, the intention of this change was to make -fno-short-wchar do 
something on Windows targets.


Really?  I don't see any mention of that in either the patch itself, or 
in the discussion of it.  It's fine, I guess, but please add a note to 
the release notes.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r290146 - Fix completely bogus types for some builtins:

[Friedman, Eli via cfe-commits]

On 12/19/2016 3:59 PM, Richard Smith via cfe-commits wrote:

Author: rsmith
Date: Mon Dec 19 17:59:34 2016
New Revision: 290146

URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/vfprintf-valid-redecl.c?rev=290146&r1=290145&r2=290146&view=diff
==
--- cfe/trunk/test/Sema/vfprintf-valid-redecl.c (original)
+++ cfe/trunk/test/Sema/vfprintf-valid-redecl.c Mon Dec 19 17:59:34 2016
@@ -1,16 +1,18 @@
  // RUN: %clang_cc1 %s -fsyntax-only -pedantic -verify
  // RUN: %clang_cc1 %s -fsyntax-only -pedantic -verify -DPREDECLARE
-// expected-no-diagnostics
  
  #ifdef PREDECLARE

  // PR16344
  // Clang has defined 'vfprint' in builtin list. If the following line occurs 
before any other
  // `vfprintf' in this file, and we getPreviousDecl()->getTypeSourceInfo() on 
it, then we will
  // get a null pointer since the one in builtin list doesn't has valid 
TypeSourceInfo.
-int vfprintf(void) { return 0; }
+int vfprintf(void) { return 0; } // expected-warning {{requires inclusion of the 
header }}
  #endif
  
  // PR4290

  // The following declaration is compatible with vfprintf, so we shouldn't
-// warn.
+// reject.
  int vfprintf();
+#ifndef PREDECLARE
+// expected-warning@-2 {{requires inclusion of the header }}
+#endif


We shouldn't warn here; this declaration of vfprintf() is compatible 
with the actual prototype.  (Granted, you can't call vfprintf() without 
including stdio.h, but that's a separate problem.)


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r290146 - Fix completely bogus types for some builtins:

[Friedman, Eli via cfe-commits]

On 12/20/2016 6:56 PM, Richard Smith wrote:
On 20 December 2016 at 18:14, Richard Smith <mailto:rich...@metafoo.co.uk>> wrote:


On 19 December 2016 at 17:00, Friedman, Eli via cfe-commits
mailto:cfe-commits@lists.llvm.org>>
wrote:

On 12/19/2016 3:59 PM, Richard Smith via cfe-commits wrote:

Author: rsmith
Date: Mon Dec 19 17:59:34 2016
New Revision: 290146

URL:

http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/vfprintf-valid-redecl.c?rev=290146&r1=290145&r2=290146&view=diff

<http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Sema/vfprintf-valid-redecl.c?rev=290146&r1=290145&r2=290146&view=diff>

==
--- cfe/trunk/test/Sema/vfprintf-valid-redecl.c (original)
+++ cfe/trunk/test/Sema/vfprintf-valid-redecl.c Mon Dec 19
17:59:34 2016
@@ -1,16 +1,18 @@
  // RUN: %clang_cc1 %s -fsyntax-only -pedantic -verify
  // RUN: %clang_cc1 %s -fsyntax-only -pedantic -verify
-DPREDECLARE
-// expected-no-diagnostics
#ifdef PREDECLARE
  // PR16344
  // Clang has defined 'vfprint' in builtin list. If the
following line occurs before any other
  // `vfprintf' in this file, and we
getPreviousDecl()->getTypeSourceInfo() on it, then we will
  // get a null pointer since the one in builtin list
doesn't has valid TypeSourceInfo.
-int vfprintf(void) { return 0; }
+int vfprintf(void) { return 0; } // expected-warning
{{requires inclusion of the header }}
  #endif
// PR4290
  // The following declaration is compatible with
vfprintf, so we shouldn't
-// warn.
+// reject.
  int vfprintf();
+#ifndef PREDECLARE
+// expected-warning@-2 {{requires inclusion of the header
}}
+#endif


We shouldn't warn here; this declaration of vfprintf() is
compatible with the actual prototype.  (Granted, you can't
call vfprintf() without including stdio.h, but that's a
separate problem.)


I agree (but I'd note that this is a pre-existing problem for
other lib builtins taking a FILE*, particularly vfscanf). Perhaps
we could deem the builtin to have a FunctionNoProtoType type in C
if it's not variadic and depends on a type that is not yet declared?


What do you think of the attached approach? I'm a little uncomfortable 
that we no longer recognise a declaration of sigsetjmp as a builtin if 
it has the wrong return type (and issue only a warning for that case).


Alternatively, if we're OK with recognising wrongly-typed declarations 
as library builtins in this corner case, we could just remove the 
warning entirely.


The "right" approach is probably something a bit more invasive. We could 
wait, and attempt to recognize the builtin later, when the function is 
declared with a prototype or called.  Or we could try to 
"forward-declare" FILE (map it to some opaque type until we have an 
actual declaration we can use).


Short of that, it's probably okay to leave things as-is for now; better 
to have extra warnings in weird edge cases than to miss a warning in a 
case where it's important.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D24378: [CodeGen] Provide an appropriate alignment for dynamic allocas

[Friedman, Eli via cfe-commits]

It probably makes sense to say that alloca should have the same 
alignment as the default stack alignment, simply because it's hard to do 
anything useful with completely unaligned memory.  That said, the 
default stack alignment is 4 bytes on 32-bit Windows, not 16.


-Eli

On 9/9/2016 10:29 AM, Richard Smith wrote:
There's an (unconvincing to me) explanation for why gcc does this 
here: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19131


On 9 Sep 2016 10:14 am, "Eli Friedman" > wrote:


efriedma added a comment.

This is probably going to lead to someone complaining about clang
realigning the stack on 32-bit Windows; are your sure we're
choosing the right alignment there?


https://reviews.llvm.org/D24378 




--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [cfe-dev] Testcase for LLVM PR9350

[Friedman, Eli via cfe-commits]

On 1/6/2017 3:05 PM, Andrew Rogers wrote:


Hi,

This is a (tested!) patch to add checks that both pre-increment and 
pre-decrement don’t overflow, for both char and short integer types 
(since they’re both narrower than int), as per the problem fixed in 
PR9350.





r291805.

-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: r302255 - Warn that the [] spelling of uuid(...) is deprecated.

[Friedman, Eli via cfe-commits]

On 5/5/2017 10:20 AM, Nico Weber via cfe-commits wrote:
Timestamps on cfe-commits are a bit messed up. I landed that just now, 
but the timestamp is from 13 minutes in the past. Maybe the clock on 
the mail server is off by 13 min?


The timestamp on the message matches the timestamp in the svn log. But 
yes, it looks like the clock on that server is way off.


-Eli

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux 
Foundation Collaborative Project

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits