Commit Graph

27 Commits

Author SHA1 Message Date
Matheus Izvekov 15f3cd6bfc
[clang] Implement ElaboratedType sugaring for types written bare
Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.

The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.

An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.

---

Troubleshooting list to deal with any breakage seen with this patch:

1) The most likely effect one would see by this patch is a change in how
   a type is printed. The type printer will, by design and default,
   print types as written. There are customization options there, but
   not that many, and they mainly apply to how to print a type that we
   somehow failed to track how it was written. This patch fixes a
   problem where we failed to distinguish between a type
   that was written without any elaborated-type qualifiers,
   such as a 'struct'/'class' tags and name spacifiers such as 'std::',
   and one that has been stripped of any 'metadata' that identifies such,
   the so called canonical types.
   Example:
   ```
   namespace foo {
     struct A {};
     A a;
   };
   ```
   If one were to print the type of `foo::a`, prior to this patch, this
   would result in `foo::A`. This is how the type printer would have,
   by default, printed the canonical type of A as well.
   As soon as you add any name qualifiers to A, the type printer would
   suddenly start accurately printing the type as written. This patch
   will make it print it accurately even when written without
   qualifiers, so we will just print `A` for the initial example, as
   the user did not really write that `foo::` namespace qualifier.

2) This patch could expose a bug in some AST matcher. Matching types
   is harder to get right when there is sugar involved. For example,
   if you want to match a type against being a pointer to some type A,
   then you have to account for getting a type that is sugar for a
   pointer to A, or being a pointer to sugar to A, or both! Usually
   you would get the second part wrong, and this would work for a
   very simple test where you don't use any name qualifiers, but
   you would discover is broken when you do. The usual fix is to
   either use the matcher which strips sugar, which is annoying
   to use as for example if you match an N level pointer, you have
   to put N+1 such matchers in there, beginning to end and between
   all those levels. But in a lot of cases, if the property you want
   to match is present in the canonical type, it's easier and faster
   to just match on that... This goes with what is said in 1), if
   you want to match against the name of a type, and you want
   the name string to be something stable, perhaps matching on
   the name of the canonical type is the better choice.

3) This patch could expose a bug in how you get the source range of some
   TypeLoc. For some reason, a lot of code is using getLocalSourceRange(),
   which only looks at the given TypeLoc node. This patch introduces a new,
   and more common TypeLoc node which contains no source locations on itself.
   This is not an inovation here, and some other, more rare TypeLoc nodes could
   also have this property, but if you use getLocalSourceRange on them, it's not
   going to return any valid locations, because it doesn't have any. The right fix
   here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive
   into the inner TypeLoc to get the source range if it doesn't find it on the
   top level one. You can use getLocalSourceRange if you are really into
   micro-optimizations and you have some outside knowledge that the TypeLocs you are
   dealing with will always include some source location.

4) Exposed a bug somewhere in the use of the normal clang type class API, where you
   have some type, you want to see if that type is some particular kind, you try a
   `dyn_cast` such as `dyn_cast<TypedefType>` and that fails because now you have an
   ElaboratedType which has a TypeDefType inside of it, which is what you wanted to match.
   Again, like 2), this would usually have been tested poorly with some simple tests with
   no qualifications, and would have been broken had there been any other kind of type sugar,
   be it an ElaboratedType or a TemplateSpecializationType or a SubstTemplateParmType.
   The usual fix here is to use `getAs` instead of `dyn_cast`, which will look deeper
   into the type. Or use `getAsAdjusted` when dealing with TypeLocs.
   For some reason the API is inconsistent there and on TypeLocs getAs behaves like a dyn_cast.

5) It could be a bug in this patch perhaps.

Let me know if you need any help!

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Differential Revision: https://reviews.llvm.org/D112374
2022-07-27 11:10:54 +02:00
Jonas Devlieghere 888673b6e3
Revert "[clang] Implement ElaboratedType sugaring for types written bare"
This reverts commit 7c51f02eff because it
stills breaks the LLDB tests. This was  re-landed without addressing the
issue or even agreement on how to address the issue. More details and
discussion in https://reviews.llvm.org/D112374.
2022-07-14 21:17:48 -07:00
Matheus Izvekov 7c51f02eff
[clang] Implement ElaboratedType sugaring for types written bare
Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.

The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.

An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.

---

Troubleshooting list to deal with any breakage seen with this patch:

