[PATCH] D140562: [clang][ASTImporter] Improve import of InjectedClassNameType.

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: vabridgers.
whisperity added a comment.

@vabridgers Please take a look at this, as per off-list discussion, in relation 
to D142822 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140562

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Oddly enough, one of the buildbots caught a not matching test that was working 
locally... on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

Alright, right now, I have no meaningful idea why this failure appears 
,
 locally I'm trying every test command as they appear in the test, and all the 
tests are passing. It's as if on that system the whole //Annex K// support 
would not be allowed. Locally I added a few debug prints and I'm getting the 
right answers for //"Whether Annex K is allowed?"//.

@njames93 @aaron.ballman Do you have any idea what could make that test fail in 
that system?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a subscriber: dyung.
whisperity added a comment.

Ping @dyung, it looks like you're the owner of the relevant build-bot. I can't 
find any information what `x86_64-sie` is...

Meanwhile, it seems my attempt at fix is not actually fixing anything, I'm 
trying to figure out how to at least disable the check on this specific 
architecture. The rest of the architectures seem to be passing normally as 
expected. Something must be special within Clang's default environment or 
compiler configuration...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

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

Alright, I've done a full reanalysis after a rebase. Overhead is not 
meaningfully measurable, even for complex TUs such as LLVM. The check is viable 
in C++ code as it finds cases (such as the one described in LLVM, but also 
Bitcoin is a primarily C++ project), so I won't rework the check to disable it 
in C++ mode explicitly. It seems the name lookup is implemented pretty well, 
helped by the fact that the names to match are short. No crashes had been 
observed in the test projects; let's hope it stays the same way; the matchers 
themselves are simple enough. The //Annex K.// matcher is only registered if in 
>= C11 mode.
I've also gone through the discussion earlier, and it looks to me as if all the 
concerns were either resolved or made obsolete due to the evolution of the 
check.

I did make several **NFC** changes to the patch in post-production, such as 
fixing the documentation here and there, ensuring that the `cert-` aliases are 
appropriately documented, and a little bit of refactoring so internal details 
of the check are genuinely internal. Thus, I'll be committing a version that is 
different from the one here.


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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf27c8ac83e7c: [clang-tidy] Add the 
`bugprone-unsafe-functions` check (authored by futogergely, committed by 
whisperity).

Changed prior to commit:
  https://reviews.llvm.org/D91000?vs=485774=494265#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

Files:
  clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.cpp
  clang-tools-extra/clang-tidy/bugprone/UnsafeFunctionsCheck.h
  clang-tools-extra/clang-tidy/cert/CERTTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/bugprone/unsafe-functions.rst
  clang-tools-extra/docs/clang-tidy/checks/cert/msc24-c.rst
  clang-tools-extra/docs/clang-tidy/checks/cert/msc33-c.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c

