[PATCH] D158641: [AArch64][Android][DRAFT] Fix FMV ifunc resolver usage on old Android APIs.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: compiler-rt/lib/builtins/cpu_model.c:1382
+return;
+#if defined(__ANDROID__)
+  // ifunc resolvers don't have hwcaps in arguments on Android API lower

MaskRay wrote:
> enh wrote:
> > ilinpv wrote:
> > > MaskRay wrote:
> > > > I am unfamiliar with how Android ndk builds compiler-rt.
> > > > 
> > > > If `__ANDROID_API__ >= 30`, shall we use the regular Linux code path?
> > > I think that leads to shipping different compile-rt libraries depend on 
> > > ANDROID_API. If this is an option to consider than runtime check 
> > > android_get_device_api_level() < 30 can be replaced by `__ANDROID_API__ < 
> > > 30`
> > depends what you mean... in 10 years or so, yes, no-one is likely to still 
> > care about the older API levels and we can just delete this. but until 
> > then, no, there's _one_ copy of compiler-rt that everyone uses, and 
> > although _OS developers_ don't need to support anything more than a couple 
> > of years old, most app developers are targeting far lower API levels than 
> > that (to maximize the number of possible customers).
> > 
> > TL;DR: "you could add that condition to the `#if`, but no-one would use it 
> > for a decade". (and i think the comment and `if` below should make it clear 
> > enough to future archeologists when this code block can be removed :-) )
> My thought was that people build Android with a specific `__ANDROID_API__`, 
> and only systems >= this level are supported.
> ```
> #If __ANDROID_API__ < 30
> ...
> #endif
> ```
> 
> This code has a greater chance to be removed when it becomes obsoleted. The 
> argument is similar to how we find obsoleted GCC workarounds.
Yes, the NDK currently just ships the oldest supported target API level for 
compiler-rt, while the Android platform build does have access to both the 
oldest supported target API level + the most recent target API level, so that 
we can make better use of features there.

Maybe I'm missing something, but it's feeling like the NDK users won't be able 
to make great use of FMV without us either bumping the minimum level or 
shipping multiple runtimes (and then using the #ifdefs properly here).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158641/new/

https://reviews.llvm.org/D158641

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

[PATCH] D158476: [driver] Search for compatible Android runtime directories

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

This approach LGTM, assuming that https://reviews.llvm.org/D140925 lands as 
well to ensure that the triple does use `androideabi`, since it would prevent 
custom build setups from needing additional help.

> (Aside: why do we need to produce runtime libraries for different versions in 
> the first place? We want one for the minimum OS version we support, of 
> course, but then there's important additions like version 29 adding ELF TLS 
> support and version 31 adding a thread properties API used by LSAN, so 
> building libraries targeting those important versions and having them be 
> dynamically selected based on your target OS version is super useful. We 
> could build the runtime libraries for every single OS version we wanted to 
> support, but that's a lot of versions (at least 21 through 31, times four 
> architectures), which is wasteful for both build times and toolchain 
> distribution size.)

I don't think we need all the versions, but today we really only use 21 (lowest 
level), and 30 (a conservative level for the platform that supports all our 
apexes). Ideally we'd also pick up the latest platform version, and we probably 
should explicitly call out 29 for ELF TLS, so that would be ~4 supported 
levels, but maybe we can use 29 instead of 30 to cut it to ~3 for most 
architectures.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D158476/new/

https://reviews.llvm.org/D158476

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

[PATCH] D150226: [Clang] Remove ability to downgrade warning on the diagnostic for setting a non fixed enum to a value outside the range of the enumeration values

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

> They'd already have had a chance to deal with their code when this was a 
> warning-default-error without "ShowInSystemHeaders"? (or, if the yhaven't 
> picked up a new compiler often enough, and they go from "a warning we didn't 
> care about" to "warning-default-error-with-ShowInSystemHeaders" - they're 
> still better off than if it'd gone straight to hard error, some chance to 
> cleanup while disabling the warning/error before picking up a compiler 
> version that makes it a hard error)

+1 for this. Having no intermediate step for developers to find/fix the warning 
in system headers (short of recompiling Clang with different settings) seems 
really rough for very downstream folks.

One other thought I had was whether this should have a different way to 
suppress a "severe" warning (i.e. 
`-fno-really-I-know-I-should-fix-this-UB-hole` a la the now defunct flag for 
automatic initialization), because it is far too common for downstream 
developers of all sorts to encounter a warning once, see that they just don't 
care about that specific instance, and then slap on a `-Wno-foo` to their 
project forever. It almost seems like there should be a strongly-worded opt-out 
for these severe warning suppressions that might be behavior-changing (even if 
we later remove the ability for users to suppress them). I know that compiler 
developers don't like adding flags (let alone confusing ones for behaviors that 
we know should have been strict from the start), but it seems like there might 
be some tradeoff here that is worth it, considering how much other downstream 
cleanup might be needed for some compiler vendors.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150226/new/

https://reviews.llvm.org/D150226

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

[PATCH] D125142: [clang][auto-init] Deprecate -enable-trivial-auto-var-init-zero-knowing-it-will-be-removed-from-clang

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.

Thank you, Kees and reviewers, for helping to make progress here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125142/new/

https://reviews.llvm.org/D125142

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

[PATCH] D127528: [Clang] Let the linker choose shared or static libunwind unless specified

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang/lib/Driver/ToolChains/CommonArgs.cpp:1477
   if (Args.hasArg(options::OPT_shared_libgcc))
 return LibGccType::SharedLibGcc;
-  // The Android NDK only provides libunwind.a, not libunwind.so.

phosek wrote:
> mstorsjo wrote:
> > Previously, `OPT_shared_libgcc` would override the default for android, but 
> > now it no longer would. I guess that might be sensible, but it seems 
> > slightly outside of what the patch says it does. (I guess it's fine to 
> > include the change too, but I just wanted to raise it for discussion.)
> I was going by the comment, but I'm fine preserving the existing behavior. 
> @srhines do you know if `-shared-libgcc` should be supported on Android?
No, there's no such shared library to even depend on, so it wouldn't be 
supported previously or in the future.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127528/new/

https://reviews.llvm.org/D127528

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

[PATCH] D116753: Default to -fno-math-errno for musl too

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Verified with the docs from musl.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116753/new/

https://reviews.llvm.org/D116753

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

[PATCH] D114036: Add Android test case for -Wpartial-availability. Also update Android availability tests to match on the whole string, so we can distinguish between "Android 16" and "Android 16.0.0"

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.

Thanks for the expanded (and more specific) tests.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114036/new/

https://reviews.llvm.org/D114036

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

[PATCH] D99996: [Driver] Drop $DEFAULT_TRIPLE-$name as a fallback program name

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Adding Dan as an FYI. While this doesn't impact Android platform or regular NDK 
users, I suppose that someone could have some esoteric build rules that are 
relying on this, but they should probably be more explicit about what they're 
running in those cases anyways.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D6/new/

https://reviews.llvm.org/D6

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

[PATCH] D96403: [Android] Use -l:libunwind.a with --rtlib=compiler-rt

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D96403#2639180 , @srhines wrote:

> In D96403#2639137 , @smeenai wrote:
>
>> In D96403#2638932 , @rprichard 
>> wrote:
>>
>>> In D96403#2638883 , @smeenai wrote:
>>>
 With NDK r22, I only see libunwind.a for armv7. Will it be provided for 
 other architectures in future NDK versions?