1) The most likely effect one would see by this patch is a change in how
   a type is printed. The type printer will, by design and default,
   print types as written. There are customization options there, but
   not that many, and they mainly apply to how to print a type that we
   somehow failed to track how it was written. This patch fixes a
   problem where we failed to distinguish between a type
   that was written without any elaborated-type qualifiers,
   such as a 'struct'/'class' tags and name spacifiers such as 'std::',
   and one that has been stripped of any 'metadata' that identifies such,
   the so called canonical types.
   Example:
   ```
   namespace foo {
     struct A {};
     A a;
   };
   ```
   If one were to print the type of `foo::a`, prior to this patch, this
   would result in `foo::A`. This is how the type printer would have,
   by default, printed the canonical type of A as well.
   As soon as you add any name qualifiers to A, the type printer would
   suddenly start accurately printing the type as written. This patch
   will make it print it accurately even when written without
   qualifiers, so we will just print `A` for the initial example, as
   the user did not really write that `foo::` namespace qualifier.

2) This patch could expose a bug in some AST matcher. Matching types
   is harder to get right when there is sugar involved. For example,
   if you want to match a type against being a pointer to some type A,
   then you have to account for getting a type that is sugar for a
   pointer to A, or being a pointer to sugar to A, or both! Usually
   you would get the second part wrong, and this would work for a
   very simple test where you don't use any name qualifiers, but
   you would discover is broken when you do. The usual fix is to
   either use the matcher which strips sugar, which is annoying
   to use as for example if you match an N level pointer, you have
   to put N+1 such matchers in there, beginning to end and between
   all those levels. But in a lot of cases, if the property you want
   to match is present in the canonical type, it's easier and faster
   to just match on that... This goes with what is said in 1), if
   you want to match against the name of a type, and you want
   the name string to be something stable, perhaps matching on
   the name of the canonical type is the better choice.

3) This patch could exposed a bug in how you get the source range of some
   TypeLoc. For some reason, a lot of code is using getLocalSourceRange(),
   which only looks at the given TypeLoc node. This patch introduces a new,
   and more common TypeLoc node which contains no source locations on itself.
   This is not an inovation here, and some other, more rare TypeLoc nodes could
   also have this property, but if you use getLocalSourceRange on them, it's not
   going to return any valid locations, because it doesn't have any. The right fix
   here is to always use getSourceRange() or getBeginLoc/getEndLoc which will dive
   into the inner TypeLoc to get the source range if it doesn't find it on the
   top level one. You can use getLocalSourceRange if you are really into
   micro-optimizations and you have some outside knowledge that the TypeLocs you are
   dealing with will always include some source location.

4) Exposed a bug somewhere in the use of the normal clang type class API, where you
   have some type, you want to see if that type is some particular kind, you try a
   `dyn_cast` such as `dyn_cast<TypedefType>` and that fails because now you have an
   ElaboratedType which has a TypeDefType inside of it, which is what you wanted to match.
   Again, like 2), this would usually have been tested poorly with some simple tests with
   no qualifications, and would have been broken had there been any other kind of type sugar,
   be it an ElaboratedType or a TemplateSpecializationType or a SubstTemplateParmType.
   The usual fix here is to use `getAs` instead of `dyn_cast`, which will look deeper
   into the type. Or use `getAsAdjusted` when dealing with TypeLocs.
   For some reason the API is inconsistent there and on TypeLocs getAs behaves like a dyn_cast.

5) It could be a bug in this patch perhaps.

Let me know if you need any help!

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Differential Revision: https://reviews.llvm.org/D112374
2022-07-15 04:16:55 +02:00
Jonas Devlieghere 3968936b92
Revert "[clang] Implement ElaboratedType sugaring for types written bare"
This reverts commit bdc6974f92 because it
breaks all the LLDB tests that import the std module.

  import-std-module/array.TestArrayFromStdModule.py
  import-std-module/deque-basic.TestDequeFromStdModule.py
  import-std-module/deque-dbg-info-content.TestDbgInfoContentDequeFromStdModule.py
  import-std-module/forward_list.TestForwardListFromStdModule.py
  import-std-module/forward_list-dbg-info-content.TestDbgInfoContentForwardListFromStdModule.py
  import-std-module/list.TestListFromStdModule.py
  import-std-module/list-dbg-info-content.TestDbgInfoContentListFromStdModule.py
  import-std-module/queue.TestQueueFromStdModule.py
  import-std-module/stack.TestStackFromStdModule.py
  import-std-module/vector.TestVectorFromStdModule.py
  import-std-module/vector-bool.TestVectorBoolFromStdModule.py
  import-std-module/vector-dbg-info-content.TestDbgInfoContentVectorFromStdModule.py
  import-std-module/vector-of-vectors.TestVectorOfVectorsFromStdModule.py