Index: clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/bugprone/unsafe-functions.c
@@ -0,0 +1,154 @@
+// RUN: %check_clang_tidy -check-suffix=WITH-ANNEX-K%s bugprone-unsafe-functions %t -- -- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -U__STDC_LIB_EXT1__   -U__STDC_WANT_LIB_EXT1__
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -D__STDC_LIB_EXT1__=1 -U__STDC_WANT_LIB_EXT1__
+// RUN: %check_clang_tidy -check-suffix=WITHOUT-ANNEX-K %s bugprone-unsafe-functions %t -- -- -U__STDC_LIB_EXT1__   -D__STDC_WANT_LIB_EXT1__=1
+// RUN: %check_clang_tidy -check-suffix=WITH-ANNEX-K-CERT-ONLY  %s bugprone-unsafe-functions %t -- \
+// RUN:   -config="{CheckOptions: [{key: bugprone-unsafe-functions.ReportMoreUnsafeFunctions, value: false}]}" \
+// RUN:-- -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1
+
+typedef __SIZE_TYPE__ size_t;
+typedef __WCHAR_TYPE__ wchar_t;
+
+char *gets(char *S);
+size_t strlen(const char *S);
+size_t wcslen(const wchar_t *S);
+
+void f1(char *S) {
+  gets(S);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'gets_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'gets_s' should be used instea
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K::[[@LINE-3]]:3: warning: function 'gets' is insecure, was deprecated and removed in C11 and C++14; 'fgets' should be used instead
+
+  strlen(S);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'strlen' is not bounds-checking; 'strnlen_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'strlen' is not bounds-checking; 'strnlen_s' should be used instead
+  // no-warning WITHOUT-ANNEX-K
+}
+
+void f1w(wchar_t *S) {
+  wcslen(S);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'wcslen' is not bounds-checking; 'wcsnlen_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'wcslen' is not bounds-checking; 'wcsnlen_s' should be used instead
+  // no-warning WITHOUT-ANNEX-K
+}
+
+struct tm;
+char *asctime(const struct tm *TimePtr);
+
+void f2(const struct tm *Time) {
+  asctime(Time);
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:3: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:3: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K::[[@LINE-3]]:3: warning: function 'asctime' is not bounds-checking and non-reentrant; 'strftime' should be used instead
+
+  char *(*F1)(const struct tm *) = asctime;
+  // CHECK-MESSAGES-WITH-ANNEX-K:   :[[@LINE-1]]:36: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITH-ANNEX-K-CERT-ONLY: :[[@LINE-2]]:36: warning: function 'asctime' is not bounds-checking and non-reentrant; 'asctime_s' should be used instead
+  // CHECK-MESSAGES-WITHOUT-ANNEX-K::[[@LINE-3]]:36: warning: function 'asctime' is not bounds-checking and non-reentrant; 'strftime' should be used instead
+
+  char *(*F2)(const struct tm *) = 
+  // CHECK-MESSAGES-WITH-ANNEX-K:   

[PATCH] D142822: [clang] ASTImporter: Fix importing of va_list types and declarations

[Whisperity via Phabricator via cfe-commits]

whisperity added subscribers: rsmith, whisperity.
whisperity added a comment.

In D142822#4095896 , @DavidSpickett 
wrote:

>   
> /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:14:18:
>  error: reference to 'std' is ambiguous
>   void f(str> ) {
>^
>   
> /home/david.spickett/llvm-project/clang/test/CodeGenCXX/cxx20-module-std-subst-2b.cpp:7:11:
>  note: candidate found by name lookup is 'std'
>   namespace std {
> ^

This looks like an ouchie of the highest order. I've been looking into 
Modules... 5-ish years ago, when it was still under design. There are two major 
concerns here, one is that we've been having a feature called //"Modules"// 
which was very Clang-specific (but a good proof-of-concept spiritual 
predecessor of what became the Standard Modules), and there are the standard 
modules. Now the file name explicitly mentions `cxx20` so this is likely about 
Standard Modules stuff. But to be fair, I'd take this implementation with a 
pinch of salt. Due to the lack of build-system support for Modules (there are 
some murmurs these days on the CMake GitLab about very soon adding meaningful 
support for this...) we do not really seem to have large-scale real-world 
experience as to how the standard really could be made working, **especially** 
in such a weird case like touching `namespace std` which //might// not be 
standards-compliant in the first place. (But yeah, we are test code here in a 
compiler; we can cut ourselves some slack.)

The code itself in these test files is really weird; it seems the test is 
centred around the idea that there are:

  // TU "A"
  module; // Code in the *global module fragment*
  namespace std { /* ... */ class basic_string; }
  
  export module Whatever; // Code in the public module interface unit of module 
Whatever.
  export /* ... */ using str = std::basic_string<...>;

So we have a symbol called `str`, which is publicly visible (to TUs importing 
`Whatever`), and the `namespace std` preceding `Whatever` in `TU "A"` is 
**not** visible but **reachable**? (The standard's way of expressing 
reachability is a kind of a mess , so I'd 
**definitely** try finding the people who worked in developing the actual 
solution that makes Standard Modules //work// in Clang... call the help of 
someone who's really expert on the Standard...) Perhaps @rsmith could help us 
clarify this situation.

You can't name `std::basic_string<...>` in the client code (because it is not 
//visible//), but you can depend on the symbol (e.g., `decltype` it to an 
alias!) because it is //reachable//!

And then you have

  // TU "B"
  import Whatever;
  
  namespace std { /* ... */ struct char_traits {}; }
  
  // use Sb mangling, not Ss, as this is not global-module std::char_traits
  /* ... */
  void f(str<..., std::char_traits<...>>)

In which there is "another" (?) `namespace std`, which should(??) also be part 
of the global module :

> The //global module// is the collection of all //global-module-fragments// 
> and all translation units that are not module units. Declarations appearing 
> in such a context are said to be in the //purview// of the global module.

I believe that `TU "B"` is **not** a module unit... But the comment (which is 
somehow misformatted?) in it would like to say that this `std` is, in fact, 
//not// part of the global module.

So somehow, the changes in this patch are making //both// `std`s part of the 
same //purview// (whatever that means, really...), thus resulting in ambiguity.

- Does this only happen on non-X86? @vabridgers, you could try adding a 
`--target=` flag locally in the test file to the compiler invocation and 
re-running the tests to verify. (Might need an LLVM build that is capable of 
generating code of other architectures.)
- My other suggestion would be putting some `->dump()` calls in your change, 
running the code and observing how the ASTs are looking in the states when 
execution is within the functions you're trying to change.




Comment at: clang/test/AST/ast-dump-traits.cpp:55
 // CHECK-NEXT: | `-ExpressionTraitExpr {{.*}}  'bool' 
__is_lvalue_expr
-// CHECK-NEXT: `-FunctionDecl {{.*}}  line:30:6{{( 
imported)?}} test_unary_expr_or_type_trait 'void ()'
+// CHECK-NEXT:  -FunctionDecl {{.*}}  line:30:6{{( 
imported)?}} test_unary_expr_or_type_trait 'void ()'
 // CHECK-NEXT:   `-CompoundStmt {{.*}} 