>>>
>>> NDK r23 should have a libunwind.a for all architectures. This change isn't 
>>> in the r23 beta 2 that was just released, but it is in the current NDK 
>>> canary build at 
>>> https://ci.android.com/builds/branches/aosp-master-ndk/grid. (e.g. build 
>>> 7220482 
>>> ).
>>
>> Got it, thanks.
>>
>> Is there a public repository for viewing the libunwind source state that the 
>> NDK r23 libunwind is built from? I have the AOSP source tree checked out 
>> (following the instructions in 
>> https://source.android.com/setup/build/downloading) on the llvm-toolchain 
>> manifest branch. toolchain/llvm-project inside that checkout 
>> (https://android.googlesource.com/toolchain/llvm-project) has a branch for 
>> ndk-release-r23, but the merge base of that branch and upstream LLVM main is 
>> from April 29 2020, and I don't see many newer changes to libunwind after 
>> that, so I wasn't sure I was looking at the right place. I know you've done 
>> a bunch more libunwind work since then (e.g. D87750 
>> , D87881 , 
>> and D90898 ), so I was curious if those 
>> were gonna be part of r23's libunwind.
>
> So libunwind is now produced as a prebuilt with the rest of the toolchain 
> build, and not built from that copy of `toolchain/llvm_project` from the NDK 
> packaging/release branch. Thus if you want to rebuild/examine the sources, 
> you need to look at the toolchain that was included for NDK r23, which is 
> `clang-r399163b` (from 
> https://android.googlesource.com/platform/ndk/+/refs/tags/ndk-r23-beta2/ndk/toolchains.py#25).
>  Given that, you can get the manifest for the toolchain directly here 
> .
>  You can use that manifest to check out the exact sources that were used to 
> build everything (following instructions from here 
> ).
>
> ^ PROCESS is correct, but I picked the wrong version (not beta2). I'll update 
> and post a better reply in a few minutes.

It is actually using `clang-r416183` from toolchains.py 
.
 From there, you can go to the manifest for building those prebuilts 
.
 This line 

 shows you the SHA for the `toolchain/llvm-project` used to build the 
prebuilts. Finally, you can look here 

 to see what was merged. Note that there's a typo for the revision number in 
the commit message, but the upstream LLVM SHA 

 mentioned there is correct. Here 

 is where this toolchain was built in case that is useful/interesting.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96403/new/

https://reviews.llvm.org/D96403

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

[PATCH] D96403: [Android] Use -l:libunwind.a with --rtlib=compiler-rt

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D96403#2639137 , @smeenai wrote:

> In D96403#2638932 , @rprichard wrote:
>
>> In D96403#2638883 , @smeenai wrote:
>>
>>> With NDK r22, I only see libunwind.a for armv7. Will it be provided for 
>>> other architectures in future NDK versions?
>>
>> NDK r23 should have a libunwind.a for all architectures. This change isn't 
>> in the r23 beta 2 that was just released, but it is in the current NDK 
>> canary build at https://ci.android.com/builds/branches/aosp-master-ndk/grid. 
>> (e.g. build 7220482 
>> ).
>
> Got it, thanks.
>
> Is there a public repository for viewing the libunwind source state that the 
> NDK r23 libunwind is built from? I have the AOSP source tree checked out 
> (following the instructions in 
> https://source.android.com/setup/build/downloading) on the llvm-toolchain 
> manifest branch. toolchain/llvm-project inside that checkout 
> (https://android.googlesource.com/toolchain/llvm-project) has a branch for 
> ndk-release-r23, but the merge base of that branch and upstream LLVM main is 
> from April 29 2020, and I don't see many newer changes to libunwind after 
> that, so I wasn't sure I was looking at the right place. I know you've done a 
> bunch more libunwind work since then (e.g. D87750 
> , D87881 , 
> and D90898 ), so I was curious if those were 
> gonna be part of r23's libunwind.

So libunwind is now produced as a prebuilt with the rest of the toolchain 
build, and not built from that copy of `toolchain/llvm_project` from the NDK 
packaging/release branch. Thus if you want to rebuild/examine the sources, you 
need to look at the toolchain that was included for NDK r23, which is 
`clang-r399163b` (from 
https://android.googlesource.com/platform/ndk/+/refs/tags/ndk-r23-beta2/ndk/toolchains.py#25).
 Given that, you can get the manifest for the toolchain directly here 
.
 You can use that manifest to check out the exact sources that were used to 
build everything (following instructions from here 
).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96403/new/

https://reviews.llvm.org/D96403

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

[PATCH] D97993: [Driver] Suppress GCC detection under -B for non-Android

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:1911
+  SmallVector Prefixes;
+  if (TargetTriple.isAndroid())
+Prefixes.assign(D.PrefixDirs.begin(), D.PrefixDirs.end());

danalbert wrote:
> I'm not entirely sure what `D.PrefixDirs` represents so maybe Android doesn't 
> need this either.
> 
> The behavior the NDK depends on is being able to find tools co-located with 
> the Clang driver location. Aside from `as`, these are all LLVM tools (lld and 
> co).
> 
> The sysroot is expected to be in `$CLANG/../sysroot`. All our headers, 
> libraries (aside from libgcc/libatomic), and CRT objects are located there.
> 
> The clang driver install location is expected to also be a GCC install 
> directory, so libgcc/libatomic are expected at 
> `$CLANG/../lib/gcc/$TRIPLE/$GCC_VERSION`.
> 
> Typical usage for the NDK does not involve `-gcc-toolchain` or `-B` at all.
> 
> If I've understood correctly, your change can be applied to Android as well 
> without breaking any of those behaviors. @srhines will need to comment on 
> whether the Android platform build needs this, but aiui anyone depending on 
> this behavior just needs to fix their build to use `-gcc-toolchain` where 
> they were previously using `-B`.
> 
> Of course, I can't speak to what our users with custom build systems that 
> don't follow our docs might be doing.
From a look at Android's build system, we are using `-B` but we don't currently 
use `--sysroot` or `-gcc-toolchain` for platform builds. I did try a build with 
this patch (but not retaining the Android-specific part), and it still worked 
fine. From looking at the constructed command lines, the platform build 
generally adds the appropriate include paths, library paths, and invokes 
separate tools directly, so we aren't relying on `-B` for this purpose. 
Cleaning this up is on my list of things to eventually do, but as I see it, 
we're not going to be negatively impacted even with the more aggressive version 
of this patch.

> Of course, I can't speak to what our users with custom build systems that 
> don't follow our docs might be doing.
I do share this concern a bit, but it's very hard for me to quantify how hard 
it would be for someone to just adapt their build if we did make a more 
aggressive change here. If it's really as easy as passing `-gcc-toolchain`, 
then perhaps that would be fine for the Release Notes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97993/new/

https://reviews.llvm.org/D97993

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

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D83144#2268760 , @fmayer wrote:

> Thanks for the review! Should I wait for Alex to take a look, or is it fine 
> like this?

George added @alexfh a while ago, but if it's ok with you, maybe give him until 
Tuesday or Wednesday next week in case he wants to respond?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83144/new/

https://reviews.llvm.org/D83144

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

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks for creating the new test, and for making this more flexible. Everything 
else looks good here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83144/new/

https://reviews.llvm.org/D83144

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

[PATCH] D83144: Allow to specify macro names for android-comparison-in-temp-failure-retry.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry.c:1
-// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t
+// RUN: %check_clang_tidy %s android-comparison-in-temp-failure-retry %t -- 
-config="{CheckOptions: [{key: 
android-comparison-in-temp-failure-retry.RetryMacros, value: 
'TEMP_FAILURE_RETRY,MY_TEMP_FAILURE_RETRY'}]}"
 

Ideally there should either be another RUN macro here without your new options, 
or there should be a separate test/file for checking here, since we'd like to 
make sure that the default behavior still works.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/android-comparison-in-temp-failure-retry.c:28
   TEMP_FAILURE_RETRY(foo());
   TEMP_FAILURE_RETRY((foo()));
 

Don't you want a check for `MY_TEMP_FAILURE_RETRY` for cases like this too (to 
make sure that it is detecting them)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83144/new/

https://reviews.llvm.org/D83144

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

[PATCH] D83726: Fix a missing update that C compiles default to gnu17.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG9d5a8b7edb28: Fix a missing update that C compiles default 
to gnu17. (authored by srhines).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83726/new/

https://reviews.llvm.org/D83726

Files:
  clang/docs/CommandGuide/clang.rst


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -146,7 +146,7 @@
 
ISO C 2017 with GNU extensions
 
- The default C language standard is ``gnu11``, except on PS4, where it is
+ The default C language standard is ``gnu17``, except on PS4, where it is
  ``gnu99``.
 
  Supported values for the C++ language are:


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -146,7 +146,7 @@
 
ISO C 2017 with GNU extensions
 
- The default C language standard is ``gnu11``, except on PS4, where it is
+ The default C language standard is ``gnu17``, except on PS4, where it is
  ``gnu99``.
 
  Supported values for the C++ language are:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83726: Fix a missing update that C compiles default to gnu17.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
srhines added a reviewer: nickdesaulniers.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

https://reviews.llvm.org/D75383 switched the C default to gnu17, but
missed this instance.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D83726

Files:
  clang/docs/CommandGuide/clang.rst


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -146,7 +146,7 @@
 
ISO C 2017 with GNU extensions
 
- The default C language standard is ``gnu11``, except on PS4, where it is
+ The default C language standard is ``gnu17``, except on PS4, where it is
  ``gnu99``.
 
  Supported values for the C++ language are:


Index: clang/docs/CommandGuide/clang.rst
===
--- clang/docs/CommandGuide/clang.rst
+++ clang/docs/CommandGuide/clang.rst
@@ -146,7 +146,7 @@
 
ISO C 2017 with GNU extensions
 
- The default C language standard is ``gnu11``, except on PS4, where it is
+ The default C language standard is ``gnu17``, except on PS4, where it is
  ``gnu99``.
 
  Supported values for the C++ language are:
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81622: [Clang] Search computed sysroot for libc++ header paths

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks, Ryan!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81622/new/

https://reviews.llvm.org/D81622



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

[PATCH] D81622: [Clang] Search computed sysroot for libc++ header paths

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Thanks, Ryan, for diagnosing and addressing this bug.




Comment at: clang/lib/Driver/ToolChains/Linux.h:45
 llvm::opt::ArgStringList ) const override;
-  virtual std::string computeSysRoot() const;
+  virtual std::string computeSysRoot() const override;
 

Can drop `virtual` here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81622/new/

https://reviews.llvm.org/D81622



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

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks, Nick, for cleaning this up and always striving to make things more 
compatible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80828/new/

https://reviews.llvm.org/D80828



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

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:565
+case llvm::Triple::thumbeb:
+  if (Triple.getEnvironment() == llvm::Triple::Android)
+return true;

Triple.isAndroid() is more clear.



Comment at: llvm/docs/ReleaseNotes.rst:94
 
+* Clang now defaults to `-fomit-frame-pointer` when targeting Linux for arm
+  and thumb when optimizations are enabled. Users that were previously not

"non-Android Linux" is probably going to make things easier to understand here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80828/new/

https://reviews.llvm.org/D80828



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

[PATCH] D80828: [Clang][A32/T32][Linux] -O2 implies -fomit-frame-pointer

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:560
 // Don't use a frame pointer on linux if optimizing for certain targets.
+case llvm::Triple::arm:
+case llvm::Triple::armeb:

For the new targets, we should only be changing the default for non-Android 
cases. Android targets should still default to `-fno-omit-frame-pointer`. This 
makes the logic here quite a bit more complex.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80828/new/

https://reviews.llvm.org/D80828



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

[PATCH] D77168: Add a flag to debug automatic variable initialization

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D77168#1988122 , @jfb wrote:

> In D77168#1988049 , @srhines wrote:
>
> > `pragma clang attribute` is interesting, but how do you apply that in a 
> > selective fashion to local variables (especially in a way that can be 
> > automated)? At first, I didn't think the goal for this should be to create 
> > a frequently used option for **most** end users, but I do think that it 
> > could be quite useful for more folks when debugging, especially if it is 
> > easy to automate (which optimization-fuel approaches are, while pragmas are 
> > not).
>
>
> `__attribute__((uninitialized))` for more selectiveness :)
>
> Of course, not automated. In general we don't really automate compiler things 
> of this sort, say UBSan. OptRemarks is the best we really have to dig into 
> these things. One other option would be to truly randomize the init pattern: 
> emit a handful of different byte patterns, and then see the crash caused by a 
> different pattern, and track it back to which local variable got that byte 
> pattern.


