[PATCH] D158641: [AArch64] Fix FMV ifunc resolver usage on old Android APIs. Rename internal compiler-rt FMV functions.

[Elliott Hughes via Phabricator via cfe-commits]

enh accepted this revision.
enh added a comment.

yeah, lgtm from the Android side too, unless rprichard has more comments?


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] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

(yeah, similar for Android --- we'll just use the clang one as-is. but if 
there's already precedent for allowing include_next, that doesn't hurt us 
either.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

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

[Elliott Hughes via Phabricator via cfe-commits]

enh 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

rprichard wrote:
> enh wrote:
> > srhines wrote:
> > > 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).
> > > 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).
> > 
> > yeah, that's the point of this code --- it's a runtime check so the NDK 
> > "just works".
> > 
> > but if people want the `__ANDROID_API__` `#if` too, that's fine. (and, like 
> > you say, the platform's two variants mean that we'll be testing both code 
> > paths, so i'm not worried about "one of these is the only one that anyone's 
> > actually building" problem.)
> > 
> > i have no opinion on whether anyone llvm is more/less likely to understand 
> > if/when `if (android_get_device_api_level() < 30)` versus `#if 
> > __ANDROID_API__ < 30` can be deleted.
> > 
> > i think the best argument for leaving this change as-is would be "anyone 
> > building their own is less likely to screw up" (since developers struggle 
> > all the time with the difference between "target" and "min" api, because 
> > the ndk terminology is different to the AndroidManifest.xml stuff that 
> > developers are more familiar with, which causes confusion). so if this was 
> > in libc++ (where i know people do build their own), i'd argue for the code 
> > as-is. but since it's compiler-rt (and i'm not aware that anyone's building 
> > that themselves) i don't think it matters either way?
> > 
> > to be clear, i'm imagining:
> > ```
> > #if __ANDROID_API__ < 30
> >   if (android_get_device_api_level() < 30) {
> > setCPUFeature(FEAT_MAX);
> > return;
> >   }
> > #endif
> > ```
> > (which brings us back to the "this is confusing" --- _just_ having the 
> > `#if` would be subtly different, which is why if i'd written this, i'd have 
> > written it as-is too!)
> Unless I'm missing something, calling android_get_device_api_level doesn't 
> work, because (a) the loader hasn't performed the necessary relocations and 
> (b) that API reads system properties, which haven't been initialized yet.
> 
> Maybe the device API could/should be exported to a /dev file, which is how we 
> exported the CPU variant to ifunc resolvers.
> 
> We could redesign Bionic so that an ifunc resolver can call 
> android_get_device_api_level: e.g:
>  - Relocate objects in bottom-up order.
>  - Collect ifunc relocations and defer them until after non-ifunc relocations.
>  - Have android_get_device_api_level in libc.so call into the 

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

[Elliott Hughes via Phabricator via cfe-commits]

enh 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

srhines wrote:
> 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).
> 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).

yeah, that's the point of this code --- it's a runtime check so the NDK "just 
works".

but if people want the `__ANDROID_API__` `#if` too, that's fine. (and, like you 
say, the platform's two variants mean that we'll be testing both code paths, so 
i'm not worried about "one of these is the only one that anyone's actually 
building" problem.)

i have no opinion on whether anyone llvm is more/less likely to understand 
if/when `if (android_get_device_api_level() < 30)` versus `#if __ANDROID_API__ 
< 30` can be deleted.

i think the best argument for leaving this change as-is would be "anyone 
building their own is less likely to screw up" (since developers struggle all 
the time with the difference between "target" and "min" api, because the ndk 
terminology is different to the AndroidManifest.xml stuff that developers are 
more familiar with, which causes confusion). so if this was in libc++ (where i 
know people do build their own), i'd argue for the code as-is. but since it's 
compiler-rt (and i'm not aware that anyone's building that themselves) i don't 
think it matters either way?

to be clear, i'm imagining:
```
#if __ANDROID_API__ < 30
  if (android_get_device_api_level() < 30) {
setCPUFeature(FEAT_MAX);
return;
  }
#endif
```
(which brings us back to the "this is confusing" --- _just_ having the `#if` 
would be subtly different, which is why if i'd written this, i'd have written 
it as-is too!)


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] D158641: [AArch64][Android][DRAFT] Fix FMV ifunc resolver usage on old Android APIs.