How is this change legit? Is this `FunctionDecl` node just "floating"? If so, 
why is there a `-` preceding in the textual dump? Usually top-level nodes do 
not have such a prefix.



Comment at: clang/test/AST/fixed_point.c:405
 
-//CHECK-NEXT: `-VarDecl {{.*}} literallast '_Accum' cinit
+//CHECK-NEXT:  -VarDecl {{.*}} literallast '_Accum' cinit
 //CHECK-NEXT:   `-FixedPointLiteral {{.*}} '_Accum' 1.0

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#4099669 , @dyung wrote:

>   -DLLVM_DEFAULT_TARGET_TRIPLE=x86_64-scei-ps4
>
> Hopefully this can help you to reproduce the problem.

Ah, thank you very much. And indeed, giving `--target=x86_64-scei-ps4` to 
Clang-Tidy will make the test case fail, but using `x86_64-linux-gnu` works 
perfectly.
Now knowing the exact triple I think I can delve into the preprocessor stack or 
something to figure out why the //Annex K.//-related flags, my best bet is that 
it's explicitly forbidden from being defined. 

  clang-tidy 
/tmp/LLVM-Build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/unsafe-functions.c.tmp.c
 --checks='-*,bugprone-unsafe-functions' -config={} -- 
--target=x86_64-linux-gnu -D__STDC_LIB_EXT1__=1 -D__STDC_WANT_LIB_EXT1__=1 
-nostdinc++

My first wish is to figure out how to reliably disable the test case and have 
this quick-fix pushed immediately so the buildbots don't continue failing, and 
then we can figure out that this check should be disabled on this specific 
platform, or something like that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D91000: [clang-tidy] Add bugprone-unsafe-functions checker.

[Whisperity via Phabricator via cfe-commits]

whisperity added a comment.

In D91000#4099773 , @dyung wrote:

> To XFAIL the test, grep for lines with "XFAIL" and "ps4" and you should find 
> some examples. It was recently changed how it worked lately.

Thank you, yes I found an example. I went with `UNSUPPORTED` instead of `XFAIL` 
because the test implements 4 or 5 separate cases and `XFAIL`ing it all would 
make some cases become //"Unexpectedly passed"//... I did a quick commit with 
marking it as unsupported with a bit of an explanation in the code, and now the 
buildbots are coming back green.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91000

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

[PATCH] D146712: [clang-tidy] Add portability-non-portable-integer-constant check

[Whisperity via Phabricator via cfe-commits]

whisperity requested changes to this revision.
whisperity added a comment.