That's just it though. This technique is tried and true in other 
compilers/tools. Why should we let past history dictate whether to facilitate 
automation in the present/future? Rather than having to "track it back", you 
can let the tooling do it for you. It gets you at least a breakage related to 
this transformation, and the best part is you can fix that one, and then run it 
again to chase down further issues. All without modifying source code. I feel 
like I can't be the only one who has used these kinds of tools for debugging 
transformations before, hence why I feel so invested in making this easy for 
everyone (compiler devs and regular users). UBSan isn't as relevant as it can 
provide diagnostics for the particular instance in which it was tripped. If we 
wanted to do something similar for initialization, we would have to mark the 
uninitialized value and track down any potential use of it, which seems like a 
lot more complexity and decreased performance too.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77168/new/

https://reviews.llvm.org/D77168



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

[PATCH] D77168: Add a flag to debug automatic variable initialization

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

`pragma clang attribute` is interesting, but how do you apply that in a 
selective fashion to local variables (especially in a way that can be 
automated)? At first, I didn't think the goal for this should be to create a 
frequently used option for **most** end users, but I do think that it could be 
quite useful for more folks when debugging, especially if it is easy to 
automate (which optimization-fuel approaches are, while pragmas are not).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77168/new/

https://reviews.llvm.org/D77168



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

[PATCH] D77746: [Driver] Default arm-linux-androideabi to -z max-page-size=4096

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thank you for making this clear.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77746/new/

https://reviews.llvm.org/D77746



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

[PATCH] D77168: Add a flag to debug automatic variable initialization

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D77168#1955312 , @jfb wrote:

> Do you not think `pragma` is a more general approach? That's what's used in a 
> bunch of other cases, and I'd like to see it attempted here.
>  If not, I agree with John that just counting up isn't a good bisection 
> experience. I'd rather see a begin / end bound.


I agree that begin/end is actually better.

My experience has been that you really don't want to be modifying source files 
if you can avoid it, as that is harder to automate in a script. And you do want 
to script this kind of debugging because it isn't a compiler crash, it's often 
a runtime issue that might take longer to reproduce/etc., so I think that it is 
preferable to remove the human (error prone) element of adding/tweaking 
pragmas. I did say that the pragmas are useful in other ways, but I don't think 
that it is effective for everything.

> You're also missing the auto-init in `initializeAlloca`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77168/new/

https://reviews.llvm.org/D77168



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

[PATCH] D77168: Add a flag to debug automatic variable initialization

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D77168#1953635 , @jfb wrote:

> I'm not sure this is a good idea at all. We want to keep the codepath as 
> simple as possible to avoid introducing bugs. If a codebase sees a crash then 
> it's easier to bisect one function at a time than doing something like this. 
> I'd much rather see bisection using pragma to apply the `uninitialized` 
> attribute to multiple declarations.


Certainly one function at a time is preferable, but that's not always possible. 
I do think that this would be a useful feature for making bisection easier, 
since automating the injection of `pragma`s into code seems a lot more 
challenging. Of course, the `pragma` approach is nice for other reasons 
(letting people carve out entire functions for initialized/uninitialized, but I 
think that is orthogonal to the debugging aspect.




Comment at: clang/lib/CodeGen/CGDecl.cpp:1814
+if (StopAfter) {
+  static unsigned Counter = 0;
+  if (Counter >= StopAfter)

MaskRay wrote:
> I am a bit worried about the static variable. This makes CodeGen not reusable.
The counter could exist in ASTContext instead.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77168/new/

https://reviews.llvm.org/D77168



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

[PATCH] D76452: Use LLD by default for Android.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D76452#1945029 , @MaskRay wrote:

> In D76452#1944786 , @srhines wrote:
>
> > In D76452#1944733 , @MaskRay wrote:
> >
> > > Another approach will be `-DCLANG_DEFAULT_LINKER=lld`. It provides a 
> > > default value for `-fuse-ld=`.
> >
> >
> > How does that work for Darwin builds? Is lld fully supported for MachO at 
> > this point, or will Clang still choose to use the preferred Apple linker?
>
>
> I am not clear about your use case. lld can be used as 
> ELF/Mach-O/COFF/WebAssembly linker. If you want to build ELF objects, 
> -fuse-ld=lld (via clangDriver) or configure clang with 
> -DCLANG_DEFAULT_LINKER=lld.
>  If you want to build Mach-O objects, lld's current Mach-O port lacks 
> features (and will be removed). The new Mach-O port is still under 
> development.


Right, so this is a problem when we ship a multi-target toolchain that does 
support targeting regular Linux, Windows, and Darwin (for host tools) in 
addition to Android. If we switched `CLANG_DEFAULT_LINKER`, then we would need 
to pass explicit flags to any Darwin builds to go back to using the system 
linker.

> To cross build ELF object on macOS, another alternative is a wrapper named 
> `ld` which invokes `lld -flavor gnu "$@"`

@danalbert Would this kind of idea work with your other reverted patch? I'm not 
sure exactly what broke on those builds.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76452/new/

https://reviews.llvm.org/D76452



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

[PATCH] D76452: Use LLD by default for Android.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D76452#1944733 , @MaskRay wrote:

> Another approach will be `-DCLANG_DEFAULT_LINKER=lld`. It provides a default 
> value for `-fuse-ld=`.


How does that work for Darwin builds? Is lld fully supported for MachO at this 
point, or will Clang still choose to use the preferred Apple linker?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76452/new/

https://reviews.llvm.org/D76452



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

[PATCH] D76452: Use LLD by default for Android.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks, Dan, for setting this up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76452/new/

https://reviews.llvm.org/D76452



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

[PATCH] D69925: remove redundant LLVM version from version string when setting CLANG_VENDOR

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D69925#1736434 , @efriedma wrote:

> I think the reason it's written this way is that CLANG_VERSION_STRING might 
> not correspond to an LLVM version?  For example, Apple has its own versioning 
> scheme.


Line 128 is already printing that info though, isn't it?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69925/new/

https://reviews.llvm.org/D69925



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

[PATCH] D67200: Add -static-openmp driver option

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Looks really nice. I am sure the NDK developers will be happy to see support 
for static OpenMP. Do you want to add the public NDK github issue link in the 
commit message?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67200/new/

https://reviews.llvm.org/D67200



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

[PATCH] D64666: [Sema] Enable -Wimplicit-int-float-conversion for integral to floating point precision loss

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D64666#1650325 , @sylvestre.ledru 
wrote:

> @ziangwan maybe you should add this improvement to the release notes, wdyt?


@sylvestre.ledru It's really great to hear that this new diagnostic is helpful. 
Thanks for the great suggestion.

@kongyi Can you take care of adding this to the release notes since @ziangwan 
has returned to university? I think that this really does deserve to be 
mentioned there. Thanks.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64666/new/

https://reviews.llvm.org/D64666



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

[PATCH] D64666: Allow Clang -Wconversion, -Wimplicit-float-conversion warns about integer type -> floating point type implicit conversion precision loss.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

You have patch.patch in the diff above. You should drop that.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64666/new/

https://reviews.llvm.org/D64666



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

[PATCH] D64655: [Clang][Driver] don't error for unsupported as options for -no-integrates-as

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

s/integrates/integrated in the title, but otherwise this looks good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64655/new/

https://reviews.llvm.org/D64655



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

[PATCH] D63774: android: enable double-word CAS on x86_64

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Craig, can you confirm that this is acceptable? I don't think there are any 
chips with SSE4.2 but without cx16, so this just seemed like an oversight. It 
might be a good idea to really audit the list of possible CPU features for 
other missing inclusions. Another idea would be to set a baseline minimum CPU 
for Android, which would cut down on having to specify so many features 
separately.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63774/new/

https://reviews.llvm.org/D63774



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:54
+
+void e() {
+  int pipefd[2];

gribozavr wrote:
> How is `e` different from `a`?
`e` uses O_CLOEXEC properly with `pipe2()` and makes sure that we don't issue 
additional diagnostics/fixes that are unnecessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62049/new/

https://reviews.llvm.org/D62049



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

[PATCH] D61967: [clang-tidy] Add a close-on-exec check on pipe() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe.cpp:17
+  pipe(pipefd);
+  // CHECK-MESSAGES-NOT: warning:
+}

hokein wrote:
> nit: no need to do it explicitly, if a warning is shown unexpectedly, the 
> test will fail.
I somehow never realized this (and there seem to be several places that also 
don't realize it). Thanks for letting us know.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61967/new/

https://reviews.llvm.org/D61967



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Everything looks great. Thanks for adding these improvements. While it's 
probably safe to commit this, perhaps you should give it 24 hours in case some 
of the other clang-tidy folks have different style or testing concerns.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62049/new/

https://reviews.llvm.org/D62049



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:48
+  pipe2(pipefd, O_NONBLOCK);
+  // CHECK-MESSAGES-NOT: warning:
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK));

Much like line 39 (which covers both lines 37 and 38), you can delete this 
CHECK-MESSAGES-NOT. The one on line 50 will cover both of these.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:64
+  TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK | O_CLOEXEC));
+  // CHECK-MESSAGES-NOT: warning:
+}