[Elliott Hughes via Phabricator via cfe-commits]

enh 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

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 :-) )


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] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/test/C/C2x/n2359.c:40
+#error "__STDC_VERSION_STDCKDINT_H__ not defined"
+// expected-error@-1 {{"__STDC_VERSION_STDCKDINT_H__ not defined"}}
+#endif

don't you need another test somewhere that this _is_ defined under some 
circumstances? (and a definition in the header itself!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:10
+
+#ifndef __STDC_VERSION_STDCKDINT_H__
+#define __STDC_VERSION_STDCKDINT_H__

i think this should just be `__STDCKDINT_H` to match the other headers' include 
guards, and then you want a _separate_ #define that defines 
`__STDC_VERSION_STDCKDINT_H__` (and defines it to the value you're claiming to 
define it to in the release notes, but don't seem to actually be defining it to 
here :-) ).

(and maybe add a test that the macro is defined with a value to the tests?)



Comment at: clang/lib/Headers/stdckdint.h:18
+#else
+#error "we need a compiler extension for this"
+#endif

i think this #else should be deleted now? (it will give a misleading error if 
you build with pre-c23.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))

ZijunZhao wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > hiraditya wrote:
> > > > enh wrote:
> > > > > hiraditya wrote:
> > > > > > xbolva00 wrote:
> > > > > > > yabinc wrote:
> > > > > > > > enh wrote:
> > > > > > > > > enh wrote:
> > > > > > > > > > enh wrote:
> > > > > > > > > > > ZijunZhao wrote:
> > > > > > > > > > > > enh wrote:
> > > > > > > > > > > > > is this ever _not_ set for clang?
> > > > > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23
> > > > > > > > > > > > I think it is set?
> > > > > > > > > > > i get an error from
> > > > > > > > > > > ```
> > > > > > > > > > > /tmp$ cat x.c
> > > > > > > > > > > #if defined(__GNUC__)
> > > > > > > > > > > #error foo
> > > > > > > > > > > #endif
> > > > > > > > > > > ```
> > > > > > > > > > > regardless of whether i compile with -std=c11 or 
> > > > > > > > > > > -std=gnu11.
> > > > > > > > > > > neither -ansi nor -pedantic seem to stop it either.
> > > > > > > > > > it does look like it _should_ be possible to not have it 
> > > > > > > > > > set though? 
> > > > > > > > > > llvm/llvm-project/clang/lib/Frontend/InitPreprocessor.cpp 
> > > > > > > > > > has:
> > > > > > > > > > ```
> > > > > > > > > >   if (LangOpts.GNUCVersion != 0) {
> > > > > > > > > > // Major, minor, patch, are given two decimal places 
> > > > > > > > > > each, so 4.2.1 becomes
> > > > > > > > > > // 40201.
> > > > > > > > > > unsigned GNUCMajor = LangOpts.GNUCVersion / 100 / 100;
> > > > > > > > > > unsigned GNUCMinor = LangOpts.GNUCVersion / 100 % 100;
> > > > > > > > > > unsigned GNUCPatch = LangOpts.GNUCVersion % 100;
> > > > > > > > > > Builder.defineMacro("__GNUC__", Twine(GNUCMajor));
> > > > > > > > > > Builder.defineMacro("__GNUC_MINOR__", Twine(GNUCMinor));
> > > > > > > > > > Builder.defineMacro("__GNUC_PATCHLEVEL__", 
> > > > > > > > > > Twine(GNUCPatch));
> > > > > > > > > > Builder.defineMacro("__GXX_ABI_VERSION", "1002");
> > > > > > > > > > 
> > > > > > > > > > if (LangOpts.CPlusPlus) {
> > > > > > > > > >   Builder.defineMacro("__GNUG__", Twine(GNUCMajor));
> > > > > > > > > >   Builder.defineMacro("__GXX_WEAK__");
> > > > > > > > > > }
> > > > > > > > > >   }
> > > > > > > > > > ```
> > > > > > > > > /me wonders whether the right test here is actually `#if 
> > > > > > > > > __has_feature(__builtin_add_overflow)` (etc)...
> > > > > > > > > 
> > > > > > > > > but at this point, you definitely need an llvm person :-)
> > > > > > > > From 
> > > > > > > > https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins,
> > > > > > > >  we can check them with
> > > > > > > >  __has_builtin(__builtin_add_overflow) && 
> > > > > > > > __has_builtin(__builtin_sub_overflow) && 
> > > > > > > > __has_builtin(__builtin_mul_overflow).
> > > > > > > > I saw some code also checks if __GNUC__ >= 5:
> > > > > > > > 
> > > > > > > > // The __GNUC__ checks can not be removed until we depend on 
> > > > > > > > GCC >= 10.1
> > > > > > > > // which is the first version that returns true for 
> > > > > > > > __has_builtin(__builtin_add_overflow)
> > > > > > > > #if __GNUC__ >= 5 || __has_builtin(__builtin_add_overflow)
> > > > > > > > 
> > > > > > > > I guess we don't need to support real gcc using this header 
> > > > > > > > here. So maybe only checking __has_builtin is enough?
> > > > > > > > 
> > > > > > > > By the way, if __builtin_add_overflow may not appear on some 
> > > > > > > > targets, do we need to modify tests to specify triple like 
> > > > > > > > "-triple "x86_64-unknown-unknown"" in 
> > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/builtins-overflow.c#L5
> > > > > > > >  ?
> > > > > > > > 
> > > > > > > #ifndef __has_builtin // Optional of course.
> > > > > > >   #define __has_builtin(x) 0  // Compatibility with non-clang 
> > > > > > > compilers.
> > > > > > > #endif
> > > > > > > 
> > > > > > > ...
> > > > > > > #if __has_builtin(__builtin_trap)
> > > > > > >   __builtin_trap();
> > > > > > > #else
> > > > > > >   abort();
> > > > > > > #endif
> > > > > > > /me wonders whether the right test here is actually #if 
> > > > > > > __has_feature(__builtin_add_overflow) (etc)...
> > > > > > 
> > > > > > i think that should be added.
> > > > > > 
> > > > > > I guess we also need a with `__STDC_VERSION__ > 202000L`? in 
> > > > > > princple we'd have a C23 number for it but i'm not sure if that has 
> > > > > > been added to clang yet.
> > > > > > i think that should be added.
> > > > > 
> > > > > i was advising the opposite --- now this is a standard C23 feature, 
> > > > > any architectures where __builtin_*_overflow doesn't work need to be 
> > > > > found and fixed. and we'll do that quicker if we unconditionally 

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