https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/45301/
2022-07-13 09:20:30 -07:00
Matheus Izvekov bdc6974f92
[clang] Implement ElaboratedType sugaring for types written bare
Without this patch, clang will not wrap in an ElaboratedType node types written
without a keyword and nested name qualifier, which goes against the intent that
we should produce an AST which retains enough details to recover how things are
written.

The lack of this sugar is incompatible with the intent of the type printer
default policy, which is to print types as written, but to fall back and print
them fully qualified when they are desugared.

An ElaboratedTypeLoc without keyword / NNS uses no storage by itself, but still
requires pointer alignment due to pre-existing bug in the TypeLoc buffer
handling.

Signed-off-by: Matheus Izvekov <mizvekov@gmail.com>

Differential Revision: https://reviews.llvm.org/D112374
2022-07-13 02:10:09 +02:00
Nathan James cfb8169059
[clang] Add a raw_ostream operator<< overload for QualType
Under the hood this prints the same as `QualType::getAsString()` but cuts out the middle-man when that string is sent to another raw_ostream.

Also cleaned up all the call sites where this occurs.

Reviewed By: aaron.ballman

Differential Revision: https://reviews.llvm.org/D123926
2022-04-20 22:09:05 +01:00
Logan Smith 2c2a297bb6 [clang][NFC] Add 'override' keyword to virtual function overrides
This patch adds override to several overriding virtual functions that were missing the keyword within the clang/ directory. These were found by the new -Wsuggest-override.
2020-07-14 08:59:57 -07:00
Stephen Kelly 8d62eba105 Add some explicit use of TK_AsIs 2020-05-23 01:04:44 +01:00
Kirstóf Umann bda3dd0d98 [analyzer][NFC] Change LangOptions to CheckerManager in the shouldRegister* functions
Some checkers may not only depend on language options but also analyzer options.
To make this possible this patch changes the parameter of the shouldRegister*
function to CheckerManager to be able to query the analyzer options when
deciding whether the checker should be registered.

Differential Revision: https://reviews.llvm.org/D75271
2020-03-27 14:34:09 +01:00
Kristof Umann 83cc1b35d1 [analyzer] Remove the default value arg from getChecker*Option
Since D57922, the config table contains every checker option, and it's default
value, so having it as an argument for getChecker*Option is redundant.

By the time any of the getChecker*Option function is called, we verified the
value in CheckerRegistry (after D57860), so we can confidently assert here, as
any irregularities detected at this point must be a programmer error. However,
in compatibility mode, verification won't happen, so the default value must be
restored.

This implies something else, other than adding removing one more potential point
of failure -- debug.ConfigDumper will always contain valid values for
checker/package options!

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

llvm-svn: 361042
2019-05-17 15:52:13 +00:00
Kristof Umann 088b1c9cdc [analyzer] Enable subcheckers to possess checker options
Under the term "subchecker", I mean checkers that do not have a checker class on
their own, like unix.MallocChecker to unix.DynamicMemoryModeling.

Since a checker object was required in order to retrieve checker options,
subcheckers couldn't possess options on their own.

This patch is also an excuse to change the argument order of getChecker*Option,
it always bothered me, now it resembles the actual command line argument
(checkername:option=value).

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

llvm-svn: 355297
2019-03-04 00:28:16 +00:00
Kristof Umann 058a7a450a [analyzer] Supply all checkers with a shouldRegister function
Introduce the boolean ento::shouldRegister##CHECKERNAME(const LangOptions &LO)
function very similarly to ento::register##CHECKERNAME. This will force every
checker to implement this function, but maybe it isn't that bad: I saw a lot of
ObjC or C++ specific checkers that should probably not register themselves based
on some LangOptions (mine too), but they do anyways.

A big benefit of this is that all registry functions now register their checker,
once it is called, registration is guaranteed.

This patch is a part of a greater effort to reinvent checker registration, more
info here: D54438#1315953

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

llvm-svn: 352277
2019-01-26 14:23:08 +00:00
Chandler Carruth 2946cd7010 Update the file headers across all of the LLVM projects in the monorepo
to reflect the new license.