Only keep this CHECK-MESSAGES-NOT line.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:75
+TEMP_FAILURE_RETRY(pipe2(pipefd, O_NONBLOCK));
+// CHECK-MESSAGES-NOT: warning:
+  }

Only keep this CHECK-MESSAGES-NOT line.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62049/new/

https://reviews.llvm.org/D62049



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:52
+
+void e() {
+  int pipefd[2];

jcai19 wrote:
> srhines wrote:
> > I'm not all that familiar with writing clang-tidy-specific tests, but 
> > should these tests here denote that a diagnostic should NOT be issued? That 
> > is usually the convention in regular Clang tests, so I assume the test 
> > runner here should be equally supportive of ensuring that the contents 
> > passed through without any other diagnostics related to pipe2 and/or 
> > O_CLOEXEC.
> That makes sense, and I have seem tests for similar checks with (e.g. 
> android-cloexec-open)  and without (e.g. android-cloexec-accep4 and 
> android-cloexec-socket) additional CHECK-MESSAGES-NOT check. But based on the 
> Testing Checks section of 
> https://clang.llvm.org/extra/clang-tidy/Contributing.html, it seems typically 
> CHECK-MASSAGES and CHECK-FIXES are sufficient for clang-tidy checks. Please 
> let me know what you think.
If you look in test/clang-tidy/android-cloexec-creat.cpp, you will see that 
there are "CHECK-MESSAGES-NOT" checks that ensure the diagnostic is not issued 
in correct cases. You can put the checks on lines 39, 58, and 67, which will 
ensure that there are no additional diagnostics being generated.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62049/new/

https://reviews.llvm.org/D62049



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

[PATCH] D57930: [Driver] Verify GCCInstallation is valid

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks for picking this up and finishing it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57930/new/

https://reviews.llvm.org/D57930



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang-tools-extra/test/clang-tidy/android-cloexec-pipe2.cpp:52
+
+void e() {
+  int pipefd[2];

I'm not all that familiar with writing clang-tidy-specific tests, but should 
these tests here denote that a diagnostic should NOT be issued? That is usually 
the convention in regular Clang tests, so I assume the test runner here should 
be equally supportive of ensuring that the contents passed through without any 
other diagnostics related to pipe2 and/or O_CLOEXEC.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62049/new/

https://reviews.llvm.org/D62049



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

[PATCH] D62049: [clang-tidy] Add a close-on-exec check on pipe2() in Android module.

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/android-cloexec-pipe2.rst:19
+
+  pipe2(pipefd, O_CLOEXEC);

Shouldn't this be "O_NONBLOCK | O_CLOEXEC" instead? Why drop the O_NONBLOCK?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62049/new/

https://reviews.llvm.org/D62049



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

[PATCH] D61931: [Driver] Use --android-tls for Android ARM/AArch64 when lld is used

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

This LGTM if Ryan is happy with it. Thanks for taking care of getting a 
workaround implemented until this can be fixed.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61931/new/

https://reviews.llvm.org/D61931



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

[PATCH] D60238: Verify that Android targets generate DWARF 4 by default.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL357713: Verify that Android targets generate DWARF 4 by 
default. (authored by srhines, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D60238?vs=193621=193753#toc

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60238/new/

https://reviews.llvm.org/D60238

Files:
  cfe/trunk/test/Driver/debug-options.c


Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -17,9 +17,14 @@
 // RUN: %clang -### -c -glldb %s -target x86_64-linux-gnu 2>&1 \
 // RUN: | FileCheck -check-prefix=G -check-prefix=G_LLDB %s
 // RUN: %clang -### -c -gsce %s -target x86_64-linux-gnu 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
+
+// Android.
+// Android should always generate DWARF4.
+// RUN: %clang -### -c -g %s -target arm-linux-androideabi 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_DWARF4 %s
 
 // Darwin.
-// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
 // RUN: %clang -### -c -g %s -target x86_64-apple-darwin 2>&1 \
 // RUN: | FileCheck -check-prefix=G_STANDALONE \
 // RUN: -check-prefix=G_DWARF2 \
@@ -272,6 +277,7 @@
 //
 // G_STANDALONE: "-cc1"
 // G_STANDALONE: "-debug-info-kind=standalone"
+// G_DWARF2: "-dwarf-version=2"
 // G_DWARF4: "-dwarf-version=4"
 //
 // G_GDB:  "-debugger-tuning=gdb"


Index: cfe/trunk/test/Driver/debug-options.c
===
--- cfe/trunk/test/Driver/debug-options.c
+++ cfe/trunk/test/Driver/debug-options.c
@@ -17,9 +17,14 @@
 // RUN: %clang -### -c -glldb %s -target x86_64-linux-gnu 2>&1 \
 // RUN: | FileCheck -check-prefix=G -check-prefix=G_LLDB %s
 // RUN: %clang -### -c -gsce %s -target x86_64-linux-gnu 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
+
+// Android.
+// Android should always generate DWARF4.
+// RUN: %clang -### -c -g %s -target arm-linux-androideabi 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_DWARF4 %s
 
 // Darwin.
-// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
 // RUN: %clang -### -c -g %s -target x86_64-apple-darwin 2>&1 \
 // RUN: | FileCheck -check-prefix=G_STANDALONE \
 // RUN: -check-prefix=G_DWARF2 \
@@ -272,6 +277,7 @@
 //
 // G_STANDALONE: "-cc1"
 // G_STANDALONE: "-debug-info-kind=standalone"
+// G_DWARF2: "-dwarf-version=2"
 // G_DWARF4: "-dwarf-version=4"
 //
 // G_GDB:  "-debugger-tuning=gdb"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60238: Verify that Android targets generate DWARF 4 by default.

[Stephen Hines via Phabricator via cfe-commits]

srhines marked an inline comment as done.
srhines added inline comments.



Comment at: clang/test/Driver/debug-options.c:280
 // G_STANDALONE: "-debug-info-kind=standalone"
+// G_DWARF2: "-dwarf-version=2"
 // G_DWARF4: "-dwarf-version=4"

aprantl wrote:
> What's that for?
Look at Line 30. You added references to it from a while ago, but it didn't 
exist.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60238/new/

https://reviews.llvm.org/D60238



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

[PATCH] D60238: Verify that Android targets generate DWARF 4 by default.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
srhines added a reviewer: aprantl.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
srhines added subscribers: pirama, chh.

In the future, Android releases will support DWARF 5, but we need to
ensure that older targets only have DWARF 4 generated for them. This
patch inserts that verification for all Android releases now. The patch
also fixes 2 minor mistakes (a mistakenly moved RUN line, and the
missing G_DWARF2 check label).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60238

Files:
  clang/test/Driver/debug-options.c


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -17,9 +17,14 @@
 // RUN: %clang -### -c -glldb %s -target x86_64-linux-gnu 2>&1 \
 // RUN: | FileCheck -check-prefix=G -check-prefix=G_LLDB %s
 // RUN: %clang -### -c -gsce %s -target x86_64-linux-gnu 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
+
+// Android.
+// Android should always generate DWARF4.
+// RUN: %clang -### -c -g %s -target arm-linux-androideabi 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_DWARF4 %s
 
 // Darwin.
-// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
 // RUN: %clang -### -c -g %s -target x86_64-apple-darwin 2>&1 \
 // RUN: | FileCheck -check-prefix=G_STANDALONE \
 // RUN: -check-prefix=G_DWARF2 \
@@ -272,6 +277,7 @@
 //
 // G_STANDALONE: "-cc1"
 // G_STANDALONE: "-debug-info-kind=standalone"
+// G_DWARF2: "-dwarf-version=2"
 // G_DWARF4: "-dwarf-version=4"
 //
 // G_GDB:  "-debugger-tuning=gdb"


Index: clang/test/Driver/debug-options.c
===
--- clang/test/Driver/debug-options.c
+++ clang/test/Driver/debug-options.c
@@ -17,9 +17,14 @@
 // RUN: %clang -### -c -glldb %s -target x86_64-linux-gnu 2>&1 \
 // RUN: | FileCheck -check-prefix=G -check-prefix=G_LLDB %s
 // RUN: %clang -### -c -gsce %s -target x86_64-linux-gnu 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
+
+// Android.
+// Android should always generate DWARF4.
+// RUN: %clang -### -c -g %s -target arm-linux-androideabi 2>&1 \
+// RUN: | FileCheck -check-prefix=G -check-prefix=G_DWARF4 %s
 
 // Darwin.
-// RUN: | FileCheck -check-prefix=G -check-prefix=G_SCE %s
 // RUN: %clang -### -c -g %s -target x86_64-apple-darwin 2>&1 \
 // RUN: | FileCheck -check-prefix=G_STANDALONE \
 // RUN: -check-prefix=G_DWARF2 \
@@ -272,6 +277,7 @@
 //
 // G_STANDALONE: "-cc1"
 // G_STANDALONE: "-debug-info-kind=standalone"
+// G_DWARF2: "-dwarf-version=2"
 // G_DWARF4: "-dwarf-version=4"
 //
 // G_GDB:  "-debugger-tuning=gdb"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60059: [Driver] implement -feverything

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In addition to -fno-everything, don't forget about -fnothing and -fno-nothing, 
which seem like they would also be nice to have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60059/new/

https://reviews.llvm.org/D60059



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

[PATCH] D58531: [clang] Specify type of pthread_create builtin

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: clang/lib/AST/ASTContext.cpp:9574
 
-  assert(!RequiresICE && "Result of intrinsic cannot be required to be an 
ICE");
+  assert(!RequiresICE && "Result of function cannot be required to be an ICE");
 

I would just remove "of function" here, since it isn't correct when you are not 
decoding a function, and the two words don't make that much more meaningful 
even in the original case, so I don't think it justifies something like "of 
intrinsic/function".


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58531/new/

https://reviews.llvm.org/D58531



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

[PATCH] D58477: [Driver] Fix float ABI default for Android ARMv8.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.

Dan, this seems pretty important for the NDK. If you submit this, would you 
want it cherry-picked ASAP?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58477/new/

https://reviews.llvm.org/D58477



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

[PATCH] D57930: [Driver] Verify GCCInstallation is valid

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Would it be reasonable to have a test for this with perhaps an invalid GCC 
installation? There is some mock GCC/sysroot testing in 
https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-gcc-toolchain.c
 and 
https://github.com/llvm/llvm-project/blob/master/clang/test/Driver/android-ndk-standalone.cpp.
 I am not sure that it will be easy to trip this same bug that way, but I think 
it is possible.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57930/new/

https://reviews.llvm.org/D57930



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL353318: Switch to cantFail(), since it does the same 
assertion. (authored by srhines, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36806/new/

https://reviews.llvm.org/D36806

Files:
  cfe/trunk/lib/Tooling/Core/Replacement.cpp


Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -519,12 +519,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : MergedRanges) {
-auto Err = FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
-R.getOffset(), R.getLength(),
-std::string(R.getLength(), ' ')));
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));
+llvm::cantFail(
+FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }


Index: cfe/trunk/lib/Tooling/Core/Replacement.cpp
===
--- cfe/trunk/lib/Tooling/Core/Replacement.cpp
+++ cfe/trunk/lib/Tooling/Core/Replacement.cpp
@@ -519,12 +519,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : MergedRanges) {
-auto Err = FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
-R.getOffset(), R.getLength(),
-std::string(R.getLength(), ' ')));
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));
+llvm::cantFail(
+FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D36806#1385936 , @lhames wrote:

> Looks like this was LGTM'd but never applied. Stephen -- do you have commit 
> access?


Yeah, I was waiting on someone with approval for this area of the code to 
comment and then lost track of it. I can recheck it tonight and then submit it. 
Thank you for going back through these patches. :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D36806/new/

https://reviews.llvm.org/D36806



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

[PATCH] D55953: Android is not GNU, so don't claim that it is.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Sorry about the delay.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55953/new/

https://reviews.llvm.org/D55953



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

[PATCH] D34796: upporting -f(no)-reorder-functions flag, clang side change

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: test/Driver/function-sections.c:77
 // RUN:   | FileCheck --check-prefix=CHECK-NOUS %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1\

There should ideally be a test for the default behavior as well (and please 
consider mentioning the default behavior in the commit message).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D34796/new/

https://reviews.llvm.org/D34796



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

[PATCH] D53850: Declares __cpu_model as dso local

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Craig, does this look ok now?


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53850/new/

https://reviews.llvm.org/D53850



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In D55029#1312167 , @ruiu wrote:

> I wouldn't rush to submit this now, given that this issue is not new at all. 
> Maybe we can just wait for Peter's response?


Sure, that's fine too. I misunderstood the "tangent" part of your initial 
comment to mean that any such change would be done in a separate CL.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55029/new/

https://reviews.llvm.org/D55029



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

[PATCH] D55029: set default max-page-size to 4KB in lld for Android Aarch64

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks Zhizhou! I'm curious about Peter's answer to the 64KiB default as well, 
but I think we should move forward with getting this patch submitted.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D55029/new/

https://reviews.llvm.org/D55029



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

[PATCH] D53463: [Driver] allow Android triples to alias for non Android targets

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Please remove the reference to b/X in the commit message. LLVM doesn't allow 
internal bug numbers, and you described the issue well enough without it.


Repository:
  rC Clang

https://reviews.llvm.org/D53463



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

[PATCH] D53121: [Driver] Add defaults for Android ARM FPUs.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

This LGTM, but we should wait to hear from Kristof before submitting.


Repository:
  rC Clang

https://reviews.llvm.org/D53121



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

[PATCH] D53109: [Driver] Default Android toolchains to libc++.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Really cool! Thanks for making everything easier to use out-of-the-box.


Repository:
  rC Clang

https://reviews.llvm.org/D53109



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

[PATCH] D52248: [SEMA] ignore duplicate declaration specifiers from typeof exprs

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: lib/Sema/SemaType.cpp:1682
 // or via one or more typedefs."
-if (!S.getLangOpts().C99 && !S.getLangOpts().CPlusPlus
-&& TypeQuals & Result.getCVRQualifiers()) {

This is broken for C11 and C17 (even before you touch anything). As we just 
talked about, let's have a helper function to detect the oldest version (and 
maybe even that should get promoted as a LANGOPT).


Repository:
  rC Clang

https://reviews.llvm.org/D52248



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

[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Thanks @dblaikie for the quick fixup. I must have accidentally dropped the '!', 
because I did run check-all to test the change.


Repository:
  rL LLVM

https://reviews.llvm.org/D52191



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

[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rC342501: Fix logic around determining use of frame pointer 
with -pg. (authored by srhines, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D52191?vs=165826=166007#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D52191

Files:
  lib/Driver/ToolChains/Clang.cpp


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4956,8 +4956,7 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasFlag(options::OPT_fomit_frame_pointer,
- options::OPT_fno_omit_frame_pointer, /*default=*/false))
+if (shouldUseFramePointer(Args, Triple))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4956,8 +4956,7 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasFlag(options::OPT_fomit_frame_pointer,
- options::OPT_fno_omit_frame_pointer, /*default=*/false))
+if (shouldUseFramePointer(Args, Triple))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342501: Fix logic around determining use of frame pointer 
with -pg. (authored by srhines, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D52191

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp


Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -4956,8 +4956,7 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasFlag(options::OPT_fomit_frame_pointer,
- options::OPT_fno_omit_frame_pointer, /*default=*/false))
+if (shouldUseFramePointer(Args, Triple))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 


Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -4956,8 +4956,7 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasFlag(options::OPT_fomit_frame_pointer,
- options::OPT_fno_omit_frame_pointer, /*default=*/false))
+if (shouldUseFramePointer(Args, Triple))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/D52191#1238628, @dblaikie wrote:

> Sure, looks good. Though my other/vague concern is why does this case error 
> about fomit-frame-pointer having no effect, but other things (like using 
> -fomit-frame-pointer on a target that requires frame pointers) that ignore 
> -fomit-frame-pointer don't? Weird. But it probably makes sense somehow.


I think the original issue is that one should not **explicitly** say "omit 
frame pointers" and "instrument for profiling with mcount". It's possible that 
there should be other errors for using "omit frame pointers" on targets that 
require them, but that should be checked independently elsewhere. Do we have 
many (any) of these kinds of targets? I'm going to submit this patch, but am 
happy to add a diagnostic later on for the other case, if you think that is 
worthwhile.


Repository:
  rC Clang

https://reviews.llvm.org/D52191



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

[PATCH] D52191: Fix logic around determining use of frame pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
srhines added a reviewer: dblaikie.
Herald added a subscriber: cfe-commits.

As part of r342165, I rewrote the logic to check whether
-fno-omit-frame-pointer was passed after a -fomit-frame-pointer
argument. This CL switches that logic to use the consolidated
shouldUseFramePointer() function. This fixes a potential issue where -pg
gets used with -fomit-frame-pointer on a platform that must always retain
frame pointers.


Repository:
  rC Clang

https://reviews.llvm.org/D52191

Files:
  lib/Driver/ToolChains/Clang.cpp


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4956,8 +4956,7 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasFlag(options::OPT_fomit_frame_pointer,
- options::OPT_fno_omit_frame_pointer, /*default=*/false))
+if (shouldUseFramePointer(Args, Triple))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 


Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4956,8 +4956,7 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasFlag(options::OPT_fomit_frame_pointer,
- options::OPT_fno_omit_frame_pointer, /*default=*/false))
+if (shouldUseFramePointer(Args, Triple))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51713: Support -fno-omit-frame-pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL342165: Support -fno-omit-frame-pointer with -pg. (authored 
by srhines, committed by ).
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