> I try to grep "std>" in clang/test/Headers but find nothing, and nothing in 
> stdalign.h is about >

grep for `__STDC_VERSION__` instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/test/Headers/stdckdint.cpp:1
+// RUN: %clang_cc1 -emit-llvm -fgnuc-version=4.2.1 -std=gnu++11 %s -o - | 
FileCheck %s
+

ZijunZhao wrote:
> enh wrote:
> > ZijunZhao wrote:
> > > enh wrote:
> > > > hiraditya wrote:
> > > > > seems like we don't have a -std=gnu23, or -std=c23 standard flag for 
> > > > > this in clang yet.
> > > > > 
> > > > > https://godbolt.org/z/7dKnGEWWE
> > > > > 
> > > > > we probably need it before testing stdckdint i guess?
> > > > other headers just use > and the previous version. (though see 
> > > > stdalign.h if you're looking for some random cleanup to do!)
> > > > seems like we don't have a -std=gnu23, or -std=c23 standard flag for 
> > > > this in clang yet.
> > > 
> > > In the local testing, `-std=c++23` works  and all tests pass 
> > > 
> > > 
> > C23 != C++23... they don't even really coordinate with one another... talk 
> > to hboehm about that some time :-)
> ohhh I think `gnu++23` != `gnu23` either  
correct. the "c" or "c++" part means "standard stuff" and replacing it with 
"gnu" or "gnu++" means "standard stuff _and_ extensions".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/test/Headers/stdckdint.cpp:1
+// RUN: %clang_cc1 -emit-llvm -fgnuc-version=4.2.1 -std=gnu++11 %s -o - | 
FileCheck %s
+