We understand that people may be surprised that we're moving the header
entirely to discuss the new license. We checked this carefully with the
Foundation's lawyer and we believe this is the correct approach.

Essentially, all code in the project is now made available by the LLVM
project under our new license, so you will see that the license headers
include that license only. Some of our contributors have contributed
code under our old license, and accordingly, we have retained a copy of
our old license notice in the top-level files in each project and
repository.

llvm-svn: 351636
2019-01-19 08:50:56 +00:00
Kristof Umann 76a21502fd [analyzer][NFC] Move CheckerRegistry from the Core directory to Frontend
ClangCheckerRegistry is a very non-obvious, poorly documented, weird concept.
It derives from CheckerRegistry, and is placed in lib/StaticAnalyzer/Frontend,
whereas it's base is located in lib/StaticAnalyzer/Core. It was, from what I can
imagine, used to circumvent the problem that the registry functions of the
checkers are located in the clangStaticAnalyzerCheckers library, but that
library depends on clangStaticAnalyzerCore. However, clangStaticAnalyzerFrontend
depends on both of those libraries.

One can make the observation however, that CheckerRegistry has no place in Core,
it isn't used there at all! The only place where it is used is Frontend, which
is where it ultimately belongs.

This move implies that since
include/clang/StaticAnalyzer/Checkers/ClangCheckers.h only contained a single function:

class CheckerRegistry;

void registerBuiltinCheckers(CheckerRegistry &registry);

it had to re purposed, as CheckerRegistry is no longer available to
clangStaticAnalyzerCheckers. It was renamed to BuiltinCheckerRegistration.h,
which actually describes it a lot better -- it does not contain the registration
functions for checkers, but only those generated by the tblgen files.

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

llvm-svn: 349275
2018-12-15 16:23:51 +00:00
Fangrui Song 407659ab0a Revert "Revert r347417 "Re-Reinstate 347294 with a fix for the failures.""
It seems the two failing tests can be simply fixed after r348037

Fix 3 cases in Analysis/builtin-functions.cpp
Delete the bad CodeGen/builtin-constant-p.c for now

llvm-svn: 348053
2018-11-30 23:41:18 +00:00
Fangrui Song f5d3335d75 Revert r347417 "Re-Reinstate 347294 with a fix for the failures."
Kept the "indirect_builtin_constant_p" test case in test/SemaCXX/constant-expression-cxx1y.cpp
while we are investigating why the following snippet fails:

  extern char extern_var;
  struct { int a; } a = {__builtin_constant_p(extern_var)};

llvm-svn: 348039
2018-11-30 21:26:09 +00:00
Hans Wennborg 48ee4ad325 Re-commit r347417 "Re-Reinstate 347294 with a fix for the failures."
This was reverted in r347656 due to me thinking it caused a miscompile of
Chromium. Turns out it was the Chromium code that was broken.

llvm-svn: 347756
2018-11-28 14:04:12 +00:00
Hans Wennborg 8c79706e89 Revert r347417 "Re-Reinstate 347294 with a fix for the failures."
This caused a miscompile in Chrome (see crbug.com/908372) that's
illustrated by this small reduction:

  static bool f(int *a, int *b) {
    return !__builtin_constant_p(b - a) || (!(b - a));
  }

  int arr[] = {1,2,3};

  bool g() {
    return f(arr, arr + 3);
  }

  $ clang -O2 -S -emit-llvm a.cc -o -

g() should return true, but after r347417 it became false for some reason.

This also reverts the follow-up commits.

r347417:
> Re-Reinstate 347294 with a fix for the failures.
>
> Don't try to emit a scalar expression for a non-scalar argument to
> __builtin_constant_p().
>
> Third time's a charm!

r347446:
> The result of is.constant() is unsigned.

r347480:
> A __builtin_constant_p() returns 0 with a function type.

r347512:
> isEvaluatable() implies a constant context.
>
> Assume that we're in a constant context if we're asking if the expression can
> be compiled into a constant initializer. This fixes the issue where a
> __builtin_constant_p() in a compound literal was diagnosed as not being
> constant, even though it's always possible to convert the builtin into a
> constant.

r347531:
> A "constexpr" is evaluated in a constant context. Make sure this is reflected
> if a __builtin_constant_p() is a part of a constexpr.

llvm-svn: 347656
2018-11-27 14:01:40 +00:00
Bill Wendling 6ff1751f7d Re-Reinstate 347294 with a fix for the failures.
Don't try to emit a scalar expression for a non-scalar argument to
__builtin_constant_p().