https://reviews.llvm.org/D51713

Files:
  cfe/trunk/lib/Driver/ToolChains/Clang.cpp
  cfe/trunk/test/Driver/clang_f_opts.c


Index: cfe/trunk/test/Driver/clang_f_opts.c
===
--- cfe/trunk/test/Driver/clang_f_opts.c
+++ cfe/trunk/test/Driver/clang_f_opts.c
@@ -531,3 +531,8 @@
 // RUN: %clang -### -S -fno-delete-null-pointer-checks 
-fdelete-null-pointer-checks %s 2>&1 | FileCheck 
-check-prefix=CHECK-NULL-POINTER-CHECKS %s
 // CHECK-NO-NULL-POINTER-CHECKS: "-fno-delete-null-pointer-checks"
 // CHECK-NULL-POINTER-CHECKS-NOT: "-fno-delete-null-pointer-checks"
+
+// RUN: %clang -### -S -fomit-frame-pointer -pg %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-MIX-OMIT-FP-PG %s
+// RUN: %clang -### -S -fomit-frame-pointer -fno-omit-frame-pointer -pg %s 
2>&1 | FileCheck -check-prefix=CHECK-MIX-NO-OMIT-FP-PG %s
+// CHECK-NO-MIX-OMIT-FP-PG: '-fomit-frame-pointer' not allowed with '-pg'
+// CHECK-MIX-NO-OMIT-FP-PG-NOT: '-fomit-frame-pointer' not allowed with '-pg'
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -4910,7 +4910,8 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasArg(options::OPT_fomit_frame_pointer))
+if (Args.hasFlag(options::OPT_fomit_frame_pointer,
+ options::OPT_fno_omit_frame_pointer, /*default=*/false))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 