ZijunZhao wrote:
> enh wrote:
> > hiraditya wrote:
> > > seems like we don't have a -std=gnu23, or -std=c23 standard flag for this 
> > > in clang yet.
> > > 
> > > https://godbolt.org/z/7dKnGEWWE
> > > 
> > > we probably need it before testing stdckdint i guess?
> > other headers just use > and the previous version. (though see stdalign.h 
> > if you're looking for some random cleanup to do!)
> > seems like we don't have a -std=gnu23, or -std=c23 standard flag for this 
> > in clang yet.
> 
> In the local testing, `-std=c++23` works  and all tests pass 
> 
> 
C23 != C++23... they don't even really coordinate with one another... talk to 
hboehm about that some time :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))

hiraditya wrote:
> xbolva00 wrote:
> > yabinc wrote:
> > > enh wrote:
> > > > enh wrote:
> > > > > enh wrote:
> > > > > > ZijunZhao wrote:
> > > > > > > enh wrote:
> > > > > > > > is this ever _not_ set for clang?
> > > > > > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23
> > > > > > > I think it is set?
> > > > > > i get an error from
> > > > > > ```
> > > > > > /tmp$ cat x.c
> > > > > > #if defined(__GNUC__)
> > > > > > #error foo
> > > > > > #endif
> > > > > > ```
> > > > > > regardless of whether i compile with -std=c11 or -std=gnu11.
> > > > > > neither -ansi nor -pedantic seem to stop it either.
> > > > > it does look like it _should_ be possible to not have it set though? 
> > > > > llvm/llvm-project/clang/lib/Frontend/InitPreprocessor.cpp has:
> > > > > ```
> > > > >   if (LangOpts.GNUCVersion != 0) {
> > > > > // Major, minor, patch, are given two decimal places each, so 
> > > > > 4.2.1 becomes
> > > > > // 40201.
> > > > > unsigned GNUCMajor = LangOpts.GNUCVersion / 100 / 100;
> > > > > unsigned GNUCMinor = LangOpts.GNUCVersion / 100 % 100;
> > > > > unsigned GNUCPatch = LangOpts.GNUCVersion % 100;
> > > > > Builder.defineMacro("__GNUC__", Twine(GNUCMajor));
> > > > > Builder.defineMacro("__GNUC_MINOR__", Twine(GNUCMinor));
> > > > > Builder.defineMacro("__GNUC_PATCHLEVEL__", Twine(GNUCPatch));
> > > > > Builder.defineMacro("__GXX_ABI_VERSION", "1002");
> > > > > 
> > > > > if (LangOpts.CPlusPlus) {
> > > > >   Builder.defineMacro("__GNUG__", Twine(GNUCMajor));
> > > > >   Builder.defineMacro("__GXX_WEAK__");
> > > > > }
> > > > >   }
> > > > > ```
> > > > /me wonders whether the right test here is actually `#if 
> > > > __has_feature(__builtin_add_overflow)` (etc)...
> > > > 
> > > > but at this point, you definitely need an llvm person :-)
> > > From 
> > > https://clang.llvm.org/docs/LanguageExtensions.html#checked-arithmetic-builtins,
> > >  we can check them with
> > >  __has_builtin(__builtin_add_overflow) && 
> > > __has_builtin(__builtin_sub_overflow) && 
> > > __has_builtin(__builtin_mul_overflow).
> > > I saw some code also checks if __GNUC__ >= 5:
> > > 
> > > // The __GNUC__ checks can not be removed until we depend on GCC >= 10.1
> > > // which is the first version that returns true for 
> > > __has_builtin(__builtin_add_overflow)
> > > #if __GNUC__ >= 5 || __has_builtin(__builtin_add_overflow)
> > > 
> > > I guess we don't need to support real gcc using this header here. So 
> > > maybe only checking __has_builtin is enough?
> > > 
> > > By the way, if __builtin_add_overflow may not appear on some targets, do 
> > > we need to modify tests to specify triple like "-triple 
> > > "x86_64-unknown-unknown"" in 
> > > https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/builtins-overflow.c#L5
> > >  ?
> > > 
> > #ifndef __has_builtin // Optional of course.
> >   #define __has_builtin(x) 0  // Compatibility with non-clang compilers.
> > #endif
> > 
> > ...
> > #if __has_builtin(__builtin_trap)
> >   __builtin_trap();
> > #else
> >   abort();
> > #endif
> > /me wonders whether the right test here is actually #if 
> > __has_feature(__builtin_add_overflow) (etc)...
> 
> i think that should be added.
> 
> I guess we also need a with `__STDC_VERSION__ > 202000L`? in princple we'd 
> have a C23 number for it but i'm not sure if that has been added to clang yet.
> i think that should be added.