Third time's a charm!

llvm-svn: 347417
2018-11-21 20:44:18 +00:00
Nico Weber 9f0246d473 Revert r347364 again, the fix was incomplete.
llvm-svn: 347389
2018-11-21 12:47:43 +00:00
Bill Wendling 91549ed15f Reinstate 347294 with a fix for the failures.
EvaluateAsInt() is sometimes called in a constant context. When that's the
case, we need to specify it as so.

llvm-svn: 347364
2018-11-20 23:24:16 +00:00
Kristof Umann 0a1f91c80c [analyzer] Restrict AnalyzerOptions' interface so that non-checker objects have to be registered
One of the reasons why AnalyzerOptions is so chaotic is that options can be
retrieved from the command line whenever and wherever. This allowed for some
options to be forgotten for a looooooong time. Have you ever heard of
"region-store-small-struct-limit"? In order to prevent this in the future, I'm
proposing to restrict AnalyzerOptions' interface so that only checker options
can be retrieved without special getters. I would like to make every option be
accessible only through a getter, but checkers from plugins are a thing, so I'll
have to figure something out for that.

This also forces developers who'd like to add a new option to register it
properly in the .def file.

This is done by

* making the third checker pointer parameter non-optional, and checked by an
  assert to be non-null.
* I added new, but private non-checkers option initializers, meant only for
  internal use,
* Renamed these methods accordingly (mind the consistent name for once with
  getBooleanOption!):
  - getOptionAsString -> getCheckerStringOption,
  - getOptionAsInteger -> getCheckerIntegerOption
* The 3 functions meant for initializing data members (with the not very
  descriptive getBooleanOption, getOptionAsString and getOptionAsUInt names)
  were renamed to be overloads of the getAndInitOption function name.
* All options were in some way retrieved via getCheckerOption. I removed it, and
  moved the logic to getStringOption and getCheckerStringOption. This did cause
  some code duplication, but that's the only way I could do it, now that checker
  and non-checker options are separated. Note that the non-checker version
  inserts the new option to the ConfigTable with the default value, but the
  checker version only attempts to find already existing entries. This is how
  it always worked, but this is clunky and I might end reworking that too, so we
  can eventually get a ConfigTable that contains the entire configuration of the
  analyzer.

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

llvm-svn: 346113
2018-11-05 03:50:37 +00:00
Stephen Kelly f2ceec4811 Port getLocStart -> getBeginLoc
Reviewers: teemperor!

Subscribers: jholewinski, whisperity, jfb, cfe-commits

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

llvm-svn: 339385
2018-08-09 21:08:08 +00:00
George Karpenkov 8dad0e6cce [analyzer] Don't throw NSNumberObjectConversion warning on object initialization in if-expression
```
if (NSNumber* x = ...)
```
is a reasonable pattern in objc++, we should not warn on it.

rdar://35152234

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

llvm-svn: 326619
2018-03-02 21:34:24 +00:00
Artem Dergachev 93fd165bfb [analyzer] NumberObjectConversion: Workaround for a linker error with modules.
A combination of C++ modules, variadic functions with more than one argument,
and const globals in headers (all three being necessary) causes some releases
of clang to misplace the matcher objects, which causes the linker to fail.

No functional change - the extra allOf() matcher is no-op here.

llvm-svn: 287045
2016-11-15 22:22:57 +00:00
Artem Dergachev e14d881808 [analyzer] NumberObjectConversion: support more types, misc updates.
Support CFNumberRef and OSNumber objects, which may also be accidentally
converted to plain integers or booleans.

Enable explicit boolean casts by default in non-pedantic mode.

Improve handling for warnings inside macros.

Improve error messages.

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

llvm-svn: 285533
2016-10-31 03:08:48 +00:00
Artem Dergachev 940c770d27 [analyzer] Add NumberObjectConversion checker.
When dealing with objects that represent numbers, such as Objective-C NSNumber,
the language provides little protection from accidentally interpreting
the value of a pointer to such object as the value of the number represented
by the object. Results of such mis-interpretation may be unexpected.

The checker attempts to fill this gap in cases when the code is obviously
incorrect.

With "Pedantic" option enabled, this checker enforces a coding style to
completely prevent errors of this kind (off by default).

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

llvm-svn: 284473
2016-10-18 11:06:28 +00:00