Index: cfe/trunk/test/Driver/clang_f_opts.c
===
--- cfe/trunk/test/Driver/clang_f_opts.c
+++ cfe/trunk/test/Driver/clang_f_opts.c
@@ -531,3 +531,8 @@
 // RUN: %clang -### -S -fno-delete-null-pointer-checks -fdelete-null-pointer-checks %s 2>&1 | FileCheck -check-prefix=CHECK-NULL-POINTER-CHECKS %s
 // CHECK-NO-NULL-POINTER-CHECKS: "-fno-delete-null-pointer-checks"
 // CHECK-NULL-POINTER-CHECKS-NOT: "-fno-delete-null-pointer-checks"
+
+// RUN: %clang -### -S -fomit-frame-pointer -pg %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-OMIT-FP-PG %s
+// RUN: %clang -### -S -fomit-frame-pointer -fno-omit-frame-pointer -pg %s 2>&1 | FileCheck -check-prefix=CHECK-MIX-NO-OMIT-FP-PG %s
+// CHECK-NO-MIX-OMIT-FP-PG: '-fomit-frame-pointer' not allowed with '-pg'
+// CHECK-MIX-NO-OMIT-FP-PG-NOT: '-fomit-frame-pointer' not allowed with '-pg'
Index: cfe/trunk/lib/Driver/ToolChains/Clang.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/Clang.cpp
+++ cfe/trunk/lib/Driver/ToolChains/Clang.cpp
@@ -4910,7 +4910,8 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasArg(options::OPT_fomit_frame_pointer))
+if (Args.hasFlag(options::OPT_fomit_frame_pointer,
+ options::OPT_fno_omit_frame_pointer, /*default=*/false))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51713: Support -fno-omit-frame-pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/D51713#1232414, @manojgupta wrote:

> What is the call generated with -pg for AMR32, __gnu_mcount_nc or _mount? 
> __gnu_mcount_nc  with "-pg" is known to be broken ( 
> https://bugs.llvm.org/show_bug.cgi?id=33845)


I CCed myself on that issue as we are trying to do a better job of supporting 
code coverage, but I don't think this bug is relevant for this patch.

Unless there are any additional reviews, I will likely submit this patch in a 
few hours.


Repository:
  rC Clang

https://reviews.llvm.org/D51713



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

[PATCH] D51713: Support -fno-omit-frame-pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

http://b/32510864 was the internal bug request, so I am noting it here for 
future reference, but I think that the patch itself is pretty self-explanatory 
(rather than filing a distinct upstream bug about this issue).


Repository:
  rC Clang

https://reviews.llvm.org/D51713



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

[PATCH] D51713: Support -fno-omit-frame-pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

This was discovered in the Android build system (which passes 
-fomit-frame-pointer by default for ARM configurations. This leads to the 
inability to specify -pg, since there is no way to override the mere presence 
of -fomit-frame-pointer.


Repository:
  rC Clang

https://reviews.llvm.org/D51713



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

[PATCH] D51713: Support -fno-omit-frame-pointer with -pg.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
Herald added a subscriber: cfe-commits.
srhines added a subscriber: nickdesaulniers.

Previously, any instance of -fomit-frame-pointer would make it such that
-pg was an invalid flag combination. If -fno-omit-frame-pointer is
passed later on the command line (such that it actually takes effect),
-pg should be allowed.


Repository:
  rC Clang

https://reviews.llvm.org/D51713

Files:
  lib/Driver/ToolChains/Clang.cpp
  test/Driver/clang_f_opts.c


Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -531,3 +531,8 @@
 // RUN: %clang -### -S -fno-delete-null-pointer-checks 
-fdelete-null-pointer-checks %s 2>&1 | FileCheck 
-check-prefix=CHECK-NULL-POINTER-CHECKS %s
 // CHECK-NO-NULL-POINTER-CHECKS: "-fno-delete-null-pointer-checks"
 // CHECK-NULL-POINTER-CHECKS-NOT: "-fno-delete-null-pointer-checks"
+
+// RUN: %clang -### -S -fomit-frame-pointer -pg %s 2>&1 | FileCheck 
-check-prefix=CHECK-NO-MIX-OMIT-FP-PG %s
+// RUN: %clang -### -S -fomit-frame-pointer -fno-omit-frame-pointer -pg %s 
2>&1 | FileCheck -check-prefix=CHECK-MIX-NO-OMIT-FP-PG %s
+// CHECK-NO-MIX-OMIT-FP-PG: '-fomit-frame-pointer' not allowed with '-pg'
+// CHECK-MIX-NO-OMIT-FP-PG-NOT: '-fomit-frame-pointer' not allowed with '-pg'
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4898,7 +4898,8 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasArg(options::OPT_fomit_frame_pointer))
+if (Args.hasFlag(options::OPT_fomit_frame_pointer,
+ options::OPT_fno_omit_frame_pointer, /*default=*/false))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 


Index: test/Driver/clang_f_opts.c
===
--- test/Driver/clang_f_opts.c
+++ test/Driver/clang_f_opts.c
@@ -531,3 +531,8 @@
 // RUN: %clang -### -S -fno-delete-null-pointer-checks -fdelete-null-pointer-checks %s 2>&1 | FileCheck -check-prefix=CHECK-NULL-POINTER-CHECKS %s
 // CHECK-NO-NULL-POINTER-CHECKS: "-fno-delete-null-pointer-checks"
 // CHECK-NULL-POINTER-CHECKS-NOT: "-fno-delete-null-pointer-checks"
+
+// RUN: %clang -### -S -fomit-frame-pointer -pg %s 2>&1 | FileCheck -check-prefix=CHECK-NO-MIX-OMIT-FP-PG %s
+// RUN: %clang -### -S -fomit-frame-pointer -fno-omit-frame-pointer -pg %s 2>&1 | FileCheck -check-prefix=CHECK-MIX-NO-OMIT-FP-PG %s
+// CHECK-NO-MIX-OMIT-FP-PG: '-fomit-frame-pointer' not allowed with '-pg'
+// CHECK-MIX-NO-OMIT-FP-PG-NOT: '-fomit-frame-pointer' not allowed with '-pg'
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -4898,7 +4898,8 @@
   }
 
   if (Arg *A = Args.getLastArg(options::OPT_pg))