Earlier (IIRC in March) we did an internal test of the check and the following 
results were obtained. The results are kinda //weird//, to say the least. 
Numbers are //after// CodeChecker has done a serverside uniqueing (most likely 
things such as //"if **`X`** in `H.h` is reported in the compilation/analysis 
of `A.cpp` and `B.cpp` then store **`X`** only once"//) of the bugs.

- memcache-d: 43
- tmux: 4
- curl: 283
- twin: 161
- vim: 2 206
- OpenSSL: **23 724**
- sqlite: 752
- ffmpeg: **87 121**
- postgres: **116 635**
- tinyxml2: 6
- libwebm: 34
- xerces: **48 732**
- bitcoin: 4 429
- protobuf: 1 405
- codechecker-ldlogger: 0
- ACID: 4 831
- Qt: **34 973**
- Contour v2: **14 246**
- LLVM: 6 018

No crashes (of the checker) were observed. Most of the large outlier numbers 
are attributable to heavy mathematics (ffmpeg, Qt), crypto (OpenSSL, 
PostgreSQL), or character encoding (Xerces, Postgres, Contour) operations in 
the project.

Unfortunately, this is a very significant amount of false positives to the 
point where the consumption of results is slowed down not on the mental load 
side, but also the technical side (such as triggering server hangs in 
CodeChecker and whatnot...). However, after putting some thoughts into it and 
trying to hand-clusterise some of the reports I've found a few significant 
opportunities where this check should be improved.

**Note:** These improvements might be worthwhile to implement as a //follow-up 
patch// which is merged together with this current review. **As it stands right 
now, this check is way too noisy (on maths-based projects) to be merged!**

1. Enums


One is when the literal is used to initialise an enum. So far I don't know 
whether simply just ignoring enums is good, or would it be worth to somehow 
emit a "unified" warning for the entire enum that follows the pattern as shown 
below.
F28149214: image.png 

2. `(u)?int([\d]+)_t`
-

The other are cases when the user explicitly initialises a **fixed-width** (or 
*constrained-width*) variable, such as `int64_t` or `uint16_t` with a literal. 
Right now, the implementation hard matches only the `IntegerLiteral` instances, 
i.e., lines such as `static constexpr std::uint64_t X = 0x07F00` where the 
width and characteristics of the literal might just as well *exactly* and 
*properly* match the variable it is assigned to (granted, we only match and 
silence `Decl`s for this, and lose expressivity on expressions), and do not 
report then.

3. Large arrays of magic literals
-

While this is not the silencing of a "valid" use case where the check, as of 
the current implementation, it is too verbose for, but I believe we could 
reasonably cull the noisiness of the check by simply saying that if there are 
array initialisers or //brace-init-lists// or whatever that contain multiple 
non-portable literals, then **either** we simply suck it up and do not warn for 
**any** of the elements... or for the sake of collegiality give **one** warning 
for the user. The former option would be the most silencing of all, while the 
latter would allow the user to do a single `// NOLINT` for the array instead of 
having to deal with `/* NOLINTBEGIN */ ... /* NOLINTEND */`.

Several chunks of noisy warnings come out of places like SHA-256, FFT, and 
various other "very mathematical" contexts where a large constant array is 
created with very specific proven values, all of which expressed and reported 
as "non-portable literals". It is no use to do 50 warnings for a single 
array. (This is perhaps after all the same(-ish) family as the "enum" problem.)




Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:87
+  } else {
+return { StrippedLiteral, 0, 0, 10 };
+  }

(Unrealistic nit: This will consume hypothetical additional literals such as 
`0!ZZZbb=` as if they were decimal literals.)



Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.cpp:153-154
+ "non-portable integer literal: integer literal with leading zeroes");
+  // Matches only the most significant bit,
+  // eg. unsigned value 0x8000.
+  } else if (IsMSBBitUsed) {

(Style nit: These lines could be joined and still fit 80-col, I think.)



Comment at: 
clang-tools-extra/clang-tidy/portability/NonPortableIntegerConstantCheck.h:16
+
+/// Finds integer literals that are being used in a non-portable manner.
+///

I am still not entirely convinced whether this one-line summary makes too much 
sense to a user who does not know the Standard out of heart... As we hunt for 
bit widths more or less perhaps we could weave this notion into the single 
sentence summary saying that integers with "non-portable