i was advising the opposite --- now this is a standard C23 feature, any 
architectures where __builtin_*_overflow doesn't work need to be found and 
fixed. and we'll do that quicker if we unconditionally expose these and (more 
importantly!) run the tests.

> I guess we also need a with __STDC_VERSION__ > 202000L?

_personally_ i think that's silly because you can't hide the header file, so it 
doesn't make any sense to just have it empty if someone's actually #included 
it. we don't do this anywhere in bionic for example, for this reason. but 
obviously that's an llvm decision, and it does look like the other similar 
headers _do_ have this check, so, yeah, probably.



Comment at: clang/test/Headers/stdckdint.cpp:1
+// RUN: %clang_cc1 -emit-llvm -fgnuc-version=4.2.1 -std=gnu++11 %s -o - | 
FileCheck %s
+

hiraditya wrote:
> seems like we don't have a -std=gnu23, or -std=c23 standard flag for this in 
> clang yet.
> 
> https://godbolt.org/z/7dKnGEWWE
> 
> we probably need it before testing stdckdint i guess?
other headers just use > and the previous version. (though see stdalign.h if 
you're looking for some random cleanup to do!)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))

enh wrote:
> enh wrote:
> > ZijunZhao wrote:
> > > enh wrote:
> > > > is this ever _not_ set for clang?
> > > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23
> > > I think it is set?
> > i get an error from
> > ```
> > /tmp$ cat x.c
> > #if defined(__GNUC__)
> > #error foo
> > #endif
> > ```
> > regardless of whether i compile with -std=c11 or -std=gnu11.
> > neither -ansi nor -pedantic seem to stop it either.
> it does look like it _should_ be possible to not have it set though? 
> llvm/llvm-project/clang/lib/Frontend/InitPreprocessor.cpp has:
> ```
>   if (LangOpts.GNUCVersion != 0) {
> // Major, minor, patch, are given two decimal places each, so 4.2.1 
> becomes
> // 40201.
> unsigned GNUCMajor = LangOpts.GNUCVersion / 100 / 100;
> unsigned GNUCMinor = LangOpts.GNUCVersion / 100 % 100;
> unsigned GNUCPatch = LangOpts.GNUCVersion % 100;
> Builder.defineMacro("__GNUC__", Twine(GNUCMajor));
> Builder.defineMacro("__GNUC_MINOR__", Twine(GNUCMinor));
> Builder.defineMacro("__GNUC_PATCHLEVEL__", Twine(GNUCPatch));
> Builder.defineMacro("__GXX_ABI_VERSION", "1002");
> 
> if (LangOpts.CPlusPlus) {
>   Builder.defineMacro("__GNUG__", Twine(GNUCMajor));
>   Builder.defineMacro("__GXX_WEAK__");
> }
>   }
> ```
/me wonders whether the right test here is actually `#if 
__has_feature(__builtin_add_overflow)` (etc)...

but at this point, you definitely need an llvm person :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))