-if (Args.hasArg(options::OPT_fomit_frame_pointer))
+if (Args.hasFlag(options::OPT_fomit_frame_pointer,
+ options::OPT_fno_omit_frame_pointer, /*default=*/false))
   D.Diag(diag::err_drv_argument_not_allowed_with) << "-fomit-frame-pointer"
   << A->getAsString(Args);
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51521: Refactor Addlibgcc to make the when and what logic more straightfoward.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks for cleaning this up and adding better checks for Android. :)


Repository:
  rC Clang

https://reviews.llvm.org/D51521



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

[PATCH] D51068: [Android] Default to -fno-math-errno

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added inline comments.
This revision is now accepted and ready to land.



Comment at: lib/Driver/ToolChains/Linux.cpp:913
+return false;
+  return Generic_ELF::IsMathErrnoDefault();
+}

pirama wrote:
> I tried to be defensive here in case the default changes in the future.  I 
> can simplify to just return true here if that'd be simpler.
I agree with this, since this matches the present behavior.


Repository:
  rC Clang

https://reviews.llvm.org/D51068



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

[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

[Stephen Hines via Phabricator via cfe-commits]

srhines abandoned this revision.
srhines added a comment.

Removed this in favor of the suggestions here. Setting the 
CMAKE_FIND_ROOT_PATH_MODE* variables does make this properly hermetic.


Repository:
  rC Clang

https://reviews.llvm.org/D48459



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

[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

LGTM. Thanks for making the checking more clear. This should help bridge the 
gap for now, so that we can resume getting per-library profile data in Android.


https://reviews.llvm.org/D45454



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

[PATCH] D45454: Add llvm_gcov_flush to be called outside a shared library

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Thanks for picking this up again and updating the change to add 
llvm_gcov_flush().




Comment at: test/profile/Inputs/instrprof-dlopen-dlclose-main.c:20
+  void (*gcov_flush)() = (void (*)())dlsym(f1_handle, "__gcov_flush");
+  if (gcov_flush != NULL) {
+fprintf(stderr, "__gcov_flush should not be visible in func.shared'\n");

Should also clear dlerror() before this call and check that dlerror() returned 
a non-NULL pointer indicating that this search failed.


https://reviews.llvm.org/D45454



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

[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/D48459#1143012, @smeenai wrote:

> Actually, I would imagine that if you're cross-compiling or using a custom 
> sysroot, you should probably also specify CMAKE_FIND_ROOT_PATH and set 
> CMAKE_FIND_ROOT_PATH_MODE_PACKAGE to ONLY, to limit all these searches to the 
> desired directories?
>
> (I'm actually playing around with a bunch of this stuff myself right now. I'm 
> curious about your CMake setup for a custom sysroot or cross-compilation and 
> what issues you've come across.)


https://android.googlesource.com/toolchain/llvm_android/+/master/build.py is 
what we use to wrap all the CMake invocations that we do to create Android's 
toolchain. We have to currently iterate because we need to create multiple 
different runtimes for all of our supported architectures. I don't think we 
have hit many other problems. This is something that is only affecting some of 
our users, but I wanted to try to fix it better than "just uninstall libxml2".

As far as this patch goes, I am trying to experiment with those other variables 
you mentioned. If that is enough to solve all of our problems, I'm probably 
just going to abandon these two patches. CMake remains terribly painful to use, 
especially for these cross-compiling scenarios, which are not that 
well-documented in general.


Repository:
  rC Clang

https://reviews.llvm.org/D48459



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

[PATCH] D45454: Make __gcov_flush visible outside a shared library

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/D45454#1144099, @davidxl wrote:

> GCC's behavior is not documented and it also has changed over the years.
>
> Unless there is a bug, changing LLVM's gcov_flush visibility can potentially 
> break clients that depend on this behavior.


I think that's the issue though. LLVM changed visibility of this function 
recently too. We had Android code depending on this function being visible, and 
that broke.


https://reviews.llvm.org/D45454



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

[PATCH] D48570: [Driver] Do not add -lpthread & -lrt with -static-libsan on Android

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.

Agree. This is the first time anyone is linking against static sanitizers on 
Android, so this is just something that we missed updating in the past.


Repository:
  rC Clang

https://reviews.llvm.org/D48570



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

[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Rui, I added you to this review (and the other corresponding one), as you 
reviewed ecbeckmann's original work here. Can you take a quick look at these? 
Thanks.


Repository:
  rC Clang

https://reviews.llvm.org/D48459



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

[PATCH] D48459: Respect CMAKE_SYSROOT and CMAKE_CROSSCOMPILING when searching for libxml2.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
Herald added a subscriber: mgorny.

If we ignore these variables, we end up always using the host
information for libxml2, when it is not appropriate. This makes builds
less hermetic than they should ideally be.


Repository:
  rC Clang

https://reviews.llvm.org/D48459

Files:
  CMakeLists.txt


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -186,7 +186,9 @@
 
 # Don't look for libxml if we're using MSan, since uninstrumented third party
 # code may call MSan interceptors like strlen, leading to false positives.
-if(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
+# Similarly, don't look for libxml on the host if we have a specific sysroot or
+# are cross-compiling.
+if(NOT CMAKE_SYSROOT AND NOT CMAKE_CROSSCOMPILING AND NOT LLVM_USE_SANITIZER 
MATCHES "Memory.*")
   set (LIBXML2_FOUND 0)
   find_package(LibXml2 2.5.3 QUIET)
   if (LIBXML2_FOUND)


Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -186,7 +186,9 @@
 
 # Don't look for libxml if we're using MSan, since uninstrumented third party
 # code may call MSan interceptors like strlen, leading to false positives.
-if(NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
+# Similarly, don't look for libxml on the host if we have a specific sysroot or
+# are cross-compiling.
+if(NOT CMAKE_SYSROOT AND NOT CMAKE_CROSSCOMPILING AND NOT LLVM_USE_SANITIZER MATCHES "Memory.*")
   set (LIBXML2_FOUND 0)
   find_package(LibXml2 2.5.3 QUIET)
   if (LIBXML2_FOUND)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D47894: [clang]: Add support for "-fno-delete-null-pointer-checks"

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/D47894#1125728, @jyknight wrote:

> In https://reviews.llvm.org/D47894#1125653, @efriedma wrote:
>
> > The problem would come from propagating nonnull-ness from something which 
> > isn't inherently nonnull.  For example, strlen has a nonnull argument, so 
> > `strlen(NULL)` is UB, therefore given `int z = strlen(x); if (x) {...}`, we 
> > can remove the null check.  (Not sure we actually do this transform at the 
> > moment, but it's something we could do in the future.)
>
>
> I think the question there is actually whether we need to, in addition to 
> supporting null pointer dereference, also cause all 
> `__attribute__((nonnull))` annotations at the C/C++ level to be ignored.
>
> And, yes, I believe we should.


Is that what the kernel community actually wants though? There are certainly 
others who want a way to strip `__attribute__((nonnull))` completely, which 
seems a bit orthogonal to the removal of null checks in general. I think it 
would be good to support such a flag, but the presence of `nonnull` inside 
kernel sources leads me to believe they want those cases to be treated 
specially.


Repository:
  rC Clang

https://reviews.llvm.org/D47894



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

[PATCH] D46300: [Clang] Implement function attribute no_stack_protector.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Looks fine. Please wait for a more experienced person to review this, however. 
Thanks for adding this feature.


Repository:
  rC Clang

https://reviews.llvm.org/D46300



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

[PATCH] D45291: [Driver] Infer Android sysroot location.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks for adding this simplified support. Sorry about the extreme delay in 
getting these reviewed.


Repository:
  rC Clang

https://reviews.llvm.org/D45291



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

[PATCH] D44995: [Driver] Include the Android multiarch includes.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks for adding this support. Sorry about the delay in reviewing.


Repository:
  rC Clang

https://reviews.llvm.org/D44995



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

[PATCH] D44815: [AArch64]: Add support for parsing rN registers.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Peter also requested that a test be added to make sure that rY was not accepted 
by the Clang assembler as a true synonym for xY.




Comment at: lib/Basic/Targets/AArch64.cpp:320
+{{"r24"}, "x24"}, {{"r25"}, "x25"}, {{"r26"}, "x26"}, {{"r27"}, "x27"},
+{{"r28"}, "x28"}, {{"r29"}, "fp"},  {{"r30"}, "lr"},
 // The S/D/Q and W/X registers overlap, but aren't really aliases; we

For x29, x30, you should really be grouping them together. For instance:

{{"r29", "fp"}, "x29}, {{"r30", "lr"}, "x30"}

This lets you remove the x29/fp and x30/lr from the first line.


Repository:
  rC Clang

https://reviews.llvm.org/D44815



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

[PATCH] D44229: Don't use -pie in relocatable link.

[Stephen Hines via Phabricator via cfe-commits]

srhines accepted this revision.
srhines added a comment.
This revision is now accepted and ready to land.

Thanks for fixing this quickly. :)


https://reviews.llvm.org/D44229



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

[PATCH] D42676: Add support for LLVM_REPOSITORY_STRING.

[Stephen Hines via Phabricator via cfe-commits]

srhines abandoned this revision.
srhines added a comment.

I'm going to solve this a different way.


Repository:
  rC Clang

https://reviews.llvm.org/D42676



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

[PATCH] D43203: [Driver] Generate .eh_frame_hdr for static executables too.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

LGTM, but we should make sure that there are no objections, especially since 
there is no rationale for why this was present to begin with.


Repository:
  rC Clang

https://reviews.llvm.org/D43203



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

[PATCH] D42676: Add support for LLVM_REPOSITORY_STRING.

[Stephen Hines via Phabricator via cfe-commits]

srhines created this revision.
Herald added subscribers: cfe-commits, hintonda, mgorny.
srhines added a reviewer: beanz.

Without this, vendor Clang builds can occasionally pick up an arbitrary
mirror location for the repository (at least on Android builds). This
allows vendor builds to be more reproducible.


Repository:
  rC Clang

https://reviews.llvm.org/D42676

Files:
  CMakeLists.txt
  lib/Basic/Version.cpp


Index: lib/Basic/Version.cpp
===
--- lib/Basic/Version.cpp
+++ lib/Basic/Version.cpp
@@ -55,6 +55,9 @@
 }
 
 std::string getLLVMRepositoryPath() {
+#if defined(LLVM_REPOSITORY_STRING)
+  return LLVM_REPOSITORY_STRING;
+#else
 #ifdef LLVM_REPOSITORY
   StringRef URL(LLVM_REPOSITORY);
 #else
@@ -69,6 +72,7 @@
 URL = URL.substr(Start);
 
   return URL;
+#endif
 }
 
 std::string getClangRevision() {
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -267,6 +267,13 @@
   add_definitions(-DCLANG_REPOSITORY_STRING="${CLANG_REPOSITORY_STRING}")
 endif()
 
+set(LLVM_REPOSITORY_STRING "" CACHE STRING
+  "Vendor-specific text for showing the repository the LLVM source is taken 
from.")
+
+if(LLVM_REPOSITORY_STRING)
+  add_definitions(-DLLVM_REPOSITORY_STRING="${LLVM_REPOSITORY_STRING}")
+endif()
+
 set(CLANG_VENDOR_UTI "org.llvm.clang" CACHE STRING
   "Vendor-specific uti.")
 


Index: lib/Basic/Version.cpp
===
--- lib/Basic/Version.cpp
+++ lib/Basic/Version.cpp
@@ -55,6 +55,9 @@
 }
 
 std::string getLLVMRepositoryPath() {
+#if defined(LLVM_REPOSITORY_STRING)
+  return LLVM_REPOSITORY_STRING;
+#else
 #ifdef LLVM_REPOSITORY
   StringRef URL(LLVM_REPOSITORY);
 #else
@@ -69,6 +72,7 @@
 URL = URL.substr(Start);
 
   return URL;
+#endif
 }
 
 std::string getClangRevision() {
Index: CMakeLists.txt
===
--- CMakeLists.txt
+++ CMakeLists.txt
@@ -267,6 +267,13 @@
   add_definitions(-DCLANG_REPOSITORY_STRING="${CLANG_REPOSITORY_STRING}")
 endif()
 
+set(LLVM_REPOSITORY_STRING "" CACHE STRING
+  "Vendor-specific text for showing the repository the LLVM source is taken from.")
+
+if(LLVM_REPOSITORY_STRING)
+  add_definitions(-DLLVM_REPOSITORY_STRING="${LLVM_REPOSITORY_STRING}")
+endif()
+
 set(CLANG_VENDOR_UTI "org.llvm.clang" CACHE STRING
   "Vendor-specific uti.")
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40476: Switch kryo to use -mcpu=cortex-a57 when invoking the assembler

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: lib/Driver/ToolChains/Gnu.cpp:661
 Arg *A;
-if ((A = Args.getLastArg(options::OPT_mcpu_EQ)) &&
-StringRef(A->getValue()).equals_lower("krait"))
-  CmdArgs.push_back("-mcpu=cortex-a15");
-else
-  Args.AddLastArg(CmdArgs, options::OPT_mcpu_EQ);
+if ((A = Args.getLastArg(options::OPT_mcpu_EQ))) {
+  StringRef CPUArg(A->getValue());

Arg *A can also get moved here.


https://reviews.llvm.org/D40476



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

[PATCH] D40476: Switch kryo to use -mcpu=cortex-a57 when invoking the assembler

[Stephen Hines via Phabricator via cfe-commits]

srhines added inline comments.



Comment at: lib/Driver/ToolChains/Gnu.cpp:660
+// of a cpu flag.
+Arg *A = Args.getLastArg(options::OPT_mcpu_EQ);
+if (A) {

Is it better to sink A into the if condition again?



Comment at: test/Driver/as-mcpu.c:1
+// == Check that krait is substituted by cortex-15 when 
invoking
+// the assembler

cortex-a15 - this is missing the "a".



Comment at: test/Driver/as-mcpu.c:6
+
+// == Check that kryo is substituted by cortex-57 when invoking
+// the assembler

cortex-a57


https://reviews.llvm.org/D40476



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping again. This is really trivial.


https://reviews.llvm.org/D36806



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping.


https://reviews.llvm.org/D36806



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping


https://reviews.llvm.org/D36806



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Ping.


https://reviews.llvm.org/D36806



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

In https://reviews.llvm.org/D36806#856070, @lhames wrote:

> I've added an optional ErrMsg argument to cantFail in r312066 - you can now 
> use this to preserve your explanatory text.


Thank you for adding this! I updated the CL to make use of it.


https://reviews.llvm.org/D36806



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

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines updated this revision to Diff 113351.
srhines added a comment.

Switch to using ErrMsg in cantFail().


https://reviews.llvm.org/D36806

Files:
  lib/Tooling/Core/Replacement.cpp


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -498,12 +498,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : MergedRanges) {
-auto Err = FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
-R.getOffset(), R.getLength(),
-std::string(R.getLength(), ' ')));
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));
+llvm::cantFail(
+FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }


Index: lib/Tooling/Core/Replacement.cpp
===
--- lib/Tooling/Core/Replacement.cpp
+++ lib/Tooling/Core/Replacement.cpp
@@ -498,12 +498,11 @@
 return MergedRanges;
   tooling::Replacements FakeReplaces;
   for (const auto  : MergedRanges) {
-auto Err = FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
-R.getOffset(), R.getLength(),
-std::string(R.getLength(), ' ')));
-assert(!Err &&
-   "Replacements must not conflict since ranges have been merged.");
-llvm::consumeError(std::move(Err));
+llvm::cantFail(
+FakeReplaces.add(Replacement(Replaces.begin()->getFilePath(),
+ R.getOffset(), R.getLength(),
+ std::string(R.getLength(), ' '))),
+"Replacements must not conflict since ranges have been merged.");
   }
   return FakeReplaces.merge(Replaces).getAffectedRanges();
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D36806: Switch to cantFail(), since it does the same assertion.

[Stephen Hines via Phabricator via cfe-commits]

srhines added a comment.

Any other comments?


https://reviews.llvm.org/D36806



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