enh wrote:
> ZijunZhao wrote:
> > enh wrote:
> > > is this ever _not_ set for clang?
> > https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23
> > I think it is set?
> i get an error from
> ```
> /tmp$ cat x.c
> #if defined(__GNUC__)
> #error foo
> #endif
> ```
> regardless of whether i compile with -std=c11 or -std=gnu11.
> neither -ansi nor -pedantic seem to stop it either.
it does look like it _should_ be possible to not have it set though? 
llvm/llvm-project/clang/lib/Frontend/InitPreprocessor.cpp has:
```
  if (LangOpts.GNUCVersion != 0) {
// Major, minor, patch, are given two decimal places each, so 4.2.1 becomes
// 40201.
unsigned GNUCMajor = LangOpts.GNUCVersion / 100 / 100;
unsigned GNUCMinor = LangOpts.GNUCVersion / 100 % 100;
unsigned GNUCPatch = LangOpts.GNUCVersion % 100;
Builder.defineMacro("__GNUC__", Twine(GNUCMajor));
Builder.defineMacro("__GNUC_MINOR__", Twine(GNUCMinor));
Builder.defineMacro("__GNUC_PATCHLEVEL__", Twine(GNUCPatch));
Builder.defineMacro("__GXX_ABI_VERSION", "1002");

if (LangOpts.CPlusPlus) {
  Builder.defineMacro("__GNUG__", Twine(GNUCMajor));
  Builder.defineMacro("__GXX_WEAK__");
}
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))

ZijunZhao wrote:
> enh wrote:
> > is this ever _not_ set for clang?
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Headers/stdbool.h#L23
> I think it is set?
i get an error from
```
/tmp$ cat x.c
#if defined(__GNUC__)
#error foo
#endif
```
regardless of whether i compile with -std=c11 or -std=gnu11.
neither -ansi nor -pedantic seem to stop it either.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D157331: [clang] Implement C23

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Headers/stdckdint.h:13
+
+#if defined(__GNUC__)
+#define ckd_add(R, A, B) __builtin_add_overflow((A), (B), (R))

is this ever _not_ set for clang?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157331

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

[PATCH] D150490: Enable frame pointer for all non-leaf functions on riscv64 Android

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

In D150490#4343128 , @hiraditya wrote:

>> Is there more context on why Android enables the frame pointer?
>
> From what i gathered, this is more of an effort to have parity such that 
> existing build flag overrides continue to be consistent.

well, when i said that on the internal chat, i thought you were asking "why do 
we say what clang already says?" :-)

if the question was actually "is there more context on why Android enables the 
frame pointer?" i'd have said something like "because Android developers [OS 
and app developers alike] do so much debugging from the field, where all we get 
is a crash report for something we probably can't repro locally, that having 
good _and_ cheap unwinds is super important to us".


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

https://reviews.llvm.org/D150490

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

[PATCH] D150490: Enable frame pointer for all non-leaf functions on riscv64 Android

[Elliott Hughes via Phabricator via cfe-commits]

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

(lgtm with craig.topper's suggested simplification.)




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:530
+   Triple.isAArch64() || Triple.isPS() || Triple.isVE() ||
+   (Triple.isAndroid() && (Triple.getArch() == 
llvm::Triple::riscv64)));
   if (NoOmitFP || mustUseNonLeafFramePointerForTarget(Triple) ||

hiraditya wrote:
> enh wrote:
> > how does this work for Android/arm64?
> becaues of `Triple.isAArch64()`, AArch64 always has non-leaf frame pointers 
> for all platforms.
lol. i could seem so much smarter if only i'd learn to read :-)


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

https://reviews.llvm.org/D150490

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

[PATCH] D150490: Enable frame pointer for all non-leaf functions on riscv64 Android

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:424
+  if (Triple.isAndroid()) {
+// AArch64 has frame pointers enabled for non-leaf functions.
+switch (Triple.getArch()) {

(where? is it simpler to just add arm64 to the switch?)



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:530
+   Triple.isAArch64() || Triple.isPS() || Triple.isVE() ||
+   (Triple.isAndroid() && (Triple.getArch() == 
llvm::Triple::riscv64)));
   if (NoOmitFP || mustUseNonLeafFramePointerForTarget(Triple) ||

how does this work for Android/arm64?


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

https://reviews.llvm.org/D150490

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

[PATCH] D150490: Enable frame pointer for all non-leaf functions on riscv64 Android

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:423
 
+  if (Triple.isAndroid() && Triple.getArch() == llvm::Triple::riscv64)
+return true;

should this...



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:439
   case llvm::Triple::riscv32:
   case llvm::Triple::riscv64:
   case llvm::Triple::sparc:

(ah, this is why you need Android early. but, yeah, probably worth moving all 
the Android stuff together, like with the other OSes?)



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:459
   Triple.isOSHurd()) {
 switch (Triple.getArch()) {
 // Don't use a frame pointer on linux if optimizing for certain targets.

...be down here instead?



Comment at: clang/lib/Driver/ToolChains/Clang.cpp:465
 case llvm::Triple::thumbeb:
   if (Triple.isAndroid())
 return true;

(take care of all this android/arm stuff where you take care of 
android/riscv64? arm64 has frame pointers by "default default", so we don't 
need to mention it?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150490

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

[PATCH] D148779: [Sema] Fix spurious warning for printf("%lb", (long)10)

[Elliott Hughes via Phabricator via cfe-commits]

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

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148779

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

[PATCH] D137268: [clang][Headers] Do not define varargs macros for __need___va_list

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

In D137268#4069992 , @zatrazz wrote:

> Could you check if this fixes your issue?

yes, thanks... the person doing the llvm update tried it and reports that it 
works.

here's the patch against our old copy of glibc: 
https://android-review.googlesource.com/c/platform/prebuilts/gcc/linux-x86/host/x86_64-linux-glibc2.17-4.8/+/2403274


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137268

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

[PATCH] D137268: [clang][Headers] Do not define varargs macros for __need___va_list

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

In D137268#4069856 , @zatrazz wrote:

> I think I have caught this because your standard conformance tests checks for 
> __gnuc_va_list 
> on wchar.h, wich is always defined on on GCC (git log shows it was changed to 
> fix XPG7 
> tests, but I am not sure exactly why the author has changed the va_list to 
> __gnuc_va_list).

bionic's tests check for `va_list`, because that's what POSIX says will be 
visible. ISO C doesn't say that, so i think the _intention_ for glibc -- musl 
seems to do this correctly -- is to say "if we're only compiling C source, you 
don't get `va_list`, but if we're compiling POSIX source, you do get 
`va_list`". so i think this `__gnuc_va_list` thing is their workaround to still 
export the _functions_ without exporting the _type_ for ISO C?

here's the bionic *POSIX* test: 
https://cs.android.com/android/platform/superproject/+/master:bionic/tests/headers/posix/wchar_h.c;l=38
 (but note that you'll have to look at the corresponding Android.bp file to see 
us define `_POSIX_SOURCE`.) note that our tests pass against _bionic_ (and, i 
think, musl). it's just old-glibc-with-new-llvm they fail against.

> And it seems that glibc seems broken also using GCC stdarg.h if I fix the 
> test to check for
> va_list instead. I will take a look at this.

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137268

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

[PATCH] D137268: [clang][Headers] Do not define varargs macros for __need___va_list

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

is there a corresponding glibc change so that `va_list` is exported for 
`_POSIX_SOURCE` cases? see 
https://android-review.git.corp.google.com/c/platform/bionic/+/2397313 where 
i'm having to disable some bionic testing against glibc because the glibc 
(2.17!)  now no longer exports `va_list`. i did look for a ToT glibc 
patch to backport (until we've _actually_ switched from glibc to musl for the 
host), but couldn't obviously find it?

https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/basedefs/wchar.h.html
 says:
"""
The  header shall define the following types:
...
va_list
[CX] As described in .
"""
which is why i think our " exports `va_list`" test is correct. 
(Android doesn't support an "ISO only" mode --- you're effectively always in 
`_POSIX_SOURCE` mode, so we build the test against glibc with `_POSIX_SOURCE` 
defined.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137268

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

[PATCH] D131057: [Sema] -Wformat: support C23 format specifier %b %B

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

In D131057#3697392 , @MaskRay wrote:

> In D131057#3697095 , @dim wrote:
>
>>> GCC 12 -Wformat -pedantic emits a warning:
>>>
>>>   warning: ISO C17 does not support the ‘%b’ gnu_printf format [-Wformat=]
>>>
>>> The behavior is not ported (and it's unclear to me how to implement it).
>>
>> If you really want this, I think it can be implemented by looking at 
>> `LangOpts::LangStd`.
>
> Something like `!getLangOpts().C2X` I suppose, but I do not find how to check 
> both `-Wformat` and `-Wpedantic`.
> Also, for the nature of the diagnostic, I think something in TableGen like 
> `def ext_... InGroup` would make sense but using a C23/C2x related -W 
> option would deviate from the GCC behavior.
> ISTM adding the diagnostic (even if we do) is not so necessary in this patch.

note that that's not even what we'd want for Android anyway ... availability 
and behavior on Android is always[1] predicated on the API level, not the C 
standard. app developers can't avoid dealing with the API level, so we've 
avoided having an orthogonal versioning concern for them. (this is true in the 
headers too, not just behavior: on Android you always get the 
API-level-appropriate _library_ no matter what version of the language you 
select.)

and, like i said elsewhere, no-one's ever noticed that clang just assumes `%m` 
is always available. even though _i've_ wanted %b since i was a kid[2], i'm not 
actually expecting %b will be heavily used in practice. (tbh, i doubt %b will 
ever be used in `scanf()` outside of tests. as far as i'm concerned only the 
`printf()` side is actually useful...)



1. the removal of `gets()` is the only exception that springs to mind.
2. even though i know that for weird-ass machines like the PDP series, octal 
was more useful than it is to the rest of us, i still can't believe %b wasn't 
in B, let alone C!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131057

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

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

> I think making scanf in the same patch makes sense. Let me check existing 
> tests...

fwiw, most of the libcs seem to have been doing these separately because scanf 
is harder. i hope to have bionic's scanf done this week though.

(note in case you haven't already that there is no %B for scanf, just %b...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131057

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

[PATCH] D131057: [Sema] -Wformat: support C23 printf %b %B

[Elliott Hughes via Phabricator via cfe-commits]

enh accepted this revision.
enh added a comment.

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131057

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

[PATCH] D117611: [Sema] Warn about printf %n on Android and Fuchsia

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

In D117611#3339137 , @glandium wrote:

> This doesn't leave much room to use `__attribute__((format(printf)))` on 
> custom printf implementations that do support `%n` on Android does it?

it would be pretty hard to get into that situation though? apps are clones of 
the zygote, so you don't have any choice over your libc on Android: it's all 
bionic, all the time. (our seccomp policies also mean "good luck trying to run 
a static musl/glibc binary".) clang's Android target already assumes bionic in 
other ways --- things like whether math functions set errno, or whatever, so 
this seems in keeping to me.

ah, you don't mean *printf* implementations, you mean "other functions that 
take printf arguments"? yeah, that's a more interesting case. though that one's 
already broken: there's already no way to say what subset of printf formats you 
do/don't support. (all the ones in the Android OS itself are all subsets of the 
bionic printf; many equal, but several *much* smaller. that would be a pretty 
cool thing to be able to specify, but that's a much bigger bug, and this change 
brings us closer to reality than we currently are, for the 99% case :-) )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117611

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

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

In D111833#3094868 , @mbenfield wrote:

> Previously this patch did not cover %c and %[, but now it does.

thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111833

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

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

[Elliott Hughes via Phabricator via cfe-commits]

enh accepted this revision.
enh added a comment.

add tests for %c and %[ too? (it's genuinely unclear to me from a quick skim 
whether this patch covers them.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111833

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

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

[Elliott Hughes via Phabricator via cfe-commits]

enh added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:662
+if (Index < FD->getNumParams()) {
+  if (const auto *POS =
+  FD->getParamDecl(Index)->getAttr())

mbenfield wrote:
> enh wrote:
> > (stray tabs?)
> Not sure what you're referring to. AFAICT there are no tabs in this file. 
oh, gerrit uses the >> symbol to mean "tab". is this tool just saying 
"indentation changed", not specifically "someone added a tab"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111833

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

[PATCH] D111833: [clang] Fortify warning for scanf calls with field width too big.

[Elliott Hughes via Phabricator via cfe-commits]

enh added a comment.

"shut up and take my money!" :-)




Comment at: clang/lib/Sema/SemaChecking.cpp:662
+if (Index < FD->getNumParams()) {
+  if (const auto *POS =
+  FD->getParamDecl(Index)->getAttr())

(stray tabs?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111833

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