[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, 
InGroup>;
 def warn_bool_switch_condition : Warning<

aaron.ballman wrote:
> I thought we had a warning group for this already, and we do, it's 
> `-Wunreachable-code`. I think the new diagnostic group be a child of the 
> existing one, if we need the group at all.
Agreed.



Comment at: lib/Sema/SemaStmt.cpp:727
+   unsigned UnpromotedWidth, bool UnpromotedSign,
+   bool ) {
   // In C++11 onwards, this is checked by the language rules.

This function is super confusing, and that's not your doing... but adding a 
`bool&` param kinda piles on. I'd much rather return a `enum class 
CaseListIsErroneous { No, Yes }`, and make `enum class UnpromotedSign` as well 
while we're here.



Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:29
+  label3:
+x++;
+  case 4:

Not that I think you should diagnose here, but I'd like to have a comment in 
the test saying that it's indeed unreachable, but we don't currently diagnose.



Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:49
+  switch (x) {
+;
+  case 7:

Can you also have one like this with typedef and declarations instead of 
statements:
```
switch (x) {
  using Foo = int;
  case 7:
  // ...
```
and

```
switch (x) {
  int im_confused = 42;
  case 7:
  // ...
```

This one is terrible but reachable:

```
switch (x) {
  ({ oh_no: g(x); });
  case 7:
  goto oh_no;
  // ...
```


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

https://reviews.llvm.org/D63139



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

[PATCH] D64695: [clang-format] Added new style rule: SortNetBSDIncludes

[Manikishan Ghantasala via Phabricator via cfe-commits]

Manikishan added a comment.

In D64695#1590948 , @Manikishan wrote:

> In D64695#1589818 , @lebedev.ri 
> wrote:
>
> > In D64695#1589740 , @Manikishan 
> > wrote:
> >
> > > In D64695#1589508 , @lebedev.ri 
> > > wrote:
> > >
> > > > Is there sufficient test coverage as to what happens if `SortPriority` 
> > > > is not set?
> > >
> > >
> > > If SortPriority is not set, the Includes will be grouped without sorting,
> >
> >
> > Let me rephrase - for the exiting `.clang-format`s, that don't currently 
> > specify `SortPriority`,
> >  this introduction of `SortPriority` should not change the header handling.
> >  Is that the case, and if so is there sufficient test coverage for that?
>
>
> I got your idea now.
>  No, there is no test coverage for that case, and with the current patch they 
> have to add SortPriority.
>  To avoid this shall I set SortPriority as Priority as default if it is not 
> defined? I think that will fix the issue.


any reviews on it ?


Repository:
  rC Clang

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

https://reviews.llvm.org/D64695



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

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5609
+def warn_mul_in_bool_context : Warning<
+  "'*' in bool context, maybe you mean '&&'?">,
+  InGroup;

aaron.ballman wrote:
> xbolva00 wrote:
> > aaron.ballman wrote:
> > > I would appreciate seeing some motivating examples for this case, because 
> > > it seems like the `&&` suggestion is misleading more often than it's 
> > > helpful in the current test cases.
> > > 
> > > Also, what is a "bool context"?
> > Bool context - places where we expect boolean expressions.
> > 
> > If you have idea for better terminology, I am happy to apply it.
> > 
> > Yeah, I will remove “&&” suggestion.. 
> I don't have a good recommendation for a replacement for "boolean context", 
> so I suppose we can leave it.
IMO it's clearer to say "assigning the result of a multiplication to a boolean 
variable always yields {true|false}"



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5612
+def warn_left_shift_in_bool_context : Warning<
+  "'<<' in boolean context, maybe you mean '<'?">,
+  InGroup;

xbolva00 wrote:
> xbolva00 wrote:
> > I will this line.
> * Ehh.. I will fix this line to use "did you mean". 
Same here, "assigning the result of a left shift to a boolean variable always 
yields {true|false}"

Same for all the other ones...



Comment at: test/Sema/warn-int-in-bool-context.c:26
+  r = a << 7;   // expected-warning {{'<<' in boolean context; did you mean 
'<'?}}
+  r = ONE << b; // expected-warning {{'<<' in boolean context; did you mean 
'<'?}}
+

I'm not sure the "did you mean" part is helpful. Do we have data showing that 
this is what people actually mean?



Comment at: test/Sema/warn-int-in-bool-context.c:32
+  r = a ? -2 : 0;
+  r = a ? 3 : -2;
+  r = a ? 0 : TWO; // expected-warning {{'?:' with integer constants in 
boolean context}}

Why wouldn't think one warn?



Comment at: test/Sema/warn-int-in-bool-context.c:33
+  r = a ? 3 : -2;
+  r = a ? 0 : TWO; // expected-warning {{'?:' with integer constants in 
boolean context}}
+  r = a ? 3 : ONE; // expected-warning {{'?:' with integer constants in 
boolean context, the expression will always evaluate to 'true'}}

Why does this one warn? It doesn't always yield the same result.



Comment at: test/Sema/warn-unreachable.c:147
+  // expected-warning@+1 {{'*' in boolean context, the expression will always 
evaluate to 'false'}}
   if (0 * x) calledFun(); // expected-warning {{will never be executed}}
 }

It seems like here the "will never be executed" warning is more useful. Do we 
want to emit both?



Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:99
+
+  if (ans == yes || no) // expected-warning {{enum constant in boolean 
context}}
+return a;

aaron.ballman wrote:
> xbolva00 wrote:
> > aaron.ballman wrote:
> > > Why do we want to diagnose this case in C++ but not in C?
> > I think we can and should but for unknown reason (for me) this is GCC’s 
> > behaviour.
> > 
> > Maybe it is too noisy for C codebases? 
> > 
> > Maybe introduce -Wenum-in-bool-context as subgroup of 
> > -Wint-in-bool-context? And enable it for C and C++?
> I think it's just a bug in GCC (they use separate frontends for C and C++, so 
> these sort of differences crop up sometimes). I think this should be safe to 
> diagnose the same in C and C++.
Agreed.


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

https://reviews.llvm.org/D63082



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

[PATCH] D64991: [analyzer][WIP] Implement a primitive reaching definitions analysis

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:30
+  };
+  const VarDecl *Var;
+  CFGBlock::ConstCFGElementRef E;

In the future we might also want to reason about `FieldDecl`s.



Comment at: clang/include/clang/Analysis/Analyses/ReachingDefinitions.h:50
+  bool operator()(Definition Lhs, Definition Rhs) {
+return Lhs.Var < Rhs.Var;
+  }

Is it safe to omit the Kind from the comparisons?



Comment at: clang/lib/Analysis/ReachingDefinitions.cpp:61
+auto AssignmentM =
+binaryOperator(isAssignmentOperator(),
+   hasLHS(declRefExpr(to(varDecl().bind("var")

Will this work if the LHS is not directly a declrefexpr?

E.g.:
```
(a, b) = 5;
```
Or more fun cases:
```
(cond ? a : b) = 5;
```



Comment at: clang/lib/Analysis/ReachingDefinitions.cpp:102
+}
+  }
+

What about the non-const arguments of calls, and indirect writes through 
pointers? Even if you plan not to handle some of these, I prefer to have a TODO 
for these the appropriate places as soon as possible.



Comment at: clang/lib/Analysis/ReachingDefinitions.cpp:104
+
+  return Ret;
+}

There are non-assignment operators too, like operator++. You might want to 
consider them as definitions too.


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

https://reviews.llvm.org/D64991



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

[PATCH] D65290: [analyzer][NFC] Prove that we only track the evaluated part of the condition

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

More tests are always welcome :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65290



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

[PATCH] D65287: [analyzer][CFG] Don't track the condition of asserts

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/include/clang/Analysis/CFG.h:860
+  /// child CFG blocks in a depth-first manner and see if all paths eventually
+  /// end up in an immediate sink block.
+  bool isInevitablySinking() const;

Is it obvious what sink means in CFG's context? Maybe we should mention what 
our definition of sink is? Think about non-csa devs too :)



Comment at: clang/lib/Analysis/CFG.cpp:5628
+
+  // FIXME: Throw-expressions are currently generating sinks during analysis:
+  // they're not supported yet, and also often used for actually terminating

Are there other nodes that are treated as sinks? If so I wonder if there is a 
way to keep the list updated. Oh, I just saw that this code is just moved 
around. 



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1743
+
+  if (Then->isInevitablySinking() || Else->isInevitablySinking())
+return true;

Do we consider a block assert like if both branches are inevitable sinking?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp:1750
+  //   [B1] -> [B2] -> [B3] -> [sink]
+  // assert(A && B || C);\   \   \
+  //  ---> [go on with the execution]

I wonder if the CFG is correct for your example. If A evaluates to false, we 
still check C before the sink. If A evaluates to true we still check B before 
going on. But maybe it is just late for me :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65287



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

[PATCH] D65249: [NFC] use C++11 in AlignOf.h, remove AlignedCharArray

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D65249#1605523 , @rnk wrote:

> I have concerns that some of the patches that you landed prior to this will 
> cause issues with old versions of MSVC, but in isolation, this is fine, and 
> if anyone complains, we can address it on a case-by-case basis. lgtm


IIUC Billy correctly, the issue was just with `aligned_storage`, but if you 
roll your own (as I have) we're fine. If I'm wrong, these tests will fail on 
MSVC bots :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65249



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

[PATCH] D65361: [analyzer] Trust global initializers when analyzing main().

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

I like the change.




Comment at: clang/lib/StaticAnalyzer/Core/RegionStore.cpp:630
+(const RegionBindings::TreeTy *)((uintptr_t)store & ~(uintptr_t)1),
+RBFactory.getTreeFactory(), (bool)((uintptr_t)store & (uintptr_t)1));
   }

baloghadamsoftware wrote:
> `(uintptr_t)1` look like a bit like some kind of magic number. Could we 
> define it as a constant instead?
Or maybe this can be abstracted away using `llvm::PointerIntPair`?


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

https://reviews.llvm.org/D65361



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

[PATCH] D65381: [analyzer][NFC] Refactoring BugReporter.cpp P3.: std::shared_pointer -> PathDiagnosticPieceRef

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Just wondering, did you check if we actually need shared ownership for this 
type? If not, do not waste your time checking for now :)




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/BugReporterVisitors.h:244
+ const ExplodedNode *N,
+ const CFGBlock *srcBlk,
+ const CFGBlock *dstBlk, BugReport ,

If you already touch these parts maybe you could capitalize some variable names.



Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:449
 
-class PathPieces : public std::list> {
+using PathDiagnosticPieceRef = std::shared_ptr;
+

NoQ wrote:
> Mmm, why do we need to define this twice?
I agree, I would love to not to see it twice.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65381



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

[PATCH] D65349: [analyzer] Be more careful with destructors of non-regions.

[Gábor Horváth via Phabricator via cfe-commits]

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

LG!


Repository:
  rC Clang

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

https://reviews.llvm.org/D65349



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

[PATCH] D65379: [analyzer][NFC] Refactoring BugReporter.cpp P2.: Clean up the construction of bug paths and finding a valid report

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

Looks good, some nits inline. I agree with Artem, we should consider omitting 
the trimming and document how to get something similar if desired for debugging.

Just a random idea: maybe the original purpose of the trimming was to be able 
to reclaim parts of the exploded graph as soon as possible?




Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2269
+  // Since the error node the BugReport is in to the original ExplodedGraph,
+  // we need to map it the one found in the trimmed graph.
+  using ReportNewNodePair = std::pair;

This comment was really hard for me to understand, could you rephrase it?



Comment at: clang/lib/StaticAnalyzer/Core/BugReporter.cpp:2305
+  BugPathGetter(const ExplodedGraph *OriginalGraph,
+   ArrayRef );
 

Formatting might be off here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65379



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

[PATCH] D65378: [analyzer][NFC] Refactoring BugReporter.cpp P1.: Store interesting symbols/regions in a simple set

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun accepted this revision.
xazax.hun added a comment.

LGTM! I also do not see much value in the old code at this point. I would 
expect PathDiagnosticConsumers to be independent of the interesting 
symbols/regions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65378



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

[PATCH] D63932: [GlobalDCE] Dead Virtual Function Elimination

[Peter Collingbourne via Phabricator via cfe-commits]

pcc added a comment.

It doesn't seem sound to create the distinction between translation unit and 
linkage unit visibility and then allow passes to relax visibility. For example, 
imagine that you have an executable with (compiled with `-fvisibility=default`):

  struct PluginInterface {
virtual void foo() = 0;
  }
  
  struct Plugin1 : PluginInterface {
virtual void foo();
  };
  
  void Plugin1::foo() {
   // ...
  }

And a DSO loaded with dlopen:

  void call_foo(PluginInterface *plugin) {
plugin->foo();
  }

Note that the lack of attribute means that the vtables have public LTO 
visibility. Depending on the contents of the DSO, the `PluginInterface` and 
`Plugin1` vtable symbols may not be referenced at all by the DSO. If the DSO is 
loaded via `dlopen` it will definitely not be referenced at link time. If we 
allow LTO to relax `PluginInterface` and `Plugin1` vtables to what you're 
calling `VCallVisibilityTranslationUnit` in the main executable and there are 
no calls to `foo()` in the main executable, we will incorrectly drop the 
definition of `Plugin1::foo()`.

The approach that I can see working involves basically the same structure as 
CFI and WPD: the LTO visibility is decided by the frontend and not relaxed by 
passes or LTO. There is only one "flavour" of `!vcall_visibility` metadata: the 
one that means the entire hierarchy below the vtable has hidden LTO visibility.

> However, the
>  changes in clang to use the llvm.type.checked.load intrinsic are causing ~1%
>  performance regression in the C++ parts of SPEC2006.

Have you checked the assembly to see whether an unused CFI check is being 
emitted?




Comment at: clang/lib/CodeGen/CGClass.cpp:2816
+  } else {
+Builder.CreateCall(CGM.getIntrinsic(llvm::Intrinsic::assume), CheckResult);
+  }

There should be no need to emit this `llvm.assume` intrinsic. We can rely on 
the unspecified behaviour in the case where the second element returned by 
`llvm.type.checked.load` is false.



Comment at: llvm/docs/TypeMetadata.rst:240
+
+  __attribute__((visibility("public")))
+  struct A {

`s/"public"/"default"/`



Comment at: llvm/lib/Transforms/IPO/GlobalDCE.cpp:183
+// unit, we know that we can see all virtual functions which might use it,
+// so VFE is safe.
+if (auto GO = dyn_cast()) {

Not necessarily, at least in this implementation, because "vtable symbol can be 
internalized" doesn't imply "all vcalls are visible". See main response.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63932



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

[PATCH] D65182: [analyzer] WIP: Add fix-it hint support.

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

The direction looks good to me. I wonder if it is worth to add some warnings 
somewhere that it is really tricky to come up with reasonable fixits for most 
path sensitive checks.


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

https://reviews.llvm.org/D65182



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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Fangrui Song via Phabricator via cfe-commits]

MaskRay marked 3 inline comments as done.
MaskRay added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5647-5650
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
+  InGroup, DefaultIgnore;

rsmith wrote:
> I think this should remain enabled by default unless you have evidence of 
> false positives. In my experience, this catches bugs like
> 
> ```
> ostream << "got expected result: " << x == 0;
> ```
> 
> ... and very little else.
> 
> That said, perhaps we could do better here, since this warning is typically 
> followed by an error: if we detect the special case of overload resolution 
> failure for an operator with an `<<` (or `>>`) operator expression on its 
> left-hand side, we could produce a much more targeted diagnostic for this 
> case and avoid the current situation of a warning followed by an error for 
> the same problem. If we did that, we could probably remove this warning 
> entirely.
Restored this one.

Searching for Wno-parentheses can probably give more interesting results...


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D65182: [analyzer] WIP: Add fix-it hint support.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D65182#1604280 , 
@baloghadamsoftware wrote:

> Hmm, I was thinking on the same for some time but I wonder how many checkers 
> could find the correct fixits? Maybe the removal fixits of double frees or 
> double file closes, but I am afraid that for most of our path-sensitive 
> checks there are no obvious fixits. Even `clang-tidy` cannot provide a fixit 
> for most of its findings. However, generally I like the idea, even for the 
> few checkers it can be applied to.


I'd be pretty surprised if any path-sensitive checker would ever have a really 
good fixit, which is why i never was super excited about this idea when people 
were asking for it on the mailing lists. This is definitely mostly for 
syntactic checkers (think `MallocSizeof` or some of our Objective-C checkers). 
But at least it's one less artificial roadblock for people choosing where to 
put their checker :/


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

https://reviews.llvm.org/D65182



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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Fangrui Song via Phabricator via cfe-commits]

MaskRay updated this revision to Diff 212265.
MaskRay edited the summary of this revision.
MaskRay added a comment.

keep -Woverloaded-shift-op-parentheses enabled by default


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  test/Misc/warning-flags-enabled.c
  test/Parser/cxx2a-spaceship.cpp
  test/SemaCXX/parentheses.cpp


Index: test/SemaCXX/parentheses.cpp
===
--- test/SemaCXX/parentheses.cpp
+++ test/SemaCXX/parentheses.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify -Wlogical-op-parentheses %s
 
 // PR16930, PR16727:
 template
Index: test/Parser/cxx2a-spaceship.cpp
===
--- test/Parser/cxx2a-spaceship.cpp
+++ test/Parser/cxx2a-spaceship.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -verify %s
+// RUN: %clang_cc1 -std=c++2a -verify -Wparentheses %s
 
 template struct X {};
 
Index: test/Misc/warning-flags-enabled.c
===
--- test/Misc/warning-flags-enabled.c
+++ test/Misc/warning-flags-enabled.c
@@ -36,7 +36,7 @@
 
 // Test if -Wshift-op-parentheses is a subgroup of -Wparentheses
 // RUN: diagtool show-enabled --no-levels -Wno-parentheses 
-Wshift-op-parentheses %s | FileCheck --check-prefix CHECK-SHIFT-OP-PARENTHESES 
%s
-// RUN: diagtool show-enabled --no-levels %s | FileCheck --check-prefix 
CHECK-SHIFT-OP-PARENTHESES %s
+// RUN: diagtool show-enabled --no-levels %s | FileCheck --check-prefix 
CHECK-NO-SHIFT-OP-PARENTHESES %s
 // RUN: diagtool show-enabled --no-levels -Wno-parentheses %s | FileCheck 
--check-prefix CHECK-NO-SHIFT-OP-PARENTHESES %s
 //
 // CHECK-SHIFT-OP-PARENTHESES: -Wshift-op-parentheses
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5630,10 +5630,10 @@
   "remove constant to silence this warning">;
 
 def warn_bitwise_op_in_bitwise_op : Warning<
-  "'%0' within '%1'">, InGroup;
+  "'%0' within '%1'">, InGroup, DefaultIgnore;
 
 def warn_logical_and_in_logical_or : Warning<
-  "'&&' within '||'">, InGroup;
+  "'&&' within '||'">, InGroup, DefaultIgnore;
 
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
@@ -5644,7 +5644,7 @@
 
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
-  "'%1' will be evaluated first">, InGroup;
+  "'%1' will be evaluated first">, InGroup, DefaultIgnore;
 
 def warn_self_assignment_builtin : Warning<
   "explicitly assigning value of variable of type %0 to itself">,


Index: test/SemaCXX/parentheses.cpp
===
--- test/SemaCXX/parentheses.cpp
+++ test/SemaCXX/parentheses.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify %s
+// RUN: %clang_cc1 -verify -Wlogical-op-parentheses %s
 
 // PR16930, PR16727:
 template
Index: test/Parser/cxx2a-spaceship.cpp
===
--- test/Parser/cxx2a-spaceship.cpp
+++ test/Parser/cxx2a-spaceship.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -std=c++2a -verify %s
+// RUN: %clang_cc1 -std=c++2a -verify -Wparentheses %s
 
 template struct X {};
 
Index: test/Misc/warning-flags-enabled.c
===
--- test/Misc/warning-flags-enabled.c
+++ test/Misc/warning-flags-enabled.c
@@ -36,7 +36,7 @@
 
 // Test if -Wshift-op-parentheses is a subgroup of -Wparentheses
 // RUN: diagtool show-enabled --no-levels -Wno-parentheses -Wshift-op-parentheses %s | FileCheck --check-prefix CHECK-SHIFT-OP-PARENTHESES %s
-// RUN: diagtool show-enabled --no-levels %s | FileCheck --check-prefix CHECK-SHIFT-OP-PARENTHESES %s
+// RUN: diagtool show-enabled --no-levels %s | FileCheck --check-prefix CHECK-NO-SHIFT-OP-PARENTHESES %s
 // RUN: diagtool show-enabled --no-levels -Wno-parentheses %s | FileCheck --check-prefix CHECK-NO-SHIFT-OP-PARENTHESES %s
 //
 // CHECK-SHIFT-OP-PARENTHESES: -Wshift-op-parentheses
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5630,10 +5630,10 @@
   "remove constant to silence this warning">;
 
 def warn_bitwise_op_in_bitwise_op : Warning<
-  "'%0' within '%1'">, InGroup;
+  "'%0' within '%1'">, InGroup, DefaultIgnore;
 
 def warn_logical_and_in_logical_or : Warning<
-  "'&&' within '||'">, InGroup;
+  "'&&' within '||'">, InGroup, DefaultIgnore;
 
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
@@ 

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Fangrui Song via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5654-5656
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
+  "'%1' will be evaluated first">, InGroup, DefaultIgnore;

rsmith wrote:
> Do you have evidence that this warning has a significant false-positive rate? 
> In my experience it's very common for people to think of `<<` as being a 
> multiplication-like operator and be surprised when it turns out to have lower 
> precedence than addition.
warn_addition_in_bitshift has many false positives. Some results when searching 
for `[-Wshift-op-parentheses]` and the most common diagnostic `operator '<<' 
has lower precedence than '+'; '+' will be evaluated first`: 

https://gitship.com/srutscher/pdp-6-emulator/blob/f41b119eee2f409ea519b15f3a76cfecb70c03d8/emu/Makefile
https://www.openwall.com/lists/musl/2014/04/04/12
https://salmonella-freebsd-x86-64.call-cc.org/chicken-4/clang/freebsd/x86-64/2018/06/21/salmonella-report/install/bvsp-spline.html
https://software.intel.com/en-us/forums/intel-threading-building-blocks/topic/431422
https://github.com/rdkit/rdkit/issues/145
ffmpeg https://bugs.freebsd.org/bugzilla/show_bug.cgi?format=multiple=191743
https://clang.debian.net/logs/2015-03-25/fairymax_4.8v-1_unstable_clang.log



Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D64849: Add platform guards to PPC vector intrinsics headers.

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367281: [PowerPC] [Clang] Add platform guards to PPC vector 
intrinsics headers (authored by chaofan, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D64849?vs=210239=212264#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D64849

Files:
  cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp
  cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h
  cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h
  cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h
  cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h

Index: cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h
===
--- cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h
+++ cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h
@@ -34,6 +34,8 @@
 #ifndef _XMMINTRIN_H_INCLUDED
 #define _XMMINTRIN_H_INCLUDED
 
+#if defined(__linux__) && defined(__ppc64__)
+
 /* Define four value permute mask */
 #define _MM_SHUFFLE(w,x,y,z) (((w) << 6) | ((x) << 4) | ((y) << 2) | (z))
 
@@ -1835,4 +1837,8 @@
 /* For backward source compatibility.  */
 //# include 
 
+#else
+#include_next 
+#endif /* defined(__linux__) && defined(__ppc64__) */
+
 #endif /* _XMMINTRIN_H_INCLUDED */
Index: cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h
===
--- cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h
+++ cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h
@@ -35,6 +35,8 @@
 #ifndef EMMINTRIN_H_
 #define EMMINTRIN_H_
 
+#if defined(__linux__) && defined(__ppc64__)
+
 #include 
 
 /* We need definitions from the SSE header files.  */
@@ -2315,4 +2317,8 @@
   return (__m128d) __A;
 }
 
+#else
+#include_next 
+#endif /* defined(__linux__) && defined(__ppc64__) */
+
 #endif /* EMMINTRIN_H_ */
Index: cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h
===
--- cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h
+++ cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h
@@ -10,6 +10,8 @@
 #ifndef _MM_MALLOC_H_INCLUDED
 #define _MM_MALLOC_H_INCLUDED
 
+#if defined(__linux__) && defined(__ppc64__)
+
 #include 
 
 /* We can't depend on  since the prototype of posix_memalign
@@ -41,4 +43,8 @@
   free (ptr);
 }
 
+#else
+#include_next 
+#endif
+
 #endif /* _MM_MALLOC_H_INCLUDED */
Index: cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h
===
--- cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h
+++ cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h
@@ -35,6 +35,8 @@
 #ifndef _MMINTRIN_H_INCLUDED
 #define _MMINTRIN_H_INCLUDED
 
+#if defined(__linux__) && defined(__ppc64__)
+
 #include 
 /* The Intel API is flexible enough that we must allow aliasing with other
vector types, and their scalar components.  */
@@ -1440,4 +1442,9 @@
   return (res.as_m64);
 #endif
 }
+
+#else
+#include_next 
+#endif /* defined(__linux__) && defined(__ppc64__) */
+
 #endif /* _MMINTRIN_H_INCLUDED */
Index: cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp
===
--- cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp
+++ cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp
@@ -16,10 +16,7 @@
 
 void PPCLinuxToolChain::AddClangSystemIncludeArgs(const ArgList ,
   ArgStringList ) const {
-  // PPC wrapper headers are implementation of x86 intrinsics on PowerPC, which
-  // is not supported on PPC32 platform.
-  if (getArch() != llvm::Triple::ppc &&
-  !DriverArgs.hasArg(clang::driver::options::OPT_nostdinc) &&
+  if (!DriverArgs.hasArg(clang::driver::options::OPT_nostdinc) &&
   !DriverArgs.hasArg(options::OPT_nobuiltininc)) {
 const Driver  = getDriver();
 SmallString<128> P(D.ResourceDir);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65239: [analyzer] RangeConstraintManager: Apply constraint ranges of bitwise operations

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added inline comments.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:496
+  // as a bitwise operation result could be null.
+  if (RS.getConcreteValue() && RS.getConcreteValue()->getExtValue() == 0)
+return State;

Instead of "we know that the value is null", we should write "we //don't// know 
that the value is //non-//null". I.e. if we're not sure, we must still do an 
early return.



Comment at: clang/lib/StaticAnalyzer/Core/RangeConstraintManager.cpp:507
+  ConstraintRangeTy Constraints = State->get();
+  for (const SymbolRef CurrentSym : SIE->symbols()) {
+if (CurrentSym == SIE)

This loop doesn't make sense. When your expression looks like `(((a + b) + c) + 
d) & (e + f)`, you don't want to iterate separately over `a`, `b`, `c`, `d`, 
`e`, `f`; but that's what this loop would do. You should only look at the LHS 
and the RHS. If you want to descend further, do so recursively, so that to keep 
track of the overall structure of the symbolic expression as you're traversing 
it, rather than traversing sub-expressions in an essentially random order.



Comment at: clang/test/Analysis/bitwise-ranges.cpp:21
+
+  // CHECK: { "symbol": "reg_$0", "range": "{ [2, 3] }" }
+  // CHECK: { "symbol": "(reg_$0) & 1U", "range": "{ [1, 1] }" 
}

This test accidentally tests debug prints. We don't want to test debug prints 
here. I suggest:
```lang=c++
clang_analyzer_eval(X >= 2 && X <= 3); // expected-warning{{TRUE}}
```


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

https://reviews.llvm.org/D65239



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

r367281 - [PowerPC] [Clang] Add platform guards to PPC vector intrinsics headers

[Qiu Chaofan via cfe-commits]

Author: chaofan
Date: Mon Jul 29 19:18:11 2019
New Revision: 367281

URL: http://llvm.org/viewvc/llvm-project?rev=367281=rev
Log:
[PowerPC] [Clang] Add platform guards to PPC vector intrinsics headers

Move the platform check out of PPC Linux toolchain code and add platform guards
to the intrinsic headers, since they are supported currently only on 64-bit
PowerPC targets.

Reviewed By: Jinsong Ji

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

Modified:
cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp
cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h
cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h
cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h
cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h

Modified: cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp?rev=367281=367280=367281=diff
==
--- cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp (original)
+++ cfe/trunk/lib/Driver/ToolChains/PPCLinux.cpp Mon Jul 29 19:18:11 2019
@@ -16,10 +16,7 @@ using namespace llvm::opt;
 
 void PPCLinuxToolChain::AddClangSystemIncludeArgs(const ArgList ,
   ArgStringList ) 
const {
-  // PPC wrapper headers are implementation of x86 intrinsics on PowerPC, which
-  // is not supported on PPC32 platform.
-  if (getArch() != llvm::Triple::ppc &&
-  !DriverArgs.hasArg(clang::driver::options::OPT_nostdinc) &&
+  if (!DriverArgs.hasArg(clang::driver::options::OPT_nostdinc) &&
   !DriverArgs.hasArg(options::OPT_nobuiltininc)) {
 const Driver  = getDriver();
 SmallString<128> P(D.ResourceDir);

Modified: cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h?rev=367281=367280=367281=diff
==
--- cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h (original)
+++ cfe/trunk/lib/Headers/ppc_wrappers/emmintrin.h Mon Jul 29 19:18:11 2019
@@ -35,6 +35,8 @@
 #ifndef EMMINTRIN_H_
 #define EMMINTRIN_H_
 
+#if defined(__linux__) && defined(__ppc64__)
+
 #include 
 
 /* We need definitions from the SSE header files.  */
@@ -2315,4 +2317,8 @@ _mm_castsi128_pd(__m128i __A)
   return (__m128d) __A;
 }
 
+#else
+#include_next 
+#endif /* defined(__linux__) && defined(__ppc64__) */
+
 #endif /* EMMINTRIN_H_ */

Modified: cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h?rev=367281=367280=367281=diff
==
--- cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h (original)
+++ cfe/trunk/lib/Headers/ppc_wrappers/mm_malloc.h Mon Jul 29 19:18:11 2019
@@ -10,6 +10,8 @@
 #ifndef _MM_MALLOC_H_INCLUDED
 #define _MM_MALLOC_H_INCLUDED
 
+#if defined(__linux__) && defined(__ppc64__)
+
 #include 
 
 /* We can't depend on  since the prototype of posix_memalign
@@ -41,4 +43,8 @@ _mm_free (void * ptr)
   free (ptr);
 }
 
+#else
+#include_next 
+#endif
+
 #endif /* _MM_MALLOC_H_INCLUDED */

Modified: cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h?rev=367281=367280=367281=diff
==
--- cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h (original)
+++ cfe/trunk/lib/Headers/ppc_wrappers/mmintrin.h Mon Jul 29 19:18:11 2019
@@ -35,6 +35,8 @@
 #ifndef _MMINTRIN_H_INCLUDED
 #define _MMINTRIN_H_INCLUDED
 
+#if defined(__linux__) && defined(__ppc64__)
+
 #include 
 /* The Intel API is flexible enough that we must allow aliasing with other
vector types, and their scalar components.  */
@@ -1440,4 +1442,9 @@ extern __inline __m64
   return (res.as_m64);
 #endif
 }
+
+#else
+#include_next 
+#endif /* defined(__linux__) && defined(__ppc64__) */
+
 #endif /* _MMINTRIN_H_INCLUDED */

Modified: cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h?rev=367281=367280=367281=diff
==
--- cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h (original)
+++ cfe/trunk/lib/Headers/ppc_wrappers/xmmintrin.h Mon Jul 29 19:18:11 2019
@@ -34,6 +34,8 @@
 #ifndef _XMMINTRIN_H_INCLUDED
 #define _XMMINTRIN_H_INCLUDED
 
+#if defined(__linux__) && defined(__ppc64__)
+
 /* Define four value permute mask */
 #define _MM_SHUFFLE(w,x,y,z) (((w) << 6) | ((x) << 4) | ((y) << 2) | (z))
 
@@ -1835,4 +1837,8 @@ do {  
\
 /* For backward source compatibility.  */
 //# include 
 
+#else
+#include_next 
+#endif /* defined(__linux__) && defined(__ppc64__) */
+
 #endif /* _XMMINTRIN_H_INCLUDED */

[PATCH] D65382: [analyzer][NFC] Refactoring BugReporter.cpp P4.: If it can be const, make it const

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

It's counter-idiomatic to pass manager objects (`ProgramStateManager`, 
`SValBuilder`, etc.) as const-references because it tells you pretty much 
nothing about what you can or cannot do with them. You don't really ever 
semantically mutate them in the first place.

But when it comes to actual data structures such as `ExplodedGraph`, we should 
totally be doing this, yeah :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65382



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

[PATCH] D65426: [Coverage] Hide coverage for regions with incorrect end locations (PR39942)

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

PTAL: https://reviews.llvm.org/D65428


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

https://reviews.llvm.org/D65426



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

[PATCH] D65428: Remove cache for macro arg stringization

[Reid Kleckner via Phabricator via cfe-commits]

rnk created this revision.
rnk added reviewers: vsk, rsmith.
Herald added a project: clang.

The cache recorded the wrong expansion location for all but the first
stringization. It seems uncommon to stringize the same macro argument
multiple times, so this cache doesn't seem that important.

Fixes PR39942


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65428

Files:
  clang/include/clang/Lex/MacroArgs.h
  clang/lib/Lex/MacroArgs.cpp
  clang/lib/Lex/TokenLexer.cpp
  clang/test/CoverageMapping/macro-stringize-twice.cpp
  clang/unittests/Lex/LexerTest.cpp

Index: clang/unittests/Lex/LexerTest.cpp
===
--- clang/unittests/Lex/LexerTest.cpp
+++ clang/unittests/Lex/LexerTest.cpp
@@ -401,18 +401,21 @@
   auto MacroArgsDeleter = [](MacroArgs *M) { M->destroy(*PP); };
   std::unique_ptr MA(
   MacroArgs::create(MI, ArgTokens, false, *PP), MacroArgsDeleter);
-  Token Result = MA->getStringifiedArgument(0, *PP, {}, {});
+  auto StringifyArg = [&](int ArgNo) {
+return MA->StringifyArgument(MA->getUnexpArgument(ArgNo), *PP,
+ /*Charify=*/false, {}, {});
+  };
+  Token Result = StringifyArg(0);
   EXPECT_EQ(tok::string_literal, Result.getKind());
   EXPECT_STREQ("\"\\\"StrArg\\\"\"", Result.getLiteralData());
-  Result = MA->getStringifiedArgument(1, *PP, {}, {});
+  Result = StringifyArg(1);
   EXPECT_EQ(tok::string_literal, Result.getKind());
   EXPECT_STREQ("\"5\"", Result.getLiteralData());
-  Result = MA->getStringifiedArgument(2, *PP, {}, {});
+  Result = StringifyArg(2);
   EXPECT_EQ(tok::string_literal, Result.getKind());
   EXPECT_STREQ("\"'C'\"", Result.getLiteralData());
 #if !defined(NDEBUG) && GTEST_HAS_DEATH_TEST
-  EXPECT_DEATH(MA->getStringifiedArgument(3, *PP, {}, {}),
-   "Invalid argument number!");
+  EXPECT_DEATH(StringifyArg(3), "Invalid arg #");
 #endif
 }
 
Index: clang/test/CoverageMapping/macro-stringize-twice.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/macro-stringize-twice.cpp
@@ -0,0 +1,30 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only %s | FileCheck %s
+
+// PR39942
+
+class a;
+template  a <<(b &, const char *);
+int c;
+#define d(l) l(__FILE__, __LINE__, c)
+#define COMPACT_GOOGLE_LOG_ERROR d(e)
+#define f(g, cond) cond ? (void)0 : h() & g
+#define i(j) COMPACT_GOOGLE_LOG_##j.g()
+#define k(j) f(i(j), 0)
+class e {
+public:
+  e(const char *, int, int);
+  a ();
+};
+class h {
+public:
+  void operator&(a &);
+};
+void use_str(const char *);
+
+#define m(func)\
+  use_str(#func);  \
+  k(ERROR) << #func;   \
+  return 0; // CHECK: File 1, [[@LINE-1]]:4 -> [[@LINE-1]]:16 = (#0 - #1)
+int main() {
+  m(asdf);
+}
Index: clang/lib/Lex/TokenLexer.cpp
===
--- clang/lib/Lex/TokenLexer.cpp
+++ clang/lib/Lex/TokenLexer.cpp
@@ -383,18 +383,10 @@
   SourceLocation ExpansionLocEnd =
   getExpansionLocForMacroDefLoc(Tokens[I+1].getLocation());
 
-  Token Res;
-  if (CurTok.is(tok::hash))  // Stringify
-Res = ActualArgs->getStringifiedArgument(ArgNo, PP,
- ExpansionLocStart,
- ExpansionLocEnd);
-  else {
-// 'charify': don't bother caching these.
-Res = MacroArgs::StringifyArgument(ActualArgs->getUnexpArgument(ArgNo),
-   PP, true,
-   ExpansionLocStart,
-   ExpansionLocEnd);
-  }
+  bool Charify = CurTok.is(tok::hashat);
+  const Token *UnexpArg = ActualArgs->getUnexpArgument(ArgNo);
+  Token Res = MacroArgs::StringifyArgument(
+  UnexpArg, PP, Charify, ExpansionLocStart, ExpansionLocEnd);
   Res.setFlag(Token::StringifiedInMacro);
 
   // The stringified/charified string leading space flag gets set to match
Index: clang/lib/Lex/MacroArgs.cpp
===
--- clang/lib/Lex/MacroArgs.cpp
+++ clang/lib/Lex/MacroArgs.cpp
@@ -76,8 +76,6 @@
 /// destroy - Destroy and deallocate the memory for this object.
 ///
 void MacroArgs::destroy(Preprocessor ) {
-  StringifiedArgs.clear();
-
   // Don't clear PreExpArgTokens, just clear the entries.  Clearing the entries
   // would deallocate the element vectors.
   for (unsigned i = 0, e = PreExpArgTokens.size(); i != e; ++i)
@@ -307,21 +305,3 @@
   ExpansionLocStart, ExpansionLocEnd);
   return Tok;
 }
-
-/// getStringifiedArgument - Compute, cache, and return the specified 

[PATCH] D65381: [analyzer][NFC] Refactoring BugReporter.cpp P3.: std::shared_pointer -> PathDiagnosticPieceRef

[Artem Dergachev via Phabricator via cfe-commits]

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

>   find clang/ -type f -exec sed -i 
> 's/std::shared_ptr/PathDiagnosticPieceRef/g' {} \;
>   git diff -U3 --no-color HEAD^ | clang-format-diff -p1 -i

All you need to know about the state of C++ refactoring tools.




Comment at: 
clang/include/clang/StaticAnalyzer/Core/BugReporter/PathDiagnostic.h:449
 
-class PathPieces : public std::list> {
+using PathDiagnosticPieceRef = std::shared_ptr;
+

Mmm, why do we need to define this twice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65381



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

[PATCH] D65379: [analyzer][NFC] Refactoring BugReporter.cpp P2.: Clean up the construction of bug paths and finding a valid report

[Artem Dergachev via Phabricator via cfe-commits]

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

If only somebody could explain to me why do even need trimming in the first 
place :/

I was only keeping it around for easier exploded graph dumps reading, but these 
days we can trivially replace it with a flag to exploded-graph-rewriter (i.e., 
combine D65345  with D64110 
).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65379



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

[PATCH] D65426: [Coverage] Hide coverage for regions with incorrect end locations (PR39942)

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I did some digging and I figured out where things go wrong. The issue is the 
repeated string-izing of the macro argument, the repetition of `#func` here. 
This hacky patch seems to fix the issue:

  diff --git a/clang/lib/Lex/MacroArgs.cpp b/clang/lib/Lex/MacroArgs.cpp
  index 5aa4679fad4..29fd25a43bb 100644
  --- a/clang/lib/Lex/MacroArgs.cpp
  +++ b/clang/lib/Lex/MacroArgs.cpp
  @@ -318,7 +318,7 @@ const Token ::getStringifiedArgument(unsigned 
ArgNo,
 if (StringifiedArgs.empty())
   StringifiedArgs.resize(getNumMacroArguments(), {});
  
  -  if (StringifiedArgs[ArgNo].isNot(tok::string_literal))
  +  //if (StringifiedArgs[ArgNo].isNot(tok::string_literal))
   StringifiedArgs[ArgNo] = StringifyArgument(getUnexpArgument(ArgNo), PP,
  /*Charify=*/false,
  ExpansionLocStart,

Basically, the second time we process a string-ized macro argument, we hit the 
cache, which uses the wrong expansion location for the second expansion. I'm 
going to try cleaning this up and we'll see if the cache is really needed.


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

https://reviews.llvm.org/D65426



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

[PATCH] D65427: [analyzer] exploded-graph-rewriter: Implement Store pointers.

[Csaba Dabis via Phabricator via cfe-commits]

Charusso accepted this revision.
Charusso added a comment.

Oh, of course. Thanks!


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

https://reviews.llvm.org/D65427



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

[PATCH] D65249: [NFC] use C++11 in AlignOf.h, remove AlignedCharArray

[Reid Kleckner via Phabricator via cfe-commits]

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

I have concerns that some of the patches that you landed prior to this will 
cause issues with old versions of MSVC, but in isolation, this is fine, and if 
anyone complains, we can address it on a case-by-case basis. lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65249



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

[PATCH] D65427: [analyzer] exploded-graph-rewriter: Implement Store pointers.

[Artem Dergachev via Phabricator via cfe-commits]

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



Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:647
 else:
+self._dump(' (%s)' % st.ptr)
 if prev_st is not None:

Charusso wrote:
> Extra space at the start of the string: `' <`.
I'm doing this intentionally because GraphViz's rendering of spaces is pretty 
weird. Without this space the new text gets agglutinated to the "Store:" 
caption.


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

https://reviews.llvm.org/D65427



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

[PATCH] D65427: [analyzer] exploded-graph-rewriter: Implement Store pointers.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 212257.
NoQ added a comment.

Hmm, on second thought...
Make the pointers a bit darker in dark mode.

F9692102: Screen Shot 2019-07-29 at 5.31.14 PM.png 



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

https://reviews.llvm.org/D65427

Files:
  clang/test/Analysis/exploded-graph-rewriter/escapes.c
  clang/test/Analysis/exploded-graph-rewriter/store.dot
  clang/utils/analyzer/exploded-graph-rewriter.py


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -644,6 +644,10 @@
 if st is None:
 self._dump(' Nothing!')
 else:
+if self._dark_mode:
+self._dump(' (%s)' % st.ptr)
+else:
+self._dump(' (%s)' % st.ptr)
 if prev_st is not None:
 if s.store.is_different(prev_st):
 self._dump('')
Index: clang/test/Analysis/exploded-graph-rewriter/store.dot
===
--- clang/test/Analysis/exploded-graph-rewriter/store.dot
+++ clang/test/Analysis/exploded-graph-rewriter/store.dot
@@ -4,6 +4,7 @@
 // UNSUPPORTED: system-windows
 
 // CHECK: Store: 
+// CHECK-SAME: (0x2)
 // CHECK-SAME: 
 // CHECK-SAME:   
 // CHECK-SAME: 
Index: clang/test/Analysis/exploded-graph-rewriter/escapes.c
===
--- clang/test/Analysis/exploded-graph-rewriter/escapes.c
+++ clang/test/Analysis/exploded-graph-rewriter/escapes.c
@@ -9,7 +9,7 @@
 // UNSUPPORTED: system-windows
 
 void escapes() {
-  // CHECK: Store: 
+  // CHECK: Store:  (0x{{[0-9a-f]*}})
   // CHECK-SAME: foo0
   // CHECK-SAME: Element\{"foo",0 S64b,char\}
   // CHECK: Environment: 


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -644,6 +644,10 @@
 if st is None:
 self._dump(' Nothing!')
 else:
+if self._dark_mode:
+self._dump(' (%s)' % st.ptr)
+else:
+self._dump(' (%s)' % st.ptr)
 if prev_st is not None:
 if s.store.is_different(prev_st):
 self._dump('')
Index: clang/test/Analysis/exploded-graph-rewriter/store.dot
===
--- clang/test/Analysis/exploded-graph-rewriter/store.dot
+++ clang/test/Analysis/exploded-graph-rewriter/store.dot
@@ -4,6 +4,7 @@
 // UNSUPPORTED: system-windows
 
 // CHECK: Store: 
+// CHECK-SAME: (0x2)
 // CHECK-SAME: 
 // CHECK-SAME:   
 // CHECK-SAME: 
Index: clang/test/Analysis/exploded-graph-rewriter/escapes.c
===
--- clang/test/Analysis/exploded-graph-rewriter/escapes.c
+++ clang/test/Analysis/exploded-graph-rewriter/escapes.c
@@ -9,7 +9,7 @@
 // UNSUPPORTED: system-windows
 
 void escapes() {
-  // CHECK: Store: 
+  // CHECK: Store:  (0x{{[0-9a-f]*}})
   // CHECK-SAME: foo0
   // CHECK-SAME: Element\{"foo",0 S64b,char\}
   // CHECK: Environment: 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Tried the suggested approach and ran into the issue described above.
Either we make the diagnostics less useful ('did you mean foo::bar?' turns into 
'unresolved identifier bar'; without mentioning the correction)  or we even 
stop providing some corrections in addition to that.

On the other hand, I agree that over time we will start emitting too many 
diagnostics at the end of TU if keep the patch as is. That is not a good way.
There should be a better option for emitting the uncorrected diagnostics closer 
to where they are produced, but I don't have a good idea on what it should be 
off the top of my head. Ideas warmly welcome.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64799



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

[PATCH] D65427: [analyzer] exploded-graph-rewriter: Implement Store pointers.

[Csaba Dabis via Phabricator via cfe-commits]

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

Thanks! Is it working in dark-mode as expected?




Comment at: clang/utils/analyzer/exploded-graph-rewriter.py:647
 else:
+self._dump(' (%s)' % st.ptr)
 if prev_st is not None:

Extra space at the start of the string: `' <`.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65427



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

[PATCH] D65378: [analyzer][NFC] Refactoring BugReporter.cpp P1.: Store interesting symbols/regions in a simple set

[Artem Dergachev via Phabricator via cfe-commits]

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

I dunno why rC162028  was doing this. That's 
some advanced archeology down there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65378



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

[PATCH] D65427: [analyzer] exploded-graph-rewriter: Implement Store pointers.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ created this revision.
NoQ added a reviewer: Charusso.
Herald added subscribers: cfe-commits, dkrupp, donat.nagy, Szelethus, 
mikhail.ramalho, a.sidorin, szepet, baloghadamsoftware, xazax.hun.
Herald added a project: clang.

Those are useful for understanding contents of `LazyCompoundVal`s.

F9691913: Screen Shot 2019-07-29 at 5.12.36 PM.png 



Repository:
  rC Clang

https://reviews.llvm.org/D65427

Files:
  clang/test/Analysis/exploded-graph-rewriter/escapes.c
  clang/test/Analysis/exploded-graph-rewriter/store.dot
  clang/utils/analyzer/exploded-graph-rewriter.py


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -644,6 +644,7 @@
 if st is None:
 self._dump(' Nothing!')
 else:
+self._dump(' (%s)' % st.ptr)
 if prev_st is not None:
 if s.store.is_different(prev_st):
 self._dump('')
Index: clang/test/Analysis/exploded-graph-rewriter/store.dot
===
--- clang/test/Analysis/exploded-graph-rewriter/store.dot
+++ clang/test/Analysis/exploded-graph-rewriter/store.dot
@@ -4,6 +4,7 @@
 // UNSUPPORTED: system-windows
 
 // CHECK: Store: 
+// CHECK-SAME: (0x2)
 // CHECK-SAME: 
 // CHECK-SAME:   
 // CHECK-SAME: 
Index: clang/test/Analysis/exploded-graph-rewriter/escapes.c
===
--- clang/test/Analysis/exploded-graph-rewriter/escapes.c
+++ clang/test/Analysis/exploded-graph-rewriter/escapes.c
@@ -9,7 +9,7 @@
 // UNSUPPORTED: system-windows
 
 void escapes() {
-  // CHECK: Store: 
+  // CHECK: Store:  (0x{{[0-9a-f]*}})
   // CHECK-SAME: foo0
   // CHECK-SAME: Element\{"foo",0 S64b,char\}
   // CHECK: Environment: 


Index: clang/utils/analyzer/exploded-graph-rewriter.py
===
--- clang/utils/analyzer/exploded-graph-rewriter.py
+++ clang/utils/analyzer/exploded-graph-rewriter.py
@@ -644,6 +644,7 @@
 if st is None:
 self._dump(' Nothing!')
 else:
+self._dump(' (%s)' % st.ptr)
 if prev_st is not None:
 if s.store.is_different(prev_st):
 self._dump('')
Index: clang/test/Analysis/exploded-graph-rewriter/store.dot
===
--- clang/test/Analysis/exploded-graph-rewriter/store.dot
+++ clang/test/Analysis/exploded-graph-rewriter/store.dot
@@ -4,6 +4,7 @@
 // UNSUPPORTED: system-windows
 
 // CHECK: Store: 
+// CHECK-SAME: (0x2)
 // CHECK-SAME: 
 // CHECK-SAME:   
 // CHECK-SAME: 
Index: clang/test/Analysis/exploded-graph-rewriter/escapes.c
===
--- clang/test/Analysis/exploded-graph-rewriter/escapes.c
+++ clang/test/Analysis/exploded-graph-rewriter/escapes.c
@@ -9,7 +9,7 @@
 // UNSUPPORTED: system-windows
 
 void escapes() {
-  // CHECK: Store: 
+  // CHECK: Store:  (0x{{[0-9a-f]*}})
   // CHECK-SAME: foo0
   // CHECK-SAME: Element\{"foo",0 S64b,char\}
   // CHECK: Environment: 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65425: [clang-doc] Fix expected output in tests

[Diego Astiazarán via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367279: [clang-doc] Fix expected output in tests (authored 
by DiegoAstiazaran, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65425?vs=212248=212255#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65425

Files:
  clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp


Index: clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -171,10 +171,6 @@
   ClangDocContext CDCtx = getClangDocContext();
   auto Err = G->generateDocForInfo(, Actual, CDCtx);
   assert(!Err);
-  SmallString<16> PathToFloat;
-  llvm::sys::path::native("path/to/float.html", PathToFloat);
-  SmallString<16> PathToInt;
-  llvm::sys::path::native("path/to/int.html", PathToInt);
   std::string Expected = R"raw(
 
 
@@ -182,11 +178,9 @@
 
   f
   
-float
+float
  f(
-int
+int
  P)
   
   Defined at line 10 of test.cpp


Index: clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
+++ clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -171,10 +171,6 @@
   ClangDocContext CDCtx = getClangDocContext();
   auto Err = G->generateDocForInfo(, Actual, CDCtx);
   assert(!Err);
-  SmallString<16> PathToFloat;
-  llvm::sys::path::native("path/to/float.html", PathToFloat);
-  SmallString<16> PathToInt;
-  llvm::sys::path::native("path/to/int.html", PathToInt);
   std::string Expected = R"raw(
 
 
@@ -182,11 +178,9 @@
 
   f
   
-float
+float
  f(
-int
+int
  P)
   
   Defined at line 10 of test.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r367279 - [clang-doc] Fix expected output in tests

[Diego Astiazaran via cfe-commits]

Author: diegoastiazaran
Date: Mon Jul 29 17:07:34 2019
New Revision: 367279

URL: http://llvm.org/viewvc/llvm-project?rev=367279=rev
Log:
[clang-doc] Fix expected output in tests

Removes conversion of html paths in output. These will always be in
posix-style paths.

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

Modified:
clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp

Modified: clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp?rev=367279=367278=367279=diff
==
--- clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp (original)
+++ clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp Mon Jul 
29 17:07:34 2019
@@ -171,10 +171,6 @@ TEST(HTMLGeneratorTest, emitFunctionHTML
   ClangDocContext CDCtx = getClangDocContext();
   auto Err = G->generateDocForInfo(, Actual, CDCtx);
   assert(!Err);
-  SmallString<16> PathToFloat;
-  llvm::sys::path::native("path/to/float.html", PathToFloat);
-  SmallString<16> PathToInt;
-  llvm::sys::path::native("path/to/int.html", PathToInt);
   std::string Expected = R"raw(
 
 
@@ -182,11 +178,9 @@ TEST(HTMLGeneratorTest, emitFunctionHTML
 
   f
   
-float
+float
  f(
-int
+int
  P)
   
   Defined at line 10 of test.cpp


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

[PATCH] D65050: [SemaTemplate] Mark a function type as dependent when its parameter list contains pack expansion

[S. B. Tam via Phabricator via cfe-commits]

cpplearner added a comment.

ping


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

https://reviews.llvm.org/D65050



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

[PATCH] D65249: [NFC] use C++11 in AlignOf.h, remove AlignedCharArray

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D65249#1603431 , @BillyONeal wrote:

> (In fact I observe many patterns in this review like:
>
> enum { Foo = alignof(void*); }
>  aligned_storage_t<1234, Foo> x;
>
> and then a bunch of casting to treat it as a char buffer; if it was just born 
> as a char buffer you can remove both the casts and the enum hack:
>
> alignas(void*) char x[1234];


Good point. I went ahead and did that. The patch is now quite trivial :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65249



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

[PATCH] D65249: [NFC] use C++11 in AlignOf.h, remove AlignedCharArray

[JF Bastien via Phabricator via cfe-commits]

jfb updated this revision to Diff 212252.
jfb added a comment.

Significantly simplify the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65249

Files:
  llvm/include/llvm/Support/AlignOf.h
  llvm/unittests/Support/AlignOfTest.cpp

Index: llvm/unittests/Support/AlignOfTest.cpp
===
--- llvm/unittests/Support/AlignOfTest.cpp
+++ llvm/unittests/Support/AlignOfTest.cpp
@@ -233,16 +233,5 @@
 #ifndef _MSC_VER
   EXPECT_EQ(sizeof(V8), sizeof(AlignedCharArrayUnion));
 #endif
-
-  EXPECT_EQ(1u, (alignof(AlignedCharArray<1, 1>)));
-  EXPECT_EQ(2u, (alignof(AlignedCharArray<2, 1>)));
-  EXPECT_EQ(4u, (alignof(AlignedCharArray<4, 1>)));
-  EXPECT_EQ(8u, (alignof(AlignedCharArray<8, 1>)));
-  EXPECT_EQ(16u, (alignof(AlignedCharArray<16, 1>)));
-
-  EXPECT_EQ(1u, sizeof(AlignedCharArray<1, 1>));
-  EXPECT_EQ(7u, sizeof(AlignedCharArray<1, 7>));
-  EXPECT_EQ(2u, sizeof(AlignedCharArray<2, 2>));
-  EXPECT_EQ(16u, sizeof(AlignedCharArray<2, 16>));
 }
 } // end anonymous namespace
Index: llvm/include/llvm/Support/AlignOf.h
===
--- llvm/include/llvm/Support/AlignOf.h
+++ llvm/include/llvm/Support/AlignOf.h
@@ -6,7 +6,7 @@
 //
 //===--===//
 //
-// This file defines the AlignedCharArray and AlignedCharArrayUnion classes.
+// This file defines the AlignedCharArrayUnion class.
 //
 //===--===//
 
@@ -18,128 +18,38 @@
 
 namespace llvm {
 
-/// \struct AlignedCharArray
-/// Helper for building an aligned character array type.
-///
-/// This template is used to explicitly build up a collection of aligned
-/// character array types. We have to build these up using a macro and explicit
-/// specialization to cope with MSVC (at least till 2015) where only an
-/// integer literal can be used to specify an alignment constraint. Once built
-/// up here, we can then begin to indirect between these using normal C++
-/// template parameters.
-
-// MSVC requires special handling here.
-#ifndef _MSC_VER
-
-template
-struct AlignedCharArray {
-  alignas(Alignment) char buffer[Size];
-};
-
-#else // _MSC_VER
-
-/// Create a type with an aligned char buffer.
-template
-struct AlignedCharArray;
-
-// We provide special variations of this template for the most common
-// alignments because __declspec(align(...)) doesn't actually work when it is
-// a member of a by-value function argument in MSVC, even if the alignment
-// request is something reasonably like 8-byte or 16-byte. Note that we can't
-// even include the declspec with the union that forces the alignment because
-// MSVC warns on the existence of the declspec despite the union member forcing
-// proper alignment.
-
-template
-struct AlignedCharArray<1, Size> {
-  union {
-char aligned;
-char buffer[Size];
-  };
-};
-
-template
-struct AlignedCharArray<2, Size> {
-  union {
-short aligned;
-char buffer[Size];
-  };
-};
-
-template
-struct AlignedCharArray<4, Size> {
-  union {
-int aligned;
-char buffer[Size];
-  };
-};
+namespace detail {
 
-template
-struct AlignedCharArray<8, Size> {
-  union {
-double aligned;
-char buffer[Size];
-  };
+template  class AlignerImpl {
+  T t;
+  AlignerImpl rest;
+  AlignerImpl() = delete;
 };
 
-
-// The rest of these are provided with a __declspec(align(...)) and we simply
-// can't pass them by-value as function arguments on MSVC.
-
-#define LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(x) \
-  template \
-  struct AlignedCharArray { \
-__declspec(align(x)) char buffer[Size]; \
-  };
-
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(16)
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(32)
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(64)
-LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT(128)
-
-#undef LLVM_ALIGNEDCHARARRAY_TEMPLATE_ALIGNMENT
-
-#endif // _MSC_VER
-
-namespace detail {
-template 
-class AlignerImpl {
-  T1 t1; T2 t2; T3 t3; T4 t4; T5 t5; T6 t6; T7 t7; T8 t8; T9 t9; T10 t10;
-
+template  class AlignerImpl {
+  T t;
   AlignerImpl() = delete;
 };
 
-template 
-union SizerImpl {
-  char arr1[sizeof(T1)], arr2[sizeof(T2)], arr3[sizeof(T3)], arr4[sizeof(T4)],
-   arr5[sizeof(T5)], arr6[sizeof(T6)], arr7[sizeof(T7)], arr8[sizeof(T8)],
-   arr9[sizeof(T9)], arr10[sizeof(T10)];
+template  union SizerImpl {
+  char arr[sizeof(T)];
+  SizerImpl rest;
 };
+
+template  union SizerImpl { char arr[sizeof(T)]; };
 } // end namespace detail
 
-/// This union template exposes a suitably aligned and sized character
-/// array member which can hold elements of any of up to ten types.
+/// A suitably aligned and sized character array member which can hold elements
+/// of any type.
 ///
-/// These types may be arrays, structs, or any other types. The goal is to
-/// expose a char array 

[PATCH] D65426: [Coverage] Hide coverage for regions with incorrect end locations (PR39942)

[Vedant Kumar via Phabricator via cfe-commits]

vsk created this revision.
vsk added reviewers: rnk, phosek, manojgupta.

... instead of crashing.

The real bug here is due to clang generating coverage for a conditional
operator expanded from a macro. The location of the end of the
conditional expression is not correct. The actual fix for this requires
chasing down an issue in the lexer/preprocessor.


https://reviews.llvm.org/D65426

Files:
  clang/lib/CodeGen/CoverageMappingGen.cpp
  clang/test/CoverageMapping/bad-coverage-for-condexpr-in-macro-pr39942.cpp


Index: clang/test/CoverageMapping/bad-coverage-for-condexpr-in-macro-pr39942.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/bad-coverage-for-condexpr-in-macro-pr39942.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping 
-dump-coverage-mapping -emit-llvm-only -main-file-name pr39942.cpp %s | 
FileCheck %s
+
+class a;
+template  a <<(b &, const char *);
+int c;
+#define d(l) l(__FILE__, __LINE__, c)
+#define COMPACT_GOOGLE_LOG_ERROR d(e)
+#define f(g, condition) 0 ? (void)0 : h() & g
+#define i(j) COMPACT_GOOGLE_LOG_##j.g()
+#define k(j) f(i(j), )
+class e {
+public:
+  e(const char *, int, int);
+  a ();
+};
+class h {
+public:
+  void operator&(a &);
+};
+#define m(lib, func) \
+#func;   \
+  k(ERROR) << #func; \
+  return 0 // CHECK: File 1, [[@LINE-1]]:4 -> [[@LINE-1]]:4 = (#0 - #1)
+bool n() { m(, ); }
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -619,7 +619,14 @@
   MostRecentLocation = getIncludeOrExpansionLoc(EndLoc);
 
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
-assert(SpellingRegion(SM, Region).isInSourceOrder());
+
+// FIXME: See llvm.org/PR39942. The clang preprocessor may not preserve
+// the correct end location for conditional expressions. Hide coverage
+// for such regions instead of crashing/asserting. This should be
+// turned back into an assert.
+if (!SpellingRegion(SM, Region).isInSourceOrder())
+  Region.setEndLoc(Region.getBeginLoc());
+
 SourceRegions.push_back(Region);
 
 if (ParentOfDeferredRegion) {


Index: clang/test/CoverageMapping/bad-coverage-for-condexpr-in-macro-pr39942.cpp
===
--- /dev/null
+++ clang/test/CoverageMapping/bad-coverage-for-condexpr-in-macro-pr39942.cpp
@@ -0,0 +1,24 @@
+// RUN: %clang_cc1 -fprofile-instrument=clang -fcoverage-mapping -dump-coverage-mapping -emit-llvm-only -main-file-name pr39942.cpp %s | FileCheck %s
+
+class a;
+template  a <<(b &, const char *);
+int c;
+#define d(l) l(__FILE__, __LINE__, c)
+#define COMPACT_GOOGLE_LOG_ERROR d(e)
+#define f(g, condition) 0 ? (void)0 : h() & g
+#define i(j) COMPACT_GOOGLE_LOG_##j.g()
+#define k(j) f(i(j), )
+class e {
+public:
+  e(const char *, int, int);
+  a ();
+};
+class h {
+public:
+  void operator&(a &);
+};
+#define m(lib, func) \
+#func;   \
+  k(ERROR) << #func; \
+  return 0 // CHECK: File 1, [[@LINE-1]]:4 -> [[@LINE-1]]:4 = (#0 - #1)
+bool n() { m(, ); }
Index: clang/lib/CodeGen/CoverageMappingGen.cpp
===
--- clang/lib/CodeGen/CoverageMappingGen.cpp
+++ clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -619,7 +619,14 @@
   MostRecentLocation = getIncludeOrExpansionLoc(EndLoc);
 
 assert(SM.isWrittenInSameFile(Region.getBeginLoc(), EndLoc));
-assert(SpellingRegion(SM, Region).isInSourceOrder());
+
+// FIXME: See llvm.org/PR39942. The clang preprocessor may not preserve
+// the correct end location for conditional expressions. Hide coverage
+// for such regions instead of crashing/asserting. This should be
+// turned back into an assert.
+if (!SpellingRegion(SM, Region).isInSourceOrder())
+  Region.setEndLoc(Region.getBeginLoc());
+
 SourceRegions.push_back(Region);
 
 if (ParentOfDeferredRegion) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65425: [clang-doc] Fix expected output in tests

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran created this revision.
DiegoAstiazaran added a reviewer: juliehockett.
DiegoAstiazaran added a project: clang-tools-extra.

Removes conversion of html paths in output. These will always be in posix-style 
paths.


https://reviews.llvm.org/D65425

Files:
  clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp


Index: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
+++ clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -171,10 +171,6 @@
   ClangDocContext CDCtx = getClangDocContext();
   auto Err = G->generateDocForInfo(, Actual, CDCtx);
   assert(!Err);
-  SmallString<16> PathToFloat;
-  llvm::sys::path::native("path/to/float.html", PathToFloat);
-  SmallString<16> PathToInt;
-  llvm::sys::path::native("path/to/int.html", PathToInt);
   std::string Expected = R"raw(
 
 
@@ -182,11 +178,9 @@
 
   f
   
-float
+float
  f(
-int
+int
  P)
   
   Defined at line 10 of test.cpp


Index: clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
===
--- clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
+++ clang-tools-extra/unittests/clang-doc/HTMLGeneratorTest.cpp
@@ -171,10 +171,6 @@
   ClangDocContext CDCtx = getClangDocContext();
   auto Err = G->generateDocForInfo(, Actual, CDCtx);
   assert(!Err);
-  SmallString<16> PathToFloat;
-  llvm::sys::path::native("path/to/float.html", PathToFloat);
-  SmallString<16> PathToInt;
-  llvm::sys::path::native("path/to/int.html", PathToInt);
   std::string Expected = R"raw(
 
 
@@ -182,11 +178,9 @@
 
   f
   
-float
+float
  f(
-int
+int
  P)
   
   Defined at line 10 of test.cpp
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r367276 - Fix Linux build

[JF Bastien via cfe-commits]

Author: jfb
Date: Mon Jul 29 16:28:44 2019
New Revision: 367276

URL: http://llvm.org/viewvc/llvm-project?rev=367276=rev
Log:
Fix Linux build

r367274 broke it

Modified:
cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

Modified: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp?rev=367276=367275=367276=diff
==
--- cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp (original)
+++ cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp Mon Jul 29 
16:28:44 2019
@@ -188,7 +188,7 @@ void DirectoryWatcherLinux::InotifyPolli
 alignas(struct inotify_event) char buffer[EventBufferLength];
   };
   auto ManagedBuffer = llvm::make_unique();
-  char *const Buf = ManagedBuffer.buffer;
+  char *const Buf = ManagedBuffer->buffer;
 
   const int EpollFD = epoll_create1(EPOLL_CLOEXEC);
   if (EpollFD == -1) {


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

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Thanks for the suggestion. Sounds reasonably simple, I'll try this and report 
back with the result.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64799



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

r367274 - [NFC] avoid AlignedCharArray in clang

[JF Bastien via cfe-commits]

Author: jfb
Date: Mon Jul 29 16:12:48 2019
New Revision: 367274

URL: http://llvm.org/viewvc/llvm-project?rev=367274=rev
Log:
[NFC] avoid AlignedCharArray in clang

As discussed in D65249, don't use AlignedCharArray or std::aligned_storage. 
Just use alignas(X) char Buf[Size];. This will allow me to remove 
AlignedCharArray entirely, and works on the current minimum version of Visual 
Studio.

Modified:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/Sema/Overload.h
cfe/trunk/lib/CodeGen/CGCleanup.cpp
cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
cfe/trunk/lib/Sema/SemaOverload.cpp
cfe/trunk/lib/Sema/TypeLocBuilder.cpp
cfe/trunk/lib/Sema/TypeLocBuilder.h

Modified: cfe/trunk/include/clang/AST/Expr.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/AST/Expr.h?rev=367274=367273=367274=diff
==
--- cfe/trunk/include/clang/AST/Expr.h (original)
+++ cfe/trunk/include/clang/AST/Expr.h Mon Jul 29 16:12:48 2019
@@ -2619,9 +2619,8 @@ public:
   /// + sizeof(Stmt *) bytes of storage, aligned to alignof(CallExpr):
   ///
   /// \code{.cpp}
-  ///   llvm::AlignedCharArray Buffer;
-  ///   CallExpr *TheCall = CallExpr::CreateTemporary(Buffer.buffer, etc);
+  ///   alignas(CallExpr) char Buffer[sizeof(CallExpr) + sizeof(Stmt *)];
+  ///   CallExpr *TheCall = CallExpr::CreateTemporary(Buffer, etc);
   /// \endcode
   static CallExpr *CreateTemporary(void *Mem, Expr *Fn, QualType Ty,
ExprValueKind VK, SourceLocation RParenLoc,

Modified: cfe/trunk/include/clang/Sema/Overload.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Overload.h?rev=367274=367273=367274=diff
==
--- cfe/trunk/include/clang/Sema/Overload.h (original)
+++ cfe/trunk/include/clang/Sema/Overload.h Mon Jul 29 16:12:48 2019
@@ -881,7 +881,7 @@ class Sema;
 constexpr static unsigned NumInlineBytes =
 24 * sizeof(ImplicitConversionSequence);
 unsigned NumInlineBytesUsed = 0;
-llvm::AlignedCharArray InlineSpace;
+alignas(void *) char InlineSpace[NumInlineBytes];
 
 // Address space of the object being constructed.
 LangAS DestAS = LangAS::Default;
@@ -904,7 +904,7 @@ class Sema;
   unsigned NBytes = sizeof(T) * N;
   if (NBytes > NumInlineBytes - NumInlineBytesUsed)
 return SlabAllocator.Allocate(N);
-  char *FreeSpaceStart = InlineSpace.buffer + NumInlineBytesUsed;
+  char *FreeSpaceStart = InlineSpace + NumInlineBytesUsed;
   assert(uintptr_t(FreeSpaceStart) % alignof(void *) == 0 &&
  "Misaligned storage!");
 

Modified: cfe/trunk/lib/CodeGen/CGCleanup.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGCleanup.cpp?rev=367274=367273=367274=diff
==
--- cfe/trunk/lib/CodeGen/CGCleanup.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGCleanup.cpp Mon Jul 29 16:12:48 2019
@@ -740,14 +740,15 @@ void CodeGenFunction::PopCleanupBlock(bo
   // here. Unfortunately, if you ask for a SmallVector, the
   // alignment isn't sufficient.
   auto *CleanupSource = reinterpret_cast(Scope.getCleanupBuffer());
-  llvm::AlignedCharArray CleanupBufferStack;
+  alignas(EHScopeStack::ScopeStackAlignment) char
+  CleanupBufferStack[8 * sizeof(void *)];
   std::unique_ptr CleanupBufferHeap;
   size_t CleanupSize = Scope.getCleanupSize();
   EHScopeStack::Cleanup *Fn;
 
   if (CleanupSize <= sizeof(CleanupBufferStack)) {
-memcpy(CleanupBufferStack.buffer, CleanupSource, CleanupSize);
-Fn = reinterpret_cast(CleanupBufferStack.buffer);
+memcpy(CleanupBufferStack, CleanupSource, CleanupSize);
+Fn = reinterpret_cast(CleanupBufferStack);
   } else {
 CleanupBufferHeap.reset(new char[CleanupSize]);
 memcpy(CleanupBufferHeap.get(), CleanupSource, CleanupSize);

Modified: cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp?rev=367274=367273=367274=diff
==
--- cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp (original)
+++ cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp Mon Jul 29 
16:12:48 2019
@@ -184,10 +184,11 @@ void DirectoryWatcherLinux::InotifyPolli
   // the inotify file descriptor should have the same alignment as
   // struct inotify_event.
 
-  auto ManagedBuffer =
-  llvm::make_unique>();
-  char *const Buf = ManagedBuffer->buffer;
+  struct Buffer {
+alignas(struct inotify_event) char buffer[EventBufferLength];
+  };
+  auto ManagedBuffer = llvm::make_unique();
+  char *const Buf = ManagedBuffer.buffer;
 
   const int EpollFD = epoll_create1(EPOLL_CLOEXEC);
   if (EpollFD == -1) {
@@ 

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64799#1605211 , @ilya-biryukov 
wrote:

> @rsmith, emitting the typos on pop expression evaluation context resulted in 
> a bunch of failures when trying to transform the resulting errors,  see a 
> typical stacktrace below.
>  It seems we can actually pop expression evaluation contexts between 
> producing and correcting a typo expression.


I think I see the problem: if we have expression `E1` as a subexpression of 
`E2`, but `E1` has its own expression evaluation context (eg, maybe it's in a 
`sizeof` expression or similar), we'll now diagnose `E1`'s typos when we leave 
that context. That by itself seems fine, but then when we run typo correction 
for `E2`, we rediscover the typo from `E1` and are surprised to find that the 
correction information has been discarded but the `TypoExpr` is still reachable 
in the AST. It seems like we could handle that by tracking the 
already-corrected `TypoExpr`s and teaching `TransformTypos` to substitute in 
the known correction in this case rather than trying to pick a correction for 
itself.

> Would you be ok with landing the workaround as is? Alternatively, any ideas 
> on how we can avoid this problem without extending the scope of the patch too 
> much?

As above, I think we can probably fix this without changing too much by 
tracking which typos we've already resolved rather than throwing away the 
information about them. Failing that, I can live with this landing as-is, but 
delaying these diagnostics to the end of the file is going to result in a bad 
user experience whenever it happens -- and I suspect it'll start happening 
significantly more than the assert currently fails, because we'll no longer 
have this assertion forcing people to call `CorrectDelayedTyposInExpr` when 
discarding an expression.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64799



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

r367271 - [DependencyCollector] Make maybeAddDependency virtual (NFC)

[Jonas Devlieghere via cfe-commits]

Author: jdevlieghere
Date: Mon Jul 29 16:02:11 2019
New Revision: 367271

URL: http://llvm.org/viewvc/llvm-project?rev=367271=rev
Log:
[DependencyCollector] Make maybeAddDependency virtual (NFC)

Make DependencyCollector::maybeAddDependency, just like its other
methods, which I made virtual a while ago. The motivation for this
change is still the LLDB reproducer.

Modified:
cfe/trunk/include/clang/Frontend/Utils.h

Modified: cfe/trunk/include/clang/Frontend/Utils.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Frontend/Utils.h?rev=367271=367270=367271=diff
==
--- cfe/trunk/include/clang/Frontend/Utils.h (original)
+++ cfe/trunk/include/clang/Frontend/Utils.h Mon Jul 29 16:02:11 2019
@@ -99,11 +99,11 @@ public:
   /// Return true if system files should be passed to sawDependency().
   virtual bool needSystemDependencies() { return false; }
 
-  // implementation detail
   /// Add a dependency \p Filename if it has not been seen before and
   /// sawDependency() returns true.
-  void maybeAddDependency(StringRef Filename, bool FromModule, bool IsSystem,
-  bool IsModuleFile, bool IsMissing);
+  virtual void maybeAddDependency(StringRef Filename, bool FromModule,
+  bool IsSystem, bool IsModuleFile,
+  bool IsMissing);
 
 protected:
   /// Return true if the filename was added to the list of dependencies, false


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

r367270 - [docs] Add a note about where UBSan emits logs

[Vedant Kumar via cfe-commits]

Author: vedantk
Date: Mon Jul 29 15:54:43 2019
New Revision: 367270

URL: http://llvm.org/viewvc/llvm-project?rev=367270=rev
Log:
[docs] Add a note about where UBSan emits logs

Modified:
cfe/trunk/docs/UndefinedBehaviorSanitizer.rst

Modified: cfe/trunk/docs/UndefinedBehaviorSanitizer.rst
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/docs/UndefinedBehaviorSanitizer.rst?rev=367270=367269=367270=diff
==
--- cfe/trunk/docs/UndefinedBehaviorSanitizer.rst (original)
+++ cfe/trunk/docs/UndefinedBehaviorSanitizer.rst Mon Jul 29 15:54:43 2019
@@ -224,6 +224,12 @@ will need to:
``UBSAN_OPTIONS=print_stacktrace=1``.
 #. Make sure ``llvm-symbolizer`` binary is in ``PATH``.
 
+Logging
+===
+
+The default log file for diagnostics is "stderr". To log diagnostics to another
+file, you can set ``UBSAN_OPTIONS=log_path=...``.
+
 Silencing Unsigned Integer Overflow
 ===
 To silence reports from unsigned integer overflow, you can set


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

r367269 - [DebugInfo] Don't emit incorrect descriptions of thunk params (PR42627)

[Vedant Kumar via cfe-commits]

Author: vedantk
Date: Mon Jul 29 15:49:55 2019
New Revision: 367269

URL: http://llvm.org/viewvc/llvm-project?rev=367269=rev
Log:
[DebugInfo] Don't emit incorrect descriptions of thunk params (PR42627)

The `this` parameter of a thunk requires adjustment. Stop emitting an
incorrect dbg.declare pointing to the unadjusted pointer.

We could describe the adjusted value instead, but there may not be much
benefit in doing so as users tend not to debug thunks.

Robert O'Callahan reports that this matches gcc's behavior.

Fixes PR42627.

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

Modified:
cfe/trunk/lib/CodeGen/CGDecl.cpp
cfe/trunk/test/CodeGenCXX/thunks.cpp

Modified: cfe/trunk/lib/CodeGen/CGDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDecl.cpp?rev=367269=367268=367269=diff
==
--- cfe/trunk/lib/CodeGen/CGDecl.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDecl.cpp Mon Jul 29 15:49:55 2019
@@ -2496,10 +2496,11 @@ void CodeGenFunction::EmitParmDecl(const
 
   setAddrOfLocalVar(, DeclPtr);
 
-  // Emit debug info for param declaration.
+  // Emit debug info for param declarations in non-thunk functions.
   if (CGDebugInfo *DI = getDebugInfo()) {
 if (CGM.getCodeGenOpts().getDebugInfo() >=
-codegenoptions::LimitedDebugInfo) {
+codegenoptions::LimitedDebugInfo &&
+!CurFuncIsThunk) {
   DI->EmitDeclareOfArgVariable(, DeclPtr.getPointer(), ArgNo, Builder);
 }
   }

Modified: cfe/trunk/test/CodeGenCXX/thunks.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CodeGenCXX/thunks.cpp?rev=367269=367268=367269=diff
==
--- cfe/trunk/test/CodeGenCXX/thunks.cpp (original)
+++ cfe/trunk/test/CodeGenCXX/thunks.cpp Mon Jul 29 15:49:55 2019
@@ -1,4 +1,5 @@
 // RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm 
-o - | FileCheck --check-prefix=CHECK --check-prefix=CHECK-NONOPT %s
+// RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -debug-info-kind=standalone 
-dwarf-version=5 -munwind-tables -emit-llvm -o - | FileCheck 
--check-prefix=CHECK --check-prefix=CHECK-NONOPT --check-prefix=CHECK-DBG %s
 // RUN: %clang_cc1 %s -triple=x86_64-pc-linux-gnu -munwind-tables -emit-llvm 
-o - -O1 -disable-llvm-passes | FileCheck --check-prefix=CHECK 
--check-prefix=CHECK-OPT %s
 
 namespace Test1 {
@@ -20,6 +21,8 @@ struct C : A, B {
 };
 
 // CHECK-LABEL: define void @_ZThn8_N5Test11C1fEv(
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 void C::f() { }
 
 }
@@ -38,6 +41,8 @@ struct B : virtual A {
 };
 
 // CHECK-LABEL: define void @_ZTv0_n24_N5Test21B1fEv(
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 void B::f() { }
 
 }
@@ -83,6 +88,8 @@ struct __attribute__((visibility("protec
 };
 
 // CHECK-LABEL: define protected void @_ZThn8_N5Test41C1fEv(
+// CHECK-DBG-NOT: dbg.declare
+// CHECK: ret void
 void C::f() { }
 
 }
@@ -166,6 +173,7 @@ namespace Test6 {
   };
 
   // CHECK-LABEL: define void @_ZThn16_N5Test66Thunks1fEv
+   // CHECK-DBG-NOT: dbg.declare
   // CHECK-NOT: memcpy
   // CHECK: {{call void @_ZN5Test66Thunks1fEv.*sret}}
   // CHECK: ret void
@@ -212,6 +220,7 @@ namespace Test7 {
   void D::baz(X, X&, _Complex float, Small, Small&, Large) { }
 
   // CHECK-LABEL: define void 
@_ZThn8_N5Test71D3bazENS_1XERS1_CfNS_5SmallERS4_NS_5LargeE(
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK-NOT: memcpy
   // CHECK: ret void
   void testD() { D d; }
@@ -227,6 +236,7 @@ namespace Test8 {
   void C::helper(NonPOD var) {}
 
   // CHECK-LABEL: define void @_ZThn8_N5Test81C3barENS_6NonPODE(
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK-NOT: load [[NONPODTYPE]], [[NONPODTYPE]]*
   // CHECK-NOT: memcpy
   // CHECK: ret void
@@ -269,10 +279,14 @@ namespace Test11 {
   //  The this-adjustment and return-adjustment thunk required when
   //  C::f appears in a vtable where A is at a nonzero offset from C.
   // CHECK: define {{.*}} @_ZTcv0_n24_v0_n32_N6Test111C1fEv(
+  // CHECK-DBG-NOT: dbg.declare
+  // CHECK: ret
 
   //  The return-adjustment thunk required when C::f appears in a vtable
   //  where A is at a zero offset from C.
   // CHECK: define {{.*}} @_ZTch0_v0_n32_N6Test111C1fEv(
+  // CHECK-DBG-NOT: dbg.declare
+  // CHECK: ret
 }
 
 // Varargs thunk test.
@@ -295,6 +309,7 @@ namespace Test12 {
   // Varargs thunk; check that both the this and covariant adjustments
   // are generated.
   // CHECK: define {{.*}} @_ZTchn8_h8_N6Test121C1fEiz
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -8
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 8
 }
@@ -318,6 +333,7 @@ namespace Test13 {
 return *this;
   }
   // CHECK: define {{.*}} @_ZTcvn8_n32_v8_n24_N6Test131D4foo1Ev
+  // CHECK-DBG-NOT: dbg.declare
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -8
   // CHECK: getelementptr inbounds i8, i8* {{.*}}, i64 -32
   // 

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur updated this revision to Diff 212237.
xur marked an inline comment as done.
xur added a comment.

Integrated Chandler's review comments.


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

https://reviews.llvm.org/D64029

Files:
  clang/lib/CodeGen/BackendUtil.cpp
  clang/test/Profile/Inputs/gcc-flag-compatibility_IR.proftext
  clang/test/Profile/gcc-flag-compatibility.c
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/test/Other/new-pm-pgo-O0.ll

Index: llvm/test/Other/new-pm-pgo-O0.ll
===
--- /dev/null
+++ llvm/test/Other/new-pm-pgo-O0.ll
@@ -0,0 +1,21 @@
+; RUN: opt -debug-pass-manager -passes='default' -pgo-kind=pgo-instr-gen-pipeline -profile-file='temp' %s 2>&1 |FileCheck %s --check-prefixes=GEN
+; RUN: llvm-profdata merge %S/Inputs/new-pm-pgo.proftext -o %t.profdata
+; RUN: opt -debug-pass-manager -passes='default' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 |FileCheck %s --check-prefixes=USE_DEFAULT,USE
+; RUN: opt -debug-pass-manager -passes='thinlto-pre-link' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_PRE_LINK,USE
+; RUN: opt -debug-pass-manager -passes='lto-pre-link' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_PRE_LINK,USE
+; RUN: opt -debug-pass-manager -passes='thinlto' -pgo-kind=pgo-instr-use-pipeline -profile-file='%t.profdata' %s 2>&1 \
+; RUN: |FileCheck %s --check-prefixes=USE_POST_LINK,USE
+
+;
+; GEN: Running pass: PGOInstrumentationGen
+; USE_DEFAULT: Running pass: PGOInstrumentationUse
+; USE_PRE_LINK: Running pass: PGOInstrumentationUse
+; USE_POST_LINK-NOT: Running pass: PGOInstrumentationUse
+; USE-NOT: Running pass: PGOIndirectCallPromotion
+; USE-NOT: Running pass: PGOMemOPSizeOpt
+
+define void @foo() {
+  ret void
+}
Index: llvm/lib/Passes/PassBuilder.cpp
===
--- llvm/lib/Passes/PassBuilder.cpp
+++ llvm/lib/Passes/PassBuilder.cpp
@@ -541,6 +541,7 @@
 bool RunProfileGen, bool IsCS,
 std::string ProfileFile,
 std::string ProfileRemappingFile) {
+  assert(Level != O0 && "Not expecting O0 here!");
   // Generally running simplification passes and the inliner with an high
   // threshold results in smaller executables, but there may be cases where
   // the size grows, so let's be conservative here and skip this simplification
@@ -571,34 +572,62 @@
 CGPipeline.addPass(createCGSCCToFunctionPassAdaptor(std::move(FPM)));
 
 MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPipeline)));
+
+// Delete anything that is now dead to make sure that we don't instrument
+// dead code. Instrumentation can end up keeping dead code around and
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
   }
 
-  // Delete anything that is now dead to make sure that we don't instrument
-  // dead code. Instrumentation can end up keeping dead code around and
-  // dramatically increase code size.
-  MPM.addPass(GlobalDCEPass());
+  if (!RunProfileGen) {
+assert(!ProfileFile.empty() && "Profile use expecting a profile file!");
+MPM.addPass(PGOInstrumentationUse(ProfileFile, ProfileRemappingFile, IsCS));
+// Cache ProfileSummaryAnalysis once to avoid the potential need to insert
+// RequireAnalysisPass for PSI before subsequent non-module passes.
+MPM.addPass(RequireAnalysisPass());
+return;
+  }
 
-  if (RunProfileGen) {
-MPM.addPass(PGOInstrumentationGen(IsCS));
+  // Perform PGO instrumentation.
+  MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
-
-// Add the profile lowering pass.
-InstrProfOptions Options;
-if (!ProfileFile.empty())
-  Options.InstrProfileOutput = ProfileFile;
-Options.DoCounterPromotion = true;
-Options.UseBFIInPromotion = IsCS;
-MPM.addPass(InstrProfiling(Options, IsCS));
-  } else if (!ProfileFile.empty()) {
+  FunctionPassManager FPM;
+  FPM.addPass(createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
+  MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+
+  // Add the profile lowering pass.
+  InstrProfOptions Options;
+  if (!ProfileFile.empty())
+Options.InstrProfileOutput = ProfileFile;
+  // Do counter promotion at Level greater than O0.
+  Options.DoCounterPromotion = true;
+  Options.UseBFIInPromotion = IsCS;
+  MPM.addPass(InstrProfiling(Options, IsCS));
+}
+
+void PassBuilder::addPGOInstrPassesForO0(ModulePassManager ,
+ bool DebugLogging, bool 

[PATCH] D64029: [PGO] Add PGO support at -O0 in the experimental new pass manager

[Rong Xu via Phabricator via cfe-commits]

xur marked 3 inline comments as done.
xur added a comment.

I'm sorry that I missed this review for this long!

In D64029#1581952 , @chandlerc wrote:

> Sorry for the delay here.
>
> It'd be nice to land the LLVM patch first and with its own testing -- we 
> should have testing for the pass builder independent of Clang (IE, in the 
> LLVM tests).
>
> One option would be to test it with a unittest, not sure we have pass builder 
> unittests at the moment.


I initially had the pipeline change for this. But clang did not use the default 
pipeline, so I removed that change.
But this makes sense now to me with a unittest.

> Probably a better option would be to define a pseudo pass name like our 
> `default`, maybe `pgo`. Then you can parse the `O0` the same way we 
> do for the `default` thing. when it is O0 you can call the new routine I've 
> suggested below. When it is higher, you can call the existing routine much 
> like the default pipeline code does. This would all be implemented inside the 
> pass builder so no issues w/ using the utility code there.
> 
> This would let us much more nicely write tests for the pipeline fragment 
> generated for PGO both at O0 and above -- we have tests that specifically 
> print out the pipeline sequence. This makes it very easy to visualize changes 
> to the pipeline. And it would let you test the O0 code path here.

hmm. I think PGO is orthogonal to default pipeline: we now have prefixes of 
"default", "thinlto-pre-link", "thinlto", "lto", "lto-pre-link". They all can 
work with PGO enabled. It seems to me PGO will not cover all of them.

> The clang part of the patch (once adapted to the narrower API) seems likely 
> to be good then.
> 
> I'm happy for this to be two patches (first llvm, then Clang), or one patch. 
> Whatever is easier for you.

Great! I would prefer to have only one patch -- that would be easier.

In D64029#1581952 , @chandlerc wrote:

> Sorry for the delay here.
>
> It'd be nice to land the LLVM patch first and with its own testing -- we 
> should have testing for the pass builder independent of Clang (IE, in the 
> LLVM tests).
>
> One option would be to test it with a unittest, not sure we have pass builder 
> unittests at the moment.
>
> Probably a better option would be to define a pseudo pass name like our 
> `default`, maybe `pgo`. Then you can parse the `O0` the same way we 
> do for the `default` thing. when it is O0 you can call the new routine I've 
> suggested below. When it is higher, you can call the existing routine much 
> like the default pipeline code does. This would all be implemented inside the 
> pass builder so no issues w/ using the utility code there.
>
> This would let us much more nicely write tests for the pipeline fragment 
> generated for PGO both at O0 and above -- we have tests that specifically 
> print out the pipeline sequence. This makes it very easy to visualize changes 
> to the pipeline. And it would let you test the O0 code path here.
>
> The clang part of the patch (once adapted to the narrower API) seems likely 
> to be good then.
>
> I'm happy for this to be two patches (first llvm, then Clang), or one patch. 
> Whatever is easier for you.







Comment at: llvm/lib/Passes/PassBuilder.cpp:576
+// dramatically increase code size.
+MPM.addPass(GlobalDCEPass());
+  }

chandlerc wrote:
> xur wrote:
> > I moved this pass under the condition when Early inline is enabled. I'm 
> > under the impression that the intention is to clean up the code for the 
> > inline.
> > Chandler: you added this pass. If you don't like this move, I can pull it 
> > out and put it under another (Level > 0) condition.
> This makes sense to me. It was probably just transcription error on my part.
Got it.



Comment at: llvm/lib/Passes/PassBuilder.cpp:579-602
   if (RunProfileGen) {
 MPM.addPass(PGOInstrumentationGen(IsCS));
 
-FunctionPassManager FPM;
-FPM.addPass(
-createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));
-MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+if (Level > 0) {
+  FunctionPassManager FPM;
+  FPM.addPass(
+  createFunctionToLoopPassAdaptor(LoopRotatePass(), DebugLogging));

chandlerc wrote:
> Rather than moving the entire routine that is really only intended for the 
> innards of the pass manager to be public, I'd do something a bit more 
> narrow...
> 
> Maybe just add pipeline method to the pass builder to generate a minimal 
> pipeline suitable for O0 usage?
> 
> I think this code will be simpler to read w/o having to have the O0 
> conditions plumbed all the way through it, and the duplication is tiny.
> 
> If you want, could pull out the use bits which are common and use early exit 
> to make the code more clear:
> 
> ```
> if (!RunProfileGen) {
>   addPGOUsePasses(...);
>   return;
> 

[PATCH] D65349: [analyzer] Be more careful with destructors of non-regions.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

In D65349#1604363 , 
@baloghadamsoftware wrote:

> Is there any real-world use-case for casting concrete integers to class 
> instances? How did you find this crashing case?


I think in original code this value was produced by doing pointer arithmetic 
over a null pointer. Which is kinda weird because we normally mis-model such 
arithmetic as resulting in a null pointer, so that to treat dereferences of 
such pointers as null dereferences (and abort the analysis immediately, never 
reaching the destructor). See also D37478 .

Also it seems that this bug has just been independently reported as 
https://bugs.llvm.org/show_bug.cgi?id=42816.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65349



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

[PATCH] D65419: [clang-doc] Fix failing tests on Windows

[Diego Astiazarán via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367264: [clang-doc] Fix failing tests on Windows (authored 
by DiegoAstiazaran, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65419?vs=212225=212234#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65419

Files:
  clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp


Index: clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
===
--- clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
+++ clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
@@ -252,6 +252,8 @@
   llvm::SmallString<128> Path =
   computeRelativePath(Type.Path, CurrentDirectory);
   llvm::sys::path::append(Path, Type.Name + ".html");
+  // Paths in HTML must be in posix-style
+  llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
   return genLink(Type.Name, Path);
 }
 


Index: clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
===
--- clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
+++ clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
@@ -252,6 +252,8 @@
   llvm::SmallString<128> Path =
   computeRelativePath(Type.Path, CurrentDirectory);
   llvm::sys::path::append(Path, Type.Name + ".html");
+  // Paths in HTML must be in posix-style
+  llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
   return genLink(Type.Name, Path);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang-tools-extra] r367264 - [clang-doc] Fix failing tests on Windows

[Diego Astiazaran via cfe-commits]

Author: diegoastiazaran
Date: Mon Jul 29 15:03:39 2019
New Revision: 367264

URL: http://llvm.org/viewvc/llvm-project?rev=367264=rev
Log:
[clang-doc] Fix failing tests on Windows

Tests on Windows were failing due to path separator differences.
Links in HTML should use posix-style paths.

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

Modified:
clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp

Modified: clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp?rev=367264=367263=367264=diff
==
--- clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp (original)
+++ clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp Mon Jul 29 15:03:39 2019
@@ -252,6 +252,8 @@ static std::unique_ptr genType
   llvm::SmallString<128> Path =
   computeRelativePath(Type.Path, CurrentDirectory);
   llvm::sys::path::append(Path, Type.Name + ".html");
+  // Paths in HTML must be in posix-style
+  llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
   return genLink(Type.Name, Path);
 }
 


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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

For the `&&` vs `||` and `&` vs `|` warnings, it seems extremely implausible to 
me that they meet the "few or no false-positives" criterion for an 
enabled-by-default warning. Even assuming that people don't know the relative 
precedences of those operators, we'd expect a false-positive rate of at least 
50%. So disabling those by default seems reasonable to me, and in line with 
prior guidance for what makes a good on-by-default warning.

For the other two changes, I'm not yet convinced we should change the default, 
but could be convinced by data about false positive rates.




Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5647-5650
 def warn_overloaded_shift_in_comparison :Warning<
   "overloaded operator %select{>>|<<}0 has higher precedence than "
   "comparison operator">,
+  InGroup, DefaultIgnore;

I think this should remain enabled by default unless you have evidence of false 
positives. In my experience, this catches bugs like

```
ostream << "got expected result: " << x == 0;
```

... and very little else.

That said, perhaps we could do better here, since this warning is typically 
followed by an error: if we detect the special case of overload resolution 
failure for an operator with an `<<` (or `>>`) operator expression on its 
left-hand side, we could produce a much more targeted diagnostic for this case 
and avoid the current situation of a warning followed by an error for the same 
problem. If we did that, we could probably remove this warning entirely.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:5654-5656
 def warn_addition_in_bitshift : Warning<
   "operator '%0' has lower precedence than '%1'; "
+  "'%1' will be evaluated first">, InGroup, DefaultIgnore;

Do you have evidence that this warning has a significant false-positive rate? 
In my experience it's very common for people to think of `<<` as being a 
multiplication-like operator and be surprised when it turns out to have lower 
precedence than addition.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

[Eric Fiselier via Phabricator via cfe-commits]

EricWF closed this revision.
EricWF added a comment.

Committed in r367263. Thanks for the change, sorry about the delay.

I'll go watch the bots now.


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

https://reviews.llvm.org/D62977



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

[clang-tools-extra] r367263 - [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

[Eric Fiselier via cfe-commits]

Author: ericwf
Date: Mon Jul 29 14:38:56 2019
New Revision: 367263

URL: http://llvm.org/viewvc/llvm-project?rev=367263=rev
Log:
[clang-tidy]: Google: new check 'google-upgrade-googletest-case'

Introduce a new check to upgrade user code based on API changes in Googletest.

The check finds uses of old Googletest APIs with "case" in their name and 
replaces them with the new APIs named with "suite".

Patch by Alex Strelnikov (st...@google.com)
Reviewed as D62977.

Added:
clang-tools-extra/trunk/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
clang-tools-extra/trunk/clang-tidy/google/UpgradeGoogletestCaseCheck.h

clang-tools-extra/trunk/docs/clang-tidy/checks/google-upgrade-googletest-case.rst
clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/
clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/gtest-typed-test.h
clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/gtest.h
clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/nosuite/
clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/nosuite/gtest/

clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest-typed-test.h
clang-tools-extra/trunk/test/clang-tidy/Inputs/gtest/nosuite/gtest/gtest.h
clang-tools-extra/trunk/test/clang-tidy/google-upgrade-googletest-case.cpp
Modified:
clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp
clang-tools-extra/trunk/docs/ReleaseNotes.rst
clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

Modified: clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt?rev=367263=367262=367263=diff
==
--- clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt (original)
+++ clang-tools-extra/trunk/clang-tidy/google/CMakeLists.txt Mon Jul 29 
14:38:56 2019
@@ -17,6 +17,7 @@ add_clang_library(clangTidyGoogleModule
   OverloadedUnaryAndCheck.cpp
   TodoCommentCheck.cpp
   UnnamedNamespaceInHeaderCheck.cpp
+  UpgradeGoogletestCaseCheck.cpp
   UsingNamespaceDirectiveCheck.cpp
 
   LINK_LIBS

Modified: clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp?rev=367263=367262=367263=diff
==
--- clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp (original)
+++ clang-tools-extra/trunk/clang-tidy/google/GoogleTidyModule.cpp Mon Jul 29 
14:38:56 2019
@@ -27,6 +27,7 @@
 #include "OverloadedUnaryAndCheck.h"
 #include "TodoCommentCheck.h"
 #include "UnnamedNamespaceInHeaderCheck.h"
+#include "UpgradeGoogletestCaseCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 
 using namespace clang::ast_matchers;
@@ -79,6 +80,8 @@ class GoogleModule : public ClangTidyMod
 CheckFactories
 .registerCheck(
 "google-readability-namespace-comments");
+CheckFactories.registerCheck(
+"google-upgrade-googletest-case");
   }
 
   ClangTidyOptions getModuleOptions() override {

Added: clang-tools-extra/trunk/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp
URL: 
http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp?rev=367263=auto
==
--- clang-tools-extra/trunk/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp 
(added)
+++ clang-tools-extra/trunk/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp 
Mon Jul 29 14:38:56 2019
@@ -0,0 +1,354 @@
+//===--- UpgradeGoogletestCaseCheck.cpp - clang-tidy 
--===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 
Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+
+#include "UpgradeGoogletestCaseCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace google {
+
+static const llvm::StringRef RenameCaseToSuiteMessage =
+"Google Test APIs named with 'case' are deprecated; use equivalent APIs "
+"named with 'suite'";
+
+static llvm::Optional
+getNewMacroName(llvm::StringRef MacroName) {
+  std::pair ReplacementMap[] = {
+  {"TYPED_TEST_CASE", "TYPED_TEST_SUITE"},
+  {"TYPED_TEST_CASE_P", "TYPED_TEST_SUITE_P"},
+  {"REGISTER_TYPED_TEST_CASE_P", "REGISTER_TYPED_TEST_SUITE_P"},
+  {"INSTANTIATE_TYPED_TEST_CASE_P", "INSTANTIATE_TYPED_TEST_SUITE_P"},
+  {"INSTANTIATE_TEST_CASE_P", "INSTANTIATE_TEST_SUITE_P"},
+  };
+
+  for (auto  : ReplacementMap) {
+if (MacroName == Mapping.first)
+  

Re: [clang-tools-extra] r367137 - [clang-format] Fix style of css file paths

[Diego Astiazarán via cfe-commits]

Hello Galina,

Sorry about that, I didn't notice that test wasn't passing on Windows.
I just created this revision  that should
fix the issue.

Thanks,
Diego

On Mon, Jul 29, 2019 at 12:12 PM Galina Kistanova 
wrote:

> Hello Diego,
>
> This commit added broken test to one of our win builders:
>
>
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/27202
>
> . . .
> Failing Tests (4):
> Extra Tools Unit Tests ::
> clang-doc/./ClangDocTests.exe/HTMLGeneratorTest.emitRecordHTML
> . . .
>
> Please have a look?
> These builder was already red and did not send any notifications.
>
> Thanks
>
> Galina
>
> On Fri, Jul 26, 2019 at 11:02 AM Diego Astiazaran via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
>
>> Author: diegoastiazaran
>> Date: Fri Jul 26 11:02:42 2019
>> New Revision: 367137
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=367137=rev
>> Log:
>> [clang-format] Fix style of css file paths
>>
>> CSS files included in HTML should have a path in posix style, it should
>> not be different for Windows.
>>
>> Differential Revision: https://reviews.llvm.org/D65309
>>
>> Modified:
>> clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
>> clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
>>
>> Modified: clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp?rev=367137=367136=367137=diff
>>
>> ==
>> --- clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp (original)
>> +++ clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp Fri Jul 26
>> 11:02:42 2019
>> @@ -231,6 +231,8 @@ genStylesheetsHTML(StringRef InfoPath, c
>>  SmallString<128> StylesheetPath = computeRelativePath("", InfoPath);
>>  llvm::sys::path::append(StylesheetPath,
>>  llvm::sys::path::filename(FilePath));
>> +// Paths in HTML must be in posix-style
>> +llvm::sys::path::native(StylesheetPath,
>> llvm::sys::path::Style::posix);
>>  LinkNode->Attributes.try_emplace("href", StylesheetPath);
>>  Out.emplace_back(std::move(LinkNode));
>>}
>>
>> Modified:
>> clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
>> URL:
>> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp?rev=367137=367136=367137=diff
>>
>> ==
>> --- clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
>> (original)
>> +++ clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp Fri
>> Jul 26 11:02:42 2019
>> @@ -110,34 +110,23 @@ TEST(HTMLGeneratorTest, emitRecordHTML)
>>ClangDocContext CDCtx = getClangDocContext();
>>auto Err = G->generateDocForInfo(, Actual, CDCtx);
>>assert(!Err);
>> -  SmallString<16> PathToF;
>> -  llvm::sys::path::native("../../../path/to/F.html", PathToF);
>> -  SmallString<16> PathToInt;
>> -  llvm::sys::path::native("../int.html", PathToInt);
>> -  SmallString<16> PathToSylesheet;
>> -  llvm::sys::path::native("../../../clang-doc-default-stylesheet.css",
>> -  PathToSylesheet);
>>std::string Expected = R"raw(
>>  
>>  class r
>> -> - std::string(PathToSylesheet.str()) +
>> - R"raw("/>
>> +
>>  
>>class r
>>Defined at line 10 of test.cpp
>>
>>  Inherits from
>> -> - R"raw(">F
>> +F
>>  , G
>>
>>Members
>>
>>  
>>private
>> -  > - R"raw(">int
>> +  int
>> X
>>  
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65419: [clang-doc] Fix failing tests on Windows

[Diego Astiazarán via Phabricator via cfe-commits]

DiegoAstiazaran created this revision.
DiegoAstiazaran added a reviewer: juliehockett.
DiegoAstiazaran added a project: clang-tools-extra.

Tests on Windows were failing due to path separator differences.
Links in HTML should use posix-style paths.


https://reviews.llvm.org/D65419

Files:
  clang-tools-extra/clang-doc/HTMLGenerator.cpp


Index: clang-tools-extra/clang-doc/HTMLGenerator.cpp
===
--- clang-tools-extra/clang-doc/HTMLGenerator.cpp
+++ clang-tools-extra/clang-doc/HTMLGenerator.cpp
@@ -252,6 +252,8 @@
   llvm::SmallString<128> Path =
   computeRelativePath(Type.Path, CurrentDirectory);
   llvm::sys::path::append(Path, Type.Name + ".html");
+  // Paths in HTML must be in posix-style
+  llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
   return genLink(Type.Name, Path);
 }
 


Index: clang-tools-extra/clang-doc/HTMLGenerator.cpp
===
--- clang-tools-extra/clang-doc/HTMLGenerator.cpp
+++ clang-tools-extra/clang-doc/HTMLGenerator.cpp
@@ -252,6 +252,8 @@
   llvm::SmallString<128> Path =
   computeRelativePath(Type.Path, CurrentDirectory);
   llvm::sys::path::append(Path, Type.Name + ".html");
+  // Paths in HTML must be in posix-style
+  llvm::sys::path::native(Path, llvm::sys::path::Style::posix);
   return genLink(Type.Name, Path);
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64799: [Sema] Emit diagnostics for uncorrected delayed typos at the end of TU

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

@rsmith, emitting the typos on pop expression evaluation context resulted in a 
bunch of failures when trying to transform the resulting errors,  see a typical 
stacktrace below.
It seems we can actually pop expression evaluation contexts between producing 
and correcting a typo expression.

I don't see how to workaround this without digging further into typo correction 
and changing its design.
Would you be ok with landing the workaround as is? Alternatively, any ideas on 
how we can avoid this problem without extending the scope of the patch too much?

  FAIL: Clang :: CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp (1595 of 
15306)
   TEST 'Clang :: 
CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp' FAILED 
  Script:
  --
  : 'RUN: at line 1';   
/usr/local/google/home/ibiryukov/projects/llvm/build-rel/bin/clang -cc1 
-internal-isystem 
/usr/local/google/home/ibiryukov/projects/llvm/build-rel/lib/clang/10.0.0/include
 -nostdsysteminc -std=c++11 -verify 
/usr/local/google/home/ibiryukov/projects/llvm/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
  --
  Exit Code: 134
  
  Command Output (stderr):
  --
  clang: 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaLookup.cpp:5378:
 const Sema::TypoExprState ::Sema::getTypoExprState(clang::TypoExpr *) 
const: Assertion `Entry != DelayedTypos.end() && "Failed to get the state for a 
TypoExpr!"' failed.
  Stack dump:
  0.  Program arguments: 
/usr/local/google/home/ibiryukov/projects/llvm/build-rel/bin/clang -cc1 
-internal-isystem 
/usr/local/google/home/ibiryukov/projects/llvm/build-rel/lib/clang/10.0.0/include
 -nostdsysteminc -std=c++11 -verify 
/usr/local/google/home/ibiryukov/projects/llvm/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp
  1.  
/usr/local/google/home/ibiryukov/projects/llvm/clang/test/CXX/temp/temp.decls/temp.class/temp.mem.enum/p1.cpp:23:26:
 current parser token ';'
   #0 0x043e1ea4 PrintStackTrace 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Unix/Signals.inc:533:13
   #1 0x043e1ea4 PrintStackTraceSignalHandler(void*) 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Unix/Signals.inc:593:0
   #2 0x043dfa9e llvm::sys::RunSignalHandlers() 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Signals.cpp:69:18
   #3 0x043e22b8 SignalHandler(int) 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/lib/Support/Unix/Signals.inc:385:1
   #4 0x7fedabb7d3a0 __restore_rt 
(/lib/x86_64-linux-gnu/libpthread.so.0+0x123a0)
   #5 0x7fedaac0bcfb raise (/lib/x86_64-linux-gnu/libc.so.6+0x36cfb)
   #6 0x7fedaabf68ad abort (/lib/x86_64-linux-gnu/libc.so.6+0x218ad)
   #7 0x7fedaabf677f (/lib/x86_64-linux-gnu/libc.so.6+0x2177f)
   #8 0x7fedaac04542 (/lib/x86_64-linux-gnu/libc.so.6+0x2f542)
   #9 0x05f4ba2e LookupBucketFor 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/include/llvm/ADT/DenseMap.h:618:5
  #10 0x05f4ba2e find 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/include/llvm/ADT/DenseMap.h:184:0
  #11 0x05f4ba2e find 
/usr/local/google/home/ibiryukov/projects/llvm/llvm/include/llvm/ADT/MapVector.h:154:0
  #12 0x05f4ba2e clang::Sema::getTypoExprState(clang::TypoExpr*) const 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaLookup.cpp:5376:0
  #13 0x05e94695 TransformTypoExpr 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7740:27
  #14 0x05e94695 clang::TreeTransform<(anonymous 
namespace)::TransformTypos>::TransformExpr(clang::Expr*) 
/usr/local/google/home/ibiryukov/projects/llvm/build-rel/tools/clang/include/clang/AST/StmtNodes.inc:1277:0
  #15 0x05e708ea hasErrorOccurred 
/usr/local/google/home/ibiryukov/projects/llvm/clang/include/clang/Sema/Sema.h:7864:38
  #16 0x05e708ea TryTransform 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7651:0
  #17 0x05e708ea Transform 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7686:0
  #18 0x05e708ea clang::Sema::CorrectDelayedTyposInExpr(clang::Expr*, 
clang::VarDecl*, llvm::function_ref 
(clang::Expr*)>) 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaExprCXX.cpp:7784:0
  #19 0x05c5deae isInvalid 
/usr/local/google/home/ibiryukov/projects/llvm/clang/include/clang/Sema/Ownership.h:208:37
  #20 0x05c5deae clang::Sema::AddInitializerToDecl(clang::Decl*, 
clang::Expr*, bool) 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Sema/SemaDecl.cpp:11236:0
  #21 0x05a22997 
clang::Parser::ParseDeclarationAfterDeclaratorAndAttributes(clang::Declarator&, 
clang::Parser::ParsedTemplateInfo const&, clang::Parser::ForRangeInit*) 
/usr/local/google/home/ibiryukov/projects/llvm/clang/lib/Parse/ParseDecl.cpp:2456:3
  #22 0x05a2093c 

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: test/OpenMP/declare_mapper_codegen.cpp:44-48
+// CK0-DAG: store i8* %0, i8** [[HANDLEADDR:%[^,]+]]
+// CK0-DAG: store i8* %1, i8** [[BPTRADDR:%[^,]+]]
+// CK0-DAG: store i8* %2, i8** [[VPTRADDR:%[^,]+]]
+// CK0-DAG: store i64 %3, i{{64|32}}* [[SIZEADDR:%[^,]+]]
+// CK0-DAG: store i64 %4, i64* [[TYPEADDR:%[^,]+]]

lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > lildmh wrote:
> > > > > ABataev wrote:
> > > > > > I would not rely on the predetermined indices here, better to use 
> > > > > > some kind of patterns here just like in other places.
> > > > > Could you give an example about what you suggest? For instance, some 
> > > > > other tests I should look into.
> > > > Just like in this test when you're using vars.
> > > Sorry I was not clear before. What do you mean by "predetermined indices" 
> > > here? If you are referring to, for example, `%0` in `store i8* %0, i8** 
> > > [[HANDLEADDR:%[^,]+]]`, I guess there is no way to get rid of `%0` 
> > > because it means the first argument of the function?
> > Yes, I meant those `%0` like registers. Better to mark them as variables in 
> > function declaration and use those names in the checks.
> Now it's like `define {{.*}}void @.omp_mapper.{{.*}}C.id{{.*}}(i8*, i8*, i8*, 
> i64, i64)`, I think you are suggesting something like `define {{.*}}void 
> @.omp_mapper.{{.*}}C.id{{.*}}(i8* [[HANDLE:%[^,]+]], i8* [[BPTR:%[^,]+]], 
> ...)`, and later I can use `store i8* [[HANDLE]], i8** [[HANDLEADDR:%[^,]+]]`
> 
> I'm not sure how to add names for function arguments. They seems to be always 
> nameless like `(i8*, i8*, i8*, i64, i64)`. Is there a way to do that?
If the clang parameters have names, the llvm params also will get the names. 
But it is not worth it to add the names to the function. Could just use regexp 
here to avoid using LLVM register names? Just `%{{·+}}`. And rely on the order, 
i.e. remove `-DAG` checks?


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

https://reviews.llvm.org/D59474



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

[PATCH] D63048: Update __VERSION__ to remove the hardcoded 4.2.1 version

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D63048#1605050 , @xbolva00 wrote:

> Should we bump __GNUC__, __GNUC_MINOR__ too?


We discussed this two weeks ago:
http://lists.llvm.org/pipermail/cfe-dev/2019-July/062890.html
It's unlikely that we will make any policy change before 9.0.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63048



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

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



Comment at: test/OpenMP/declare_mapper_codegen.cpp:44-48
+// CK0-DAG: store i8* %0, i8** [[HANDLEADDR:%[^,]+]]
+// CK0-DAG: store i8* %1, i8** [[BPTRADDR:%[^,]+]]
+// CK0-DAG: store i8* %2, i8** [[VPTRADDR:%[^,]+]]
+// CK0-DAG: store i64 %3, i{{64|32}}* [[SIZEADDR:%[^,]+]]
+// CK0-DAG: store i64 %4, i64* [[TYPEADDR:%[^,]+]]

ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > lildmh wrote:
> > > > ABataev wrote:
> > > > > I would not rely on the predetermined indices here, better to use 
> > > > > some kind of patterns here just like in other places.
> > > > Could you give an example about what you suggest? For instance, some 
> > > > other tests I should look into.
> > > Just like in this test when you're using vars.
> > Sorry I was not clear before. What do you mean by "predetermined indices" 
> > here? If you are referring to, for example, `%0` in `store i8* %0, i8** 
> > [[HANDLEADDR:%[^,]+]]`, I guess there is no way to get rid of `%0` because 
> > it means the first argument of the function?
> Yes, I meant those `%0` like registers. Better to mark them as variables in 
> function declaration and use those names in the checks.
Now it's like `define {{.*}}void @.omp_mapper.{{.*}}C.id{{.*}}(i8*, i8*, i8*, 
i64, i64)`, I think you are suggesting something like `define {{.*}}void 
@.omp_mapper.{{.*}}C.id{{.*}}(i8* [[HANDLE:%[^,]+]], i8* [[BPTR:%[^,]+]], 
...)`, and later I can use `store i8* [[HANDLE]], i8** [[HANDLEADDR:%[^,]+]]`

I'm not sure how to add names for function arguments. They seems to be always 
nameless like `(i8*, i8*, i8*, i64, i64)`. Is there a way to do that?


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

https://reviews.llvm.org/D59474



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

[PATCH] D65192: [Sema] Disable some enabled-by-default -Wparentheses diagnostics

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I'm not sure the history behind why these were added as default-on 
warningsthey don't really seem appropriate as default warnings to me, 
either.

But I think someone else probably ought to approve the change.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65192



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: test/OpenMP/declare_mapper_codegen.cpp:44-48
+// CK0-DAG: store i8* %0, i8** [[HANDLEADDR:%[^,]+]]
+// CK0-DAG: store i8* %1, i8** [[BPTRADDR:%[^,]+]]
+// CK0-DAG: store i8* %2, i8** [[VPTRADDR:%[^,]+]]
+// CK0-DAG: store i64 %3, i{{64|32}}* [[SIZEADDR:%[^,]+]]
+// CK0-DAG: store i64 %4, i64* [[TYPEADDR:%[^,]+]]

lildmh wrote:
> ABataev wrote:
> > lildmh wrote:
> > > ABataev wrote:
> > > > I would not rely on the predetermined indices here, better to use some 
> > > > kind of patterns here just like in other places.
> > > Could you give an example about what you suggest? For instance, some 
> > > other tests I should look into.
> > Just like in this test when you're using vars.
> Sorry I was not clear before. What do you mean by "predetermined indices" 
> here? If you are referring to, for example, `%0` in `store i8* %0, i8** 
> [[HANDLEADDR:%[^,]+]]`, I guess there is no way to get rid of `%0` because it 
> means the first argument of the function?
Yes, I meant those `%0` like registers. Better to mark them as variables in 
function declaration and use those names in the checks.


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

https://reviews.llvm.org/D59474



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

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

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



Comment at: test/OpenMP/declare_mapper_codegen.cpp:44-48
+// CK0-DAG: store i8* %0, i8** [[HANDLEADDR:%[^,]+]]
+// CK0-DAG: store i8* %1, i8** [[BPTRADDR:%[^,]+]]
+// CK0-DAG: store i8* %2, i8** [[VPTRADDR:%[^,]+]]
+// CK0-DAG: store i64 %3, i{{64|32}}* [[SIZEADDR:%[^,]+]]
+// CK0-DAG: store i64 %4, i64* [[TYPEADDR:%[^,]+]]

ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > I would not rely on the predetermined indices here, better to use some 
> > > kind of patterns here just like in other places.
> > Could you give an example about what you suggest? For instance, some other 
> > tests I should look into.
> Just like in this test when you're using vars.
Sorry I was not clear before. What do you mean by "predetermined indices" here? 
If you are referring to, for example, `%0` in `store i8* %0, i8** 
[[HANDLEADDR:%[^,]+]]`, I guess there is no way to get rid of `%0` because it 
means the first argument of the function?


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

https://reviews.llvm.org/D59474



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

[PATCH] D63048: Update __VERSION__ to remove the hardcoded 4.2.1 version

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

https://bugs.llvm.org/show_bug.cgi?id=42817


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63048



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

r367256 - [driver][test] Use /dev/null in as-options.s instead

[Jordan Rupprecht via cfe-commits]

Author: rupprecht
Date: Mon Jul 29 13:09:20 2019
New Revision: 367256

URL: http://llvm.org/viewvc/llvm-project?rev=367256=rev
Log:
[driver][test] Use /dev/null in as-options.s instead

Modified:
cfe/trunk/test/Driver/as-options.s

Modified: cfe/trunk/test/Driver/as-options.s
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/as-options.s?rev=367256=367255=367256=diff
==
--- cfe/trunk/test/Driver/as-options.s (original)
+++ cfe/trunk/test/Driver/as-options.s Mon Jul 29 13:09:20 2019
@@ -74,13 +74,13 @@
 // -Wa flags shouldn't cause warnings without an assembler stage with
 // -fno-integrated-as either.
 // RUN: %clang -Wa,-mno-warn-deprecated -fno-integrated-as -x c++ %s -S 2>&1 \
-// RUN:   -o %t.o \
+// RUN:   -o /dev/null \
 // RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
 
 // But -m flags for the integrated assembler _should_ warn if the integrated
 // assembler is not in use.
-// RUN: %clang -mrelax-all -fintegrated-as -x c++ %s -S -o %t.o 2>&1 \
+// RUN: %clang -mrelax-all -fintegrated-as -x c++ %s -S -o /dev/null 2>&1 \
 // RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
-// RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o %t.o 2>&1 \
+// RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o /dev/null 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
 // WARN: unused


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

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Nandor Licker via Phabricator via cfe-commits]

nand marked 10 inline comments as done.
nand added a comment.

We can add a separate integer type which tracks all the additional information 
required by `__builtin_constant_p` and compile all integers to it in this 
context. A later patch added an APInt fallback to the interpreter if an 
integral cannot be mapped to a type supported by the VM - this mechanism could 
be used to implement the fallback for contexts which cast pointers to integers.




Comment at: clang/lib/AST/ExprVM/Compiler.h:125
+/// Size of the local, in bytes.
+unsigned Size;
+  };

jfb wrote:
> `ByteSize` since it's the size in bytes :)
I've removed the size field since it's not going to be used, but left the 
structure since it will gain other fields in future patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64146



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

r367255 - Give the 'signed/unsigned wchar_t' extension a warning flag, and follow

[Richard Smith via cfe-commits]

Author: rsmith
Date: Mon Jul 29 13:00:46 2019
New Revision: 367255

URL: http://llvm.org/viewvc/llvm-project?rev=367255=rev
Log:
Give the 'signed/unsigned wchar_t' extension a warning flag, and follow
GCC 9 in promoting it to an error by default.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp
cfe/trunk/test/Misc/warning-flags.c
cfe/trunk/test/SemaCXX/wchar_t.cpp

Modified: cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td?rev=367255=367254=367255=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td Mon Jul 29 13:00:46 
2019
@@ -8509,10 +8509,11 @@ def warn_sync_fetch_and_nand_semantics_c
   InGroup>;
 
 // Type
-def ext_invalid_sign_spec : Extension<"'%0' cannot be signed or unsigned">;
+def ext_wchar_t_sign_spec : ExtWarn<"'%0' cannot be signed or unsigned">,
+  InGroup>, DefaultError;
 def warn_receiver_forward_class : Warning<
-"receiver %0 is a forward class and corresponding @interface may not 
exist">,
-InGroup;
+  "receiver %0 is a forward class and corresponding @interface may not exist">,
+  InGroup;
 def note_method_sent_forward_class : Note<"method %0 is used for the forward 
class">;
 def ext_missing_declspec : ExtWarn<
   "declaration specifier missing, defaulting to 'int'">;

Modified: cfe/trunk/lib/Sema/SemaType.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaType.cpp?rev=367255=367254=367255=diff
==
--- cfe/trunk/lib/Sema/SemaType.cpp (original)
+++ cfe/trunk/lib/Sema/SemaType.cpp Mon Jul 29 13:00:46 2019
@@ -1290,14 +1290,14 @@ static QualType ConvertDeclSpecToType(Ty
 if (DS.getTypeSpecSign() == DeclSpec::TSS_unspecified)
   Result = Context.WCharTy;
 else if (DS.getTypeSpecSign() == DeclSpec::TSS_signed) {
-  S.Diag(DS.getTypeSpecSignLoc(), diag::ext_invalid_sign_spec)
+  S.Diag(DS.getTypeSpecSignLoc(), diag::ext_wchar_t_sign_spec)
 << DS.getSpecifierName(DS.getTypeSpecType(),
Context.getPrintingPolicy());
   Result = Context.getSignedWCharType();
 } else {
   assert(DS.getTypeSpecSign() == DeclSpec::TSS_unsigned &&
 "Unknown TSS value");
-  S.Diag(DS.getTypeSpecSignLoc(), diag::ext_invalid_sign_spec)
+  S.Diag(DS.getTypeSpecSignLoc(), diag::ext_wchar_t_sign_spec)
 << DS.getSpecifierName(DS.getTypeSpecType(),
Context.getPrintingPolicy());
   Result = Context.getUnsignedWCharType();

Modified: cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp?rev=367255=367254=367255=diff
==
--- cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp (original)
+++ cfe/trunk/test/ASTMerge/exprs-cpp/test.cpp Mon Jul 29 13:00:46 2019
@@ -1,5 +1,5 @@
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++1z -fcxx-exceptions 
-emit-pch -o %t.1.ast %S/Inputs/exprs3.cpp
-// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++1z -fcxx-exceptions 
-ast-merge %t.1.ast -fsyntax-only -verify %s
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++1z -fcxx-exceptions 
-Wno-signed-unsigned-wchar -emit-pch -o %t.1.ast %S/Inputs/exprs3.cpp
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++1z -fcxx-exceptions 
-Wno-signed-unsigned-wchar -ast-merge %t.1.ast -fsyntax-only -verify %s
 // expected-no-diagnostics
 
 static_assert(Ch1 == 'a');

Modified: cfe/trunk/test/Misc/warning-flags.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/warning-flags.c?rev=367255=367254=367255=diff
==
--- cfe/trunk/test/Misc/warning-flags.c (original)
+++ cfe/trunk/test/Misc/warning-flags.c Mon Jul 29 13:00:46 2019
@@ -96,4 +96,4 @@ CHECK-NEXT:   warn_weak_import
 
 The list of warnings in -Wpedantic should NEVER grow.
 
-CHECK: Number in -Wpedantic (not covered by other -W flags): 28
+CHECK: Number in -Wpedantic (not covered by other -W flags): 27

Modified: cfe/trunk/test/SemaCXX/wchar_t.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/wchar_t.cpp?rev=367255=367254=367255=diff
==
--- cfe/trunk/test/SemaCXX/wchar_t.cpp (original)
+++ cfe/trunk/test/SemaCXX/wchar_t.cpp Mon Jul 29 13:00:46 2019
@@ -1,10 +1,12 @@
-// RUN: %clang_cc1 -fsyntax-only -pedantic -verify %s 
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-signed-unsigned-wchar 
-verify=allow-signed %s
+// 

r367254 - When determining whether a lambda-expression is implicitly constexpr,

[Richard Smith via cfe-commits]

Author: rsmith
Date: Mon Jul 29 12:59:45 2019
New Revision: 367254

URL: http://llvm.org/viewvc/llvm-project?rev=367254=rev
Log:
When determining whether a lambda-expression is implicitly constexpr,
check the formal rules rather than seeing if the normal checks produce a
diagnostic.

This fixes the handling of C++2a extensions in lambdas in C++17 mode,
as well as some corner cases in earlier language modes where we issue
diagnostics for things other than not satisfying the formal constexpr
requirements.

Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaDecl.cpp
cfe/trunk/lib/Sema/SemaDeclCXX.cpp
cfe/trunk/lib/Sema/SemaLambda.cpp
cfe/trunk/test/CXX/dcl.dcl/dcl.spec/dcl.constexpr/p3.cpp
cfe/trunk/test/SemaCXX/cxx1z-constexpr-lambdas.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=367254=367253=367254=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Jul 29 12:59:45 2019
@@ -2076,8 +2076,16 @@ public:
  bool );
   bool AddOverriddenMethods(CXXRecordDecl *DC, CXXMethodDecl *MD);
 
-  bool CheckConstexprFunctionDecl(const FunctionDecl *FD);
-  bool CheckConstexprFunctionBody(const FunctionDecl *FD, Stmt *Body);
+  enum class CheckConstexprKind {
+/// Diagnose issues that are non-constant or that are extensions.
+Diagnose,
+/// Identify whether this function satisfies the formal rules for constexpr
+/// functions in the current lanugage mode (with no extensions).
+CheckValid
+  };
+
+  bool CheckConstexprFunctionDefinition(const FunctionDecl *FD,
+CheckConstexprKind Kind);
 
   void DiagnoseHiddenVirtualMethods(CXXMethodDecl *MD);
   void FindHiddenVirtualMethods(CXXMethodDecl *MD,

Modified: cfe/trunk/lib/Sema/SemaDecl.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDecl.cpp?rev=367254=367253=367254=diff
==
--- cfe/trunk/lib/Sema/SemaDecl.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDecl.cpp Mon Jul 29 12:59:45 2019
@@ -13532,8 +13532,7 @@ Decl *Sema::ActOnFinishFunctionBody(Decl
 }
 
 if (!IsInstantiation && FD && FD->isConstexpr() && !FD->isInvalidDecl() &&
-(!CheckConstexprFunctionDecl(FD) ||
- !CheckConstexprFunctionBody(FD, Body)))
+!CheckConstexprFunctionDefinition(FD, CheckConstexprKind::Diagnose))
   FD->setInvalidDecl();
 
 if (FD && FD->hasAttr()) {

Modified: cfe/trunk/lib/Sema/SemaDeclCXX.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclCXX.cpp?rev=367254=367253=367254=diff
==
--- cfe/trunk/lib/Sema/SemaDeclCXX.cpp (original)
+++ cfe/trunk/lib/Sema/SemaDeclCXX.cpp Mon Jul 29 12:59:45 2019
@@ -1569,11 +1569,34 @@ void Sema::CheckCXXDefaultArguments(Func
   }
 }
 
+/// Check that the given type is a literal type. Issue a diagnostic if not,
+/// if Kind is Diagnose.
+/// \return \c true if a problem has been found (and optionally diagnosed).
+template 
+static bool CheckLiteralType(Sema , Sema::CheckConstexprKind Kind,
+ SourceLocation Loc, QualType T, unsigned DiagID,
+ Ts &&...DiagArgs) {
+  if (T->isDependentType())
+return false;
+
+  switch (Kind) {
+  case Sema::CheckConstexprKind::Diagnose:
+return SemaRef.RequireLiteralType(Loc, T, DiagID,
+  std::forward(DiagArgs)...);
+
+  case Sema::CheckConstexprKind::CheckValid:
+return !T->isLiteralType(SemaRef.Context);
+  }
+
+  llvm_unreachable("unknown CheckConstexprKind");
+}
+
 // CheckConstexprParameterTypes - Check whether a function's parameter types
 // are all literal types. If so, return true. If not, produce a suitable
 // diagnostic and return false.
 static bool CheckConstexprParameterTypes(Sema ,
- const FunctionDecl *FD) {
+ const FunctionDecl *FD,
+ Sema::CheckConstexprKind Kind) {
   unsigned ArgIndex = 0;
   const FunctionProtoType *FT = FD->getType()->getAs();
   for (FunctionProtoType::param_type_iterator i = FT->param_type_begin(),
@@ -1581,11 +1604,10 @@ static bool CheckConstexprParameterTypes
i != e; ++i, ++ArgIndex) {
 const ParmVarDecl *PD = FD->getParamDecl(ArgIndex);
 SourceLocation ParamLoc = PD->getLocation();
-if (!(*i)->isDependentType() &&
-SemaRef.RequireLiteralType(
-ParamLoc, *i, diag::err_constexpr_non_literal_param, ArgIndex + 1,
-PD->getSourceRange(), isa(FD),
-FD->isConsteval()))
+if (CheckLiteralType(SemaRef, Kind, ParamLoc, *i,
+

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: test/OpenMP/declare_mapper_codegen.cpp:44-48
+// CK0-DAG: store i8* %0, i8** [[HANDLEADDR:%[^,]+]]
+// CK0-DAG: store i8* %1, i8** [[BPTRADDR:%[^,]+]]
+// CK0-DAG: store i8* %2, i8** [[VPTRADDR:%[^,]+]]
+// CK0-DAG: store i64 %3, i{{64|32}}* [[SIZEADDR:%[^,]+]]
+// CK0-DAG: store i64 %4, i64* [[TYPEADDR:%[^,]+]]

lildmh wrote:
> ABataev wrote:
> > I would not rely on the predetermined indices here, better to use some kind 
> > of patterns here just like in other places.
> Could you give an example about what you suggest? For instance, some other 
> tests I should look into.
Just like in this test when you're using vars.


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

https://reviews.llvm.org/D59474



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

r367253 - [driver][test] Update as-options.s to not write to a readonly tree

[Jordan Rupprecht via cfe-commits]

Author: rupprecht
Date: Mon Jul 29 12:57:31 2019
New Revision: 367253

URL: http://llvm.org/viewvc/llvm-project?rev=367253=rev
Log:
[driver][test] Update as-options.s to not write to a readonly tree

The as-options.s test writes to the build tree as of r367165. Some build 
systems configure this to be readonly, so this fails. Explicitly write to the 
output tree using `%t` to avoid this.

Modified:
cfe/trunk/test/Driver/as-options.s

Modified: cfe/trunk/test/Driver/as-options.s
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/as-options.s?rev=367253=367252=367253=diff
==
--- cfe/trunk/test/Driver/as-options.s (original)
+++ cfe/trunk/test/Driver/as-options.s Mon Jul 29 12:57:31 2019
@@ -74,12 +74,13 @@
 // -Wa flags shouldn't cause warnings without an assembler stage with
 // -fno-integrated-as either.
 // RUN: %clang -Wa,-mno-warn-deprecated -fno-integrated-as -x c++ %s -S 2>&1 \
+// RUN:   -o %t.o \
 // RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
 
 // But -m flags for the integrated assembler _should_ warn if the integrated
 // assembler is not in use.
-// RUN: %clang -mrelax-all -fintegrated-as -x c++ %s -S 2>&1 \
+// RUN: %clang -mrelax-all -fintegrated-as -x c++ %s -S -o %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=NOWARN --allow-empty %s
-// RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S 2>&1 \
+// RUN: %clang -mrelax-all -fno-integrated-as -x c++ %s -S -o %t.o 2>&1 \
 // RUN:   | FileCheck --check-prefix=WARN --allow-empty %s
 // WARN: unused


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

[PATCH] D63048: Update __VERSION__ to remove the hardcoded 4.2.1 version

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

Some ICC behaviour..

icc -dumpversion
19.0.3.199

printf("%d %d %s", __GNUC__, __GNUC_MINOR__, __VERSION__);
./a.out 
7 4 Intel(R) C++ gcc 7.4 mode

Should we bump __GNUC__, __GNUC_MINOR__ too?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63048



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

[PATCH] D49754: Add -m(no-)spe, and e500 CPU definitions and support to clang

[Justin Hibbits via Phabricator via cfe-commits]

jhibbits requested review of this revision.
jhibbits added a comment.

Should've marked it as need review earlier.


Repository:
  rC Clang

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

https://reviews.llvm.org/D49754



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

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[Eli Friedman via Phabricator via cfe-commits]

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

LGTM




Comment at: lib/CodeGen/CGBuiltin.cpp:8182
   switch (BuiltinID) {
   default: return nullptr;
   case NEON::BI__builtin_neon_vbsl_v:

dmajor wrote:
> efriedma wrote:
> > I'm a little concerned about the overall code structure here; even if 
> > moving the code for the MSVC builtins is enough to fix those builtins 
> > specifically, is it actually impossible to hit this "default"?  If it is, 
> > can we convert it to an "unreachable"?
> I'm not sure if this question was directed to me... this was a drive-by patch 
> from my end so I'm not familiar with what other types of builtins there might 
> be.
> 
> I should probably mention that I'm hoping to get a fix merged to 9.0 in order 
> to unblock Firefox. Unless someone can tell me that the unreachable is 
> definitely safe, I'd worry about adding instability into the release branch. 
> Perhaps the unreachable could be done in a separate commit only on 10.0 trunk 
> where the tolerance for surprises is generally better.
> 
It should be impossible to reach this normally, I think; any target-specific 
builtin which codegen supports should be handled earlier, and target-specific 
builtins only exist on the relevant target.  The issue would just be a weird 
crash if you add a new builtin Builtins.def without code generation support, 
instead of a nicer "unsupported" error.

But I'm okay taking this as-is without trying to refactor the code to handle 
that gracefully, if we need this for 9.0.


Repository:
  rC Clang

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

https://reviews.llvm.org/D65403



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

Re: r367193 - Buildbot fix for r367190

[Gabor Borsik via cfe-commits]

Looks good to me. Thanks for the fix.

Aaron Ballman  ezt írta (időpont: 2019. júl. 29.,
H, 21:06):

> On Mon, Jul 29, 2019 at 3:03 PM Galina Kistanova via cfe-commits
>  wrote:
> >
> > Hello Gabor ,
> >
> > It looks like this commit broke tests on couple builders:
> >
> >
> http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/18867/steps/test-check-all/logs/stdio
> >
> http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
> > . . .
> > 
> > Failing Tests (1):
> > Clang :: Analysis/taint-generic.c
> >
> >
> > Please have a look?
> > These builders were already red and did not send any notifications.
>
> I think Reid fixed this in r367249
> (http://llvm.org/viewvc/llvm-project?view=revision=367249).
>
> ~Aaron
>
> >
> > Thanks
> >
> > Galina
> >
> > On Sun, Jul 28, 2019 at 8:00 AM Gabor Borsik via cfe-commits <
> cfe-commits@lists.llvm.org> wrote:
> >>
> >> Author: boga95
> >> Date: Sun Jul 28 07:57:41 2019
> >> New Revision: 367193
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=367193=rev
> >> Log:
> >> Buildbot fix for r367190
> >>
> >> Modified:
> >> cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
> >>
> >> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
> >> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp?rev=367193=367192=367193=diff
> >>
> ==
> >> --- cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
> (original)
> >> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp Sun
> Jul 28 07:57:41 2019
> >> @@ -811,7 +811,7 @@ void ento::registerGenericTaintChecker(C
> >>llvm::Optional Config =
> >>getConfiguration(Mgr, Checker, Option, ConfigFile);
> >>if (Config)
> >> -Checker->parseConfiguration(Mgr, Option,
> std::move(Config).getValue());
> >> +Checker->parseConfiguration(Mgr, Option,
> std::move(Config.getValue()));
> >>  }
> >>
> >>  bool ento::shouldRegisterGenericTaintChecker(const LangOptions ) {
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >
> > ___
> > cfe-commits mailing list
> > cfe-commits@lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[David Major via Phabricator via cfe-commits]

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



Comment at: lib/CodeGen/CGBuiltin.cpp:8182
   switch (BuiltinID) {
   default: return nullptr;
   case NEON::BI__builtin_neon_vbsl_v:

efriedma wrote:
> I'm a little concerned about the overall code structure here; even if moving 
> the code for the MSVC builtins is enough to fix those builtins specifically, 
> is it actually impossible to hit this "default"?  If it is, can we convert it 
> to an "unreachable"?
I'm not sure if this question was directed to me... this was a drive-by patch 
from my end so I'm not familiar with what other types of builtins there might 
be.

I should probably mention that I'm hoping to get a fix merged to 9.0 in order 
to unblock Firefox. Unless someone can tell me that the unreachable is 
definitely safe, I'd worry about adding instability into the release branch. 
Perhaps the unreachable could be done in a separate commit only on 10.0 trunk 
where the tolerance for surprises is generally better.



Repository:
  rC Clang

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

https://reviews.llvm.org/D65403



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

[PATCH] D63174: [clang][NewPM] Add RUNS for tests that produce slightly different IR under new PM

[Leonard Chan via Phabricator via cfe-commits]

leonardchan abandoned this revision.
leonardchan added a comment.

Replaced with D65110 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63174



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

Re: [clang-tools-extra] r367137 - [clang-format] Fix style of css file paths

[Galina Kistanova via cfe-commits]

Hello Diego,

This commit added broken test to one of our win builders:

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/27202

. . .
Failing Tests (4):
Extra Tools Unit Tests ::
clang-doc/./ClangDocTests.exe/HTMLGeneratorTest.emitRecordHTML
. . .

Please have a look?
These builder was already red and did not send any notifications.

Thanks

Galina

On Fri, Jul 26, 2019 at 11:02 AM Diego Astiazaran via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: diegoastiazaran
> Date: Fri Jul 26 11:02:42 2019
> New Revision: 367137
>
> URL: http://llvm.org/viewvc/llvm-project?rev=367137=rev
> Log:
> [clang-format] Fix style of css file paths
>
> CSS files included in HTML should have a path in posix style, it should
> not be different for Windows.
>
> Differential Revision: https://reviews.llvm.org/D65309
>
> Modified:
> clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
> clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
>
> Modified: clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp?rev=367137=367136=367137=diff
>
> ==
> --- clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp (original)
> +++ clang-tools-extra/trunk/clang-doc/HTMLGenerator.cpp Fri Jul 26
> 11:02:42 2019
> @@ -231,6 +231,8 @@ genStylesheetsHTML(StringRef InfoPath, c
>  SmallString<128> StylesheetPath = computeRelativePath("", InfoPath);
>  llvm::sys::path::append(StylesheetPath,
>  llvm::sys::path::filename(FilePath));
> +// Paths in HTML must be in posix-style
> +llvm::sys::path::native(StylesheetPath,
> llvm::sys::path::Style::posix);
>  LinkNode->Attributes.try_emplace("href", StylesheetPath);
>  Out.emplace_back(std::move(LinkNode));
>}
>
> Modified: clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp?rev=367137=367136=367137=diff
>
> ==
> --- clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp
> (original)
> +++ clang-tools-extra/trunk/unittests/clang-doc/HTMLGeneratorTest.cpp Fri
> Jul 26 11:02:42 2019
> @@ -110,34 +110,23 @@ TEST(HTMLGeneratorTest, emitRecordHTML)
>ClangDocContext CDCtx = getClangDocContext();
>auto Err = G->generateDocForInfo(, Actual, CDCtx);
>assert(!Err);
> -  SmallString<16> PathToF;
> -  llvm::sys::path::native("../../../path/to/F.html", PathToF);
> -  SmallString<16> PathToInt;
> -  llvm::sys::path::native("../int.html", PathToInt);
> -  SmallString<16> PathToSylesheet;
> -  llvm::sys::path::native("../../../clang-doc-default-stylesheet.css",
> -  PathToSylesheet);
>std::string Expected = R"raw(
>  
>  class r
> - - std::string(PathToSylesheet.str()) +
> - R"raw("/>
> +
>  
>class r
>Defined at line 10 of test.cpp
>
>  Inherits from
> - - R"raw(">F
> +F
>  , G
>
>Members
>
>  
>private
> -   - R"raw(">int
> +  int
> X
>  
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59474: [OpenMP 5.0] Codegen support for user-defined mappers

[Lingda Li via Phabricator via cfe-commits]

lildmh marked an inline comment as done.
lildmh added a comment.

Thanks Alexey! Could you look into the runtime patch D60972 
 then?




Comment at: test/OpenMP/declare_mapper_codegen.cpp:44-48
+// CK0-DAG: store i8* %0, i8** [[HANDLEADDR:%[^,]+]]
+// CK0-DAG: store i8* %1, i8** [[BPTRADDR:%[^,]+]]
+// CK0-DAG: store i8* %2, i8** [[VPTRADDR:%[^,]+]]
+// CK0-DAG: store i64 %3, i{{64|32}}* [[SIZEADDR:%[^,]+]]
+// CK0-DAG: store i64 %4, i64* [[TYPEADDR:%[^,]+]]

ABataev wrote:
> I would not rely on the predetermined indices here, better to use some kind 
> of patterns here just like in other places.
Could you give an example about what you suggest? For instance, some other 
tests I should look into.


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

https://reviews.llvm.org/D59474



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

[PATCH] D65403: [COFF, ARM64] Reorder handling of aarch64 MSVC builtins

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.



Comment at: lib/CodeGen/CGBuiltin.cpp:8182
   switch (BuiltinID) {
   default: return nullptr;
   case NEON::BI__builtin_neon_vbsl_v:

I'm a little concerned about the overall code structure here; even if moving 
the code for the MSVC builtins is enough to fix those builtins specifically, is 
it actually impossible to hit this "default"?  If it is, can we convert it to 
an "unreachable"?


Repository:
  rC Clang

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

https://reviews.llvm.org/D65403



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

[PATCH] D64089: [Driver] Introduce -stdlib++-isystem

[Louis Dionne via Phabricator via cfe-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: dexonsmith.

This looks fine to me, but I'd like to have someone else familiar with the 
Driver/Frontend design sign off too. The implementation is trivial but I'd like 
support on the fact that this is a good idea (which I think it is).




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1307
+  HasStdlibxxIsystem ? TC.AddClangCXXStdlibIsystemArgs(Args, CmdArgs)
+ : TC.AddClangCXXStdlibIncludeArgs(Args, CmdArgs);
+});

smeenai wrote:
> ldionne wrote:
> > So, before, we would populate `-stdlib=XXX` here unconditionally for `cc1`.
> > 
> > After the patch, we do _not_ populate `-stdlib=XXX` and instead we pass 
> > `-internal-isystem` to `cc1` with the contents of `-stdlib++-isystem` 
> > whenever that option is provided, otherwise we fall back to the previous 
> > behaviour.
> > 
> > Did I get that right? If so, could we investigate getting rid of 
> > `AddClangCXXStdlibIncludeArgs` altogether? I don't think it is needed 
> > anymore since my refactor of the driver for Darwin.
> Sorry, I'd missed this. You're right; we'll continue passing `-stdlib=XXX` in 
> the old case (i.e. when there's no `-stdlib++-isystem` argument), but not if 
> it's there.
> 
> When you say getting rid of `AddClangCXXStdlibIncludeArgs`, that's the one in 
> the driver (in the toolchains) right? I thought your Darwin refactor involved 
> moving C++ header search logic from the frontend into the driver ... did you 
> mean getting rid of the C++ header search path logic in the frontend?
Sorry, I should have been more clear. I meant specifically getting rid of this: 
https://github.com/llvm-project/clang/blob/master/lib/Driver/ToolChain.cpp#L832

In that snippet, we pass `-stdlib=XXX` down to the frontend. My understanding 
is that this was only necessary because the frontend on Darwin needed to know 
which stdlib was in use, in order to setup the header search paths correctly. 
Now that it's not needed anymore, I think we could get rid of that hack 
altogether.

However, looking at it further, changing that is way beyond the scope of your 
PR. I had not realized that other non-Darwin toolchain provided implementations 
of `AddClangCXXStdlibIncludeArgs` (which is `virtual` and meant to be 
overridden).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64089



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

Re: r367193 - Buildbot fix for r367190

[Galina Kistanova via cfe-commits]

Hello Gabor ,

It looks like this commit broke tests on couple builders:

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/18867/steps/test-check-all/logs/stdio
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast
. . .

Failing Tests (1):
Clang :: Analysis/taint-generic.c


Please have a look?
These builders were already red and did not send any notifications.

Thanks

Galina

On Sun, Jul 28, 2019 at 8:00 AM Gabor Borsik via cfe-commits <
cfe-commits@lists.llvm.org> wrote:

> Author: boga95
> Date: Sun Jul 28 07:57:41 2019
> New Revision: 367193
>
> URL: http://llvm.org/viewvc/llvm-project?rev=367193=rev
> Log:
> Buildbot fix for r367190
>
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
> URL:
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp?rev=367193=367192=367193=diff
>
> ==
> --- cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp
> (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp Sun Jul
> 28 07:57:41 2019
> @@ -811,7 +811,7 @@ void ento::registerGenericTaintChecker(C
>llvm::Optional Config =
>getConfiguration(Mgr, Checker, Option, ConfigFile);
>if (Config)
> -Checker->parseConfiguration(Mgr, Option,
> std::move(Config).getValue());
> +Checker->parseConfiguration(Mgr, Option,
> std::move(Config.getValue()));
>  }
>
>  bool ento::shouldRegisterGenericTaintChecker(const LangOptions ) {
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D64475: [clangd] Duplicate lines of semantic highlightings sent removed.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

Mostly final NITs from me, but there is an important one about removing the 
`erase` call on `didOpen`.




Comment at: clang-tools-extra/clangd/ClangdLSPServer.cpp:467
+std::lock_guard Lock(HighlightingsMutex);
+FileToHighlightings.erase(File);
+  }

Now that the patch landed, this is obsolete. Could you remove?



Comment at: clang-tools-extra/clangd/ClangdServer.cpp:80
+FileID MainFileID = SM.getMainFileID();
+unsigned int FileSize = SM.getFileEntryForID(MainFileID)->getSize();
+int NLines = AST.getSourceManager().getLineNumber(MainFileID, FileSize);

NIT: use `unsigned` instead of `unsigned int`



Comment at: clang-tools-extra/clangd/ClangdServer.h:56
+  virtual void onHighlightingsReady(
+  PathRef File, std::vector Highlightings, int NLines) 
{}
 };

NIT: could you document `NLines`



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:294
+  // ArrayRefs to the current line in the highlights.
+  ArrayRef NewLine(New.begin(),
+  /*length*/0UL);

Could we try to find a better name for this? Having `Line` and `NextLine()` 
which represent line numbers and `NewLine` which represents highlightings, 
creates some confusion.



Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:310
+  // highlightings beyond the end of the file. That might crash the client.
+  for (int Line = 0; Line != std::numeric_limits::max() && Line < NLines;
+   Line = NextLine()) {

`Line != intmax` is redundant (NLines is <= intmax by definition).
Maybe remove it altogether to simplify the loop condition?



Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:80
+diffHighlightings(ArrayRef New,
+  ArrayRef Old, int NLines);
 

Could you document what `NLines` is? Specifically, whether it's the number of 
lines for `New` or `Old`.




Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:60
+
+void checkDiffedHighlights(llvm::StringRef OldCode, llvm::StringRef NewCode) {
+  Annotations OldTest(OldCode);

NIT: add documentation on how this should be used
Most importantly, the fact that we need to put `^` on all changed lines should 
be mentioned.



Comment at: clang-tools-extra/clangd/unittests/SemanticHighlightingTests.cpp:66
+
+  std::map> ExpectedLines;
+  for (const Position  : NewTest.points()) {

NIT: use `llvm::DenseMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64475



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

r367249 - Fix taint-generic.c on Windows, handle case in OS error

[Reid Kleckner via cfe-commits]

Author: rnk
Date: Mon Jul 29 11:48:50 2019
New Revision: 367249

URL: http://llvm.org/viewvc/llvm-project?rev=367249=rev
Log:
Fix taint-generic.c on Windows, handle case in OS error

Modified:
cfe/trunk/test/Analysis/taint-generic.c

Modified: cfe/trunk/test/Analysis/taint-generic.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/taint-generic.c?rev=367249=367248=367249=diff
==
--- cfe/trunk/test/Analysis/taint-generic.c (original)
+++ cfe/trunk/test/Analysis/taint-generic.c Mon Jul 29 11:48:50 2019
@@ -32,7 +32,7 @@
 
 // CHECK-ILL-FORMED: (frontend): invalid input for checker option
 // CHECK-ILL-FORMED-SAME:
'alpha.security.taint.TaintPropagation:Config',
-// CHECK-ILL-FORMED-SAME:that expects a valid yaml file: Invalid 
argument
+// CHECK-ILL-FORMED-SAME:that expects a valid yaml file: 
{{[Ii]}}nvalid argument
 
 // RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=alpha.security.taint \


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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

Thanks for the review!
Since I did some refactoring I will wait for an additional accept before 
committing.


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

https://reviews.llvm.org/D64256



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

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 212207.
xazax.hun edited the summary of this revision.
xazax.hun added a comment.

- Rebase, add the results of testing on real world projects to the description.


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

https://reviews.llvm.org/D65127

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -121,24 +121,47 @@
 
 namespace std {
 template 
-struct basic_iterator {};
+struct basic_iterator {
+  basic_iterator operator++();
+  T& operator*();
+};
+
+template
+bool operator!=(basic_iterator, basic_iterator);
 
 template 
 struct vector {
   typedef basic_iterator iterator;
   iterator begin();
+  iterator end();
   T *data();
+  T (int n);
+};
+
+template
+struct basic_string_view {
+  basic_string_view(const T *);
+  const T *begin() const;
 };
 
 template
 struct basic_string {
   const T *c_str() const;
+  operator basic_string_view () const;
 };
 
+
 template
 struct unique_ptr {
   T *get() const;
 };
+
+template
+struct optional {
+  optional();
+  optional(const T&);
+  T *();
+};
 }
 
 void modelIterators() {
@@ -168,3 +191,36 @@
 int *danglingUniquePtrFromTemp2() {
   return std::unique_ptr().get(); // expected-warning {{returning address of local temporary object}}
 }
+
+void danglingReferenceFromTempOwner() {
+  int  = *std::optional(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = *std::optional(5); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  int  = std::vector().at(3); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+std::vector getTempVec();
+std::optional> getTempOptVec();
+
+void testLoops() {
+  for (auto i : getTempVec()) // ok
+;
+  for (auto i : *getTempOptVec()) // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+;
+}
+
+int (std::vector ) {
+  std::vector::iterator it = v.begin();
+  int& value = *it;
+  return value; // ok
+}
+
+int () {
+  std::unique_ptr localOwner;
+  int  = *localOwner.get();
+  // In real world code localOwner is usually moved here.
+  return p; // ok
+}
+
+const char *trackThroughMultiplePointer() {
+  return std::basic_string_view(std::basic_string()).begin(); // expected-warning {{returning address of local temporary object}}
+}
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6563,16 +6563,30 @@
   if (auto *Conv = dyn_cast_or_null(Callee))
 if (isRecordWithAttr(Conv->getConversionType()))
   return true;
-  if (!Callee->getParent()->isInStdNamespace() || !Callee->getIdentifier())
+  if (!Callee->getParent()->isInStdNamespace())
 return false;
-  if (!isRecordWithAttr(Callee->getReturnType()) &&
-  !Callee->getReturnType()->isPointerType())
-return false;
-  return llvm::StringSwitch(Callee->getName())
-  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
-  .Cases("end", "rend", "cend", "crend", true)
-  .Cases("c_str", "data", "get", true)
-  .Default(false);
+  if (Callee->getReturnType()->isPointerType() ||
+  isRecordWithAttr(Callee->getReturnType())) {
+if (!Callee->getIdentifier())
+  return false;
+return llvm::StringSwitch(Callee->getName())
+.Cases("begin", "rbegin", "cbegin", "crbegin", true)
+.Cases("end", "rend", "cend", "crend", true)
+.Cases("c_str", "data", "get", true)
+// Map and set types.
+.Cases("find", "equal_range", "lower_bound", "upper_bound", true)
+.Default(false);
+  } else if (Callee->getReturnType()->isReferenceType()) {
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||
+ OO == OverloadedOperatorKind::OO_Star;
+}
+return llvm::StringSwitch(Callee->getName())
+.Cases("front", "back", "at", true)
+.Default(false);
+  }
+  return false;
 }
 
 static void handleGslAnnotatedTypes(IndirectLocalPath , Expr *Call,
@@ -6592,6 +6606,12 @@
 if (MD && shouldTrackImplicitObjectArg(MD))
   VisitPointerArg(MD, MCE->getImplicitObjectArgument());
 return;
+  } else if (auto *OCE = dyn_cast(Call)) {
+FunctionDecl *Callee = OCE->getDirectCallee();
+if (Callee->isCXXInstanceMember() &&
+shouldTrackImplicitObjectArg(cast(Callee)))
+  VisitPointerArg(Callee, OCE->getArg(0));
+return;
   }
 
   if (auto *CCE = dyn_cast(Call)) {
___
cfe-commits mailing 

[PATCH] D65127: Even more warnings utilizing gsl::Owner/gsl::Pointer annotations

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun marked an inline comment as done.
xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6581
+if (!Callee->getIdentifier()) {
+  auto OO = Callee->getOverloadedOperator();
+  return OO == OverloadedOperatorKind::OO_Subscript ||

If we want to relax the warnings to give more results we could extend the 
checking of these overloaded operators for annotated types. But this would 
imply that the user need to have the expected semantics for those types and can 
only suppress false positives by removing some gsl:::owner/poinnter annotations.


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

https://reviews.llvm.org/D65127



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

[PATCH] D65120: More warnings regarding gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 212206.
xazax.hun marked 3 inline comments as done.
xazax.hun edited the summary of this revision.
xazax.hun added a comment.

- Add new line to end of file.
- Rebase, calls no longer need special handling.


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

https://reviews.llvm.org/D65120

Files:
  clang/lib/Sema/SemaInit.cpp
  clang/test/Analysis/inner-pointer.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -2,7 +2,6 @@
 struct [[gsl::Owner(int)]] MyIntOwner {
   MyIntOwner();
   int *();
-  int *c_str() const;
 };
 
 struct [[gsl::Pointer(int)]] MyIntPointer {
@@ -52,16 +51,6 @@
   return t.releaseAsRawPointer(); // ok
 }
 
-int *danglingRawPtrFromLocal() {
-  MyIntOwner t;
-  return t.c_str(); // TODO
-}
-
-int *danglingRawPtrFromTemp() {
-  MyIntPointer p;
-  return p.toOwner().c_str(); // TODO
-}
-
 struct Y {
   int a[4];
 };
@@ -103,6 +92,12 @@
   return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
 }
 
+MyIntOwner makeTempOwner();
+
+MyIntPointer danglingGslPtrFromTemporary2() {
+  return makeTempOwner(); // expected-warning {{returning address of local temporary object}}
+}
+
 MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
   return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
 }
@@ -124,12 +119,52 @@
   global2 = MyLongOwnerWithConversion{}; // TODO ?
 }
 
-struct IntVector {
-  int *begin();
-  int *end();
+namespace std {
+template 
+struct basic_iterator {};
+
+template 
+struct vector {
+  typedef basic_iterator iterator;
+  iterator begin();
+  T *data();
+};
+
+template
+struct basic_string {
+  const T *c_str() const;
+};
+
+template
+struct unique_ptr {
+  T *get() const;
 };
+}
 
 void modelIterators() {
-  int *it = IntVector{}.begin(); // TODO ?
+  std::vector::iterator it = std::vector().begin(); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
   (void)it;
 }
+
+std::vector::iterator modelIteratorReturn() {
+  return std::vector().begin(); // expected-warning {{returning address of local temporary object}}
+}
+
+const char *danglingRawPtrFromLocal() {
+  std::basic_string s;
+  return s.c_str(); // expected-warning {{address of stack memory associated with local variable 's' returned}}
+}
+
+const char *danglingRawPtrFromTemp() {
+  return std::basic_string().c_str(); // expected-warning {{returning address of local temporary object}}
+}
+
+std::unique_ptr getUniquePtr();
+
+int *danglingUniquePtrFromTemp() {
+  return getUniquePtr().get(); // expected-warning {{returning address of local temporary object}}
+}
+
+int *danglingUniquePtrFromTemp2() {
+  return std::unique_ptr().get(); // expected-warning {{returning address of local temporary object}}
+}
Index: clang/test/Analysis/inner-pointer.cpp
===
--- clang/test/Analysis/inner-pointer.cpp
+++ clang/test/Analysis/inner-pointer.cpp
@@ -1,5 +1,6 @@
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.InnerPointer \
-// RUN:   %s -analyzer-output=text -verify
+// RUN:   %s -analyzer-output=text -Wno-dangling -Wno-dangling-field \
+// RUN:   -Wno-return-stack-address -verify
 
 #include "Inputs/system-header-simulator-cxx.h"
 namespace std {
Index: clang/lib/Sema/SemaInit.cpp
===
--- clang/lib/Sema/SemaInit.cpp
+++ clang/lib/Sema/SemaInit.cpp
@@ -6559,6 +6559,22 @@
   return false;
 }
 
+static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
+  if (auto *Conv = dyn_cast_or_null(Callee))
+if (isRecordWithAttr(Conv->getConversionType()))
+  return true;
+  if (!Callee->getParent()->isInStdNamespace() || !Callee->getIdentifier())
+return false;
+  if (!isRecordWithAttr(Callee->getReturnType()) &&
+  !Callee->getReturnType()->isPointerType())
+return false;
+  return llvm::StringSwitch(Callee->getName())
+  .Cases("begin", "rbegin", "cbegin", "crbegin", true)
+  .Cases("end", "rend", "cend", "crend", true)
+  .Cases("c_str", "data", "get", true)
+  .Default(false);
+}
+
 static void handleGslAnnotatedTypes(IndirectLocalPath , Expr *Call,
 LocalVisitor Visit) {
   auto VisitPointerArg = [&](const Decl *D, Expr *Arg) {
@@ -6572,15 +6588,14 @@
   };
 
   if (auto *MCE = dyn_cast(Call)) {
-const FunctionDecl *Callee = MCE->getDirectCallee();
-if (auto *Conv = dyn_cast_or_null(Callee))
-  if (isRecordWithAttr(Conv->getConversionType()))
-VisitPointerArg(Callee, MCE->getImplicitObjectArgument());
+const auto *MD = cast_or_null(MCE->getDirectCallee());
+

[PATCH] D65120: More warnings regarding gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:6563
 
+static bool shouldTrackImplicitObjectArg(const CXXMethodDecl *Callee) {
+  if (auto *Conv = dyn_cast_or_null(Callee))

gribozavr wrote:
> This looks like an ad-hoc rule. Is there a way to express it with attributes 
> instead?
It is indeed an ad-hoc rule. The point is that we do know that `std` methods 
behave this way and function attributes will be able to express the same in the 
future. But we are still experimenting with what is the best spelling, 
representation etc for those function attributes. So given the value of these 
checks I do not want to wait until function annotations are upstreamed since we 
can achieve the same with relatively little code. I run these checks on ~300 
open source projects and not a single false positive was found in the latest 
run. 



Comment at: clang/lib/Sema/SemaInit.cpp:6572
+return false;
+  return llvm::StringSwitch(Callee->getName())
+  .Cases("begin", "rbegin", "cbegin", "crbegin", true)

gribozavr wrote:
> Should we also check that `Callee->getParent` is an owner?
> 
> Otherwise the check would complain about `begin()` on, for example, a 
> `std::string_view`.
This is intentional. We only warn if the initialization chain can be tracked 
back to a temporary (or local in some cases) owner. 
So we warn for the following code:
```
const char *trackThroughMultiplePointer() {
  return std::basic_string_view(std::basic_string()).begin(); // 
expected-warning {{returning address of local temporary object}}
}
```


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

https://reviews.llvm.org/D65120



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

[PATCH] D63082: [Diagnostics] Added support for -Wint-in-bool-context

[Dávid Bolvanský via Phabricator via cfe-commits]

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



Comment at: test/SemaCXX/warn-int-in-bool-context.cpp:96
+
+  if (f == apple || orange) // expected-warning {{enum constant in boolean 
context}}
+return a;

@aaron.ballman : In C (but not in C++ ugh?) mode we have: 'use of logical '||' 
with constant operand' here..

/home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:18:
 warning: use of logical '||' with constant operand
  if (f == apple || orange) // expected-warning {{enum constant in boolean 
context}}
 ^  ~~
/home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:18:
 note: use '|' for a bitwise operation
  if (f == apple || orange) // expected-warning {{enum constant in boolean 
context}}
 ^~
 |
/home/xbolva00/LLVM/llvm/tools/clang/test/SemaCXX/warn-int-in-bool-context.cpp:106:21:
 warning: enum constant in boolean context
  if (f == apple || orange) // expected-warning {{enum constant in boolean 
context}}






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

https://reviews.llvm.org/D63082



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added inline comments.



Comment at: clang/lib/Sema/SemaInit.cpp:7095
   auto *MTE = dyn_cast(L);
+  if (IsGslPtrInitWithGslTempOwner) {
+Diag(DiagLoc, diag::warn_dangling_lifetime_pointer) << DiagRange;

gribozavr wrote:
> It is unclear to me why `if (IsGslPtrInitWithGslTempOwner)` is before `if 
> (!MTE)`, seems like that exclusion should apply to our case, too.
The reason was that I always skipped MTEs and was looking for the expr that 
actually initialized MTEs. Relying on MTEs, however, turned out to simplify the 
code a bit. Updating the diff, thanks!


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

https://reviews.llvm.org/D64256



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

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 212205.
xazax.hun marked an inline comment as done.
xazax.hun added a comment.

- Actually move the code snippet in question.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+int *noFalsePositive(MyIntOwner ) {
+  MyIntPointer p = o;
+  return &*p; // ok
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // expected-warning 

[PATCH] D64256: Teach some warnings to respect gsl::Pointer and gsl::Owner attributes

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun updated this revision to Diff 212204.
xazax.hun marked an inline comment as done.
xazax.hun added a comment.

- A small refactoring based on an observation from a review comment.


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

https://reviews.llvm.org/D64256

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaInit.cpp
  clang/test/Sema/warn-lifetime-analysis-nocfg.cpp

Index: clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
===
--- /dev/null
+++ clang/test/Sema/warn-lifetime-analysis-nocfg.cpp
@@ -0,0 +1,135 @@
+// RUN: %clang_cc1 -fsyntax-only -Wdangling -Wdangling-field -Wreturn-stack-address -verify %s
+struct [[gsl::Owner(int)]] MyIntOwner {
+  MyIntOwner();
+  int *();
+  int *c_str() const;
+};
+
+struct [[gsl::Pointer(int)]] MyIntPointer {
+  MyIntPointer(int *p = nullptr);
+  // Conversion operator and constructor conversion will result in two
+  // different ASTs. The former is tested with another owner and 
+  // pointer type.
+  MyIntPointer(const MyIntOwner &);
+  int *();
+  MyIntOwner toOwner();
+};
+
+struct [[gsl::Pointer(long)]] MyLongPointerFromConversion {
+  MyLongPointerFromConversion(long *p = nullptr);
+  long *();
+};
+
+struct [[gsl::Owner(long)]] MyLongOwnerWithConversion {
+  MyLongOwnerWithConversion();
+  operator MyLongPointerFromConversion();
+  long *();
+  MyIntPointer releaseAsMyPointer();
+  long *releaseAsRawPointer();
+};
+
+void danglingHeapObject() {
+  new MyLongPointerFromConversion(MyLongOwnerWithConversion{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  new MyIntPointer(MyIntOwner{}); // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+}
+
+void intentionalFalseNegative() {
+  int i;
+  MyIntPointer p{};
+  // In this case we do not have enough information in a statement local
+  // analysis to detect the problem.
+  new MyIntPointer(p);
+  new MyIntPointer(MyIntPointer{p});
+}
+
+MyIntPointer ownershipTransferToMyPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsMyPointer(); // ok
+}
+
+long *ownershipTransferToRawPointer() {
+  MyLongOwnerWithConversion t;
+  return t.releaseAsRawPointer(); // ok
+}
+
+int *danglingRawPtrFromLocal() {
+  MyIntOwner t;
+  return t.c_str(); // TODO
+}
+
+int *danglingRawPtrFromTemp() {
+  MyIntPointer p;
+  return p.toOwner().c_str(); // TODO
+}
+
+struct Y {
+  int a[4];
+};
+
+void dangligGslPtrFromTemporary() {
+  MyIntPointer p = Y{}.a; // expected-warning {{temporary whose address is used as value of local variable 'p' will be destroyed at the end of the full-expression}}
+  (void)p;
+}
+
+struct DanglingGslPtrField {
+  MyIntPointer p; // expected-note 2{{pointer member declared here}}
+  MyLongPointerFromConversion p2; // expected-note {{pointer member declared here}}
+  DanglingGslPtrField(int i) : p() {} // expected-warning {{initializing pointer member 'p' with the stack address of parameter 'i'}}
+  DanglingGslPtrField() : p2(MyLongOwnerWithConversion{}) {} // expected-warning {{initializing pointer member 'p2' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+  DanglingGslPtrField(double) : p(MyIntOwner{}) {} // expected-warning {{initializing pointer member 'p' to point to a temporary object whose lifetime is shorter than the lifetime of the constructed object}}
+};
+
+MyIntPointer danglingGslPtrFromLocal() {
+  int j;
+  return  // expected-warning {{address of stack memory associated with local variable 'j' returned}}
+}
+
+MyIntPointer returningLocalPointer() {
+  MyIntPointer localPointer;
+  return localPointer; // ok
+}
+
+MyIntPointer daglingGslPtrFromLocalOwner() {
+  MyIntOwner localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyLongPointerFromConversion daglingGslPtrFromLocalOwnerConv() {
+  MyLongOwnerWithConversion localOwner;
+  return localOwner; // expected-warning {{address of stack memory associated with local variable 'localOwner' returned}}
+}
+
+MyIntPointer danglingGslPtrFromTemporary() {
+  return MyIntOwner{}; // expected-warning {{returning address of local temporary object}}
+}
+
+MyLongPointerFromConversion danglingGslPtrFromTemporaryConv() {
+  return MyLongOwnerWithConversion{}; // expected-warning {{returning address of local temporary object}}
+}
+
+int *noFalsePositive(MyIntOwner ) {
+  MyIntPointer p = o;
+  return &*p; // ok
+}
+
+MyIntPointer global;
+MyLongPointerFromConversion global2;
+
+void initLocalGslPtrWithTempOwner() {
+  MyIntPointer p = MyIntOwner{}; // expected-warning {{object backing the pointer will be destroyed at the end of the full-expression}}
+  p = MyIntOwner{}; // TODO ?
+  global = MyIntOwner{}; // TODO ?
+  MyLongPointerFromConversion p2 = MyLongOwnerWithConversion{}; // 

[PATCH] D64146: [ConstExprPreter][WIP] Initial patch for the constexpr interpreter

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

In D64146#1604717 , @nand wrote:

> > How do you intend to represent pointers cast to integer types? Allocating 
> > 64 bits of state for a 64-bit integer is insufficient to model that case.
>
> Is this ever going to be allowed in constexpr?


It's not sufficient for this to handle only the things that are allowed in 
constant expressions; you also need to allow the things that are allowed by 
Clang's current constant evaluator, which includes this case. There are also 
constructs that allow arbitrary constant folding within the context of constant 
expression evaluation (such as a `__builtin_constant_p` conditional). So yes, 
you need to deal with this.

> If that is the case, we'll add a separate type for all integers which are 
> large enough to hold a pointer, a tagged union indicating whether the value 
> is a number or a pointer. This would add significant overhead, but I don't 
> see any other way which can correctly diagnose UB when casting a random 
> integer to a pointer.

These cases are likely to be rare enough that separately-allocated storage for 
this case could work. You'll need at least one bit somewhere to track whether 
you're in the "pointer cast to integer" case, though.

(You also need to be able to distinguish between an integer value and an 
uninitialized integer and an out-of-lifetime integer, so you'll presumably need 
some side-table to track the state of subobjects anyway.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64146



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

[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

> I fear it is necessary: at least it matches documented behaviour of both the 
> Sun/Oracle Studio compilers and gcc.

I will defer to your opinion here. But -- one last attempt at dissuading you. :)

Is this really doing something _important_, or is it just legacy cruft which 
can be safely ignored by now? With your "logb" example, it seems to me that it 
is probably preferable to always use the new correct "xpg6" implementation, and 
just ignore the legacy/incorrect version. Similarly, the example given in 
https://gcc.gnu.org/PR40411 of freopen -- again, seems like it'd be better to 
just use the new xpg6 behavior, always.

> The -std= options usually get passed to the linking step because CFLAGS is 
> added to the options as well

With gnu make they are not (unless it's doing a single-step compiling+linking 
step). Other tools like CMake also don't pass standards versions to linking. 
This makes sense, because a program can contain code compiled with multiple 
standards versions, and multiple languages. Thus, I'd expect most users to just 
get the default xpg6 and Xa objects, even if they do specify -std=c90 for 
compilation.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64793



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

[PATCH] D65410: [PassManager] First Pass implementation at -O1 pass pipeline

[Eric Christopher via Phabricator via cfe-commits]

echristo created this revision.
echristo added reviewers: chandlerc, hfinkel.
Herald added subscribers: cfe-commits, jfb, dexonsmith, steven_wu, hiraditya, 
javed.absar, mcrosier, mehdi_amini.
Herald added projects: clang, LLVM.

As a follow-up to my initial mail to llvm-dev here's a first pass at the O1 
 described there.

Some rough internal testing using a bootstrap and test of clang has shown a 
combined build and test time for clang with nearly equivalent performance to O3 
 and quite a speedup over O0 - it's 
currently a little slower than the existing O1 
, likely due to the clang+llvm 
testsuite use of the same binaries many times rather than a few for individual 
tests. Build time is a bit better. For a larger build and smaller test time 
(think a couple of unittests), this is a bit better than either O3 
, O0, or O1 
. Overall binary size drops 
significantly compared to O0.

This change doesn't include any change to move from selection dag to fast isel 
and that will come with other numbers that should help inform that decision. I 
also haven't done any real debuggability studies with this pipeline yet, I 
wanted to get the initial start done so that people could see it and we could 
start tweaking after.

Test updates: Outside of the newpm tests most of the updates are coming from 
either optimization passes not run anymore (and without a compelling argument 
at the moment) that were largely used for canonicalization in clang.

Original post:

http://lists.llvm.org/pipermail/llvm-dev/2019-April/131494.html


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65410

Files:
  clang/test/CodeGen/2008-07-30-implicit-initialization.c
  clang/test/CodeGen/arm-fp16-arguments.c
  clang/test/CodeGen/arm-vfp16-arguments2.cpp
  clang/test/CodeGenCXX/auto-var-init.cpp
  clang/test/CodeGenCXX/stack-reuse.cpp
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/lib/Passes/PassBuilder.cpp
  llvm/lib/Transforms/IPO/PassManagerBuilder.cpp
  llvm/test/Feature/optnone-opt.ll
  llvm/test/Other/new-pm-defaults.ll
  llvm/test/Other/new-pm-thinlto-defaults.ll
  llvm/test/Transforms/MemCpyOpt/lifetime.ll
  llvm/test/Transforms/PhaseOrdering/simplifycfg-options.ll

Index: llvm/test/Transforms/PhaseOrdering/simplifycfg-options.ll
===
--- llvm/test/Transforms/PhaseOrdering/simplifycfg-options.ll
+++ llvm/test/Transforms/PhaseOrdering/simplifycfg-options.ll
@@ -7,7 +7,7 @@
 
 define i1 @PR33605(i32 %a, i32 %b, i32* %c) {
 ; ALL-LABEL: @PR33605(
-; ALL-NEXT:  for.body:
+; ALL-NEXT:  entry:
 ; ALL-NEXT:[[OR:%.*]] = or i32 [[B:%.*]], [[A:%.*]]
 ; ALL-NEXT:[[ARRAYIDX:%.*]] = getelementptr inbounds i32, i32* [[C:%.*]], i64 1
 ; ALL-NEXT:[[TMP0:%.*]] = load i32, i32* [[ARRAYIDX]], align 4
@@ -18,7 +18,7 @@
 ; ALL-NEXT:tail call void @foo()
 ; ALL-NEXT:br label [[IF_END]]
 ; ALL:   if.end:
-; ALL-NEXT:[[CHANGED_1_OFF0:%.*]] = phi i1 [ true, [[IF_THEN]] ], [ false, [[FOR_BODY:%.*]] ]
+; ALL-NEXT:[[CHANGED_1_OFF0:%.*]] = phi i1 [ true, [[IF_THEN]] ], [ false, [[ENTRY:%.*]] ]
 ; ALL-NEXT:[[TMP1:%.*]] = load i32, i32* [[C]], align 4
 ; ALL-NEXT:[[CMP_1:%.*]] = icmp eq i32 [[OR]], [[TMP1]]
 ; ALL-NEXT:br i1 [[CMP_1]], label [[IF_END_1:%.*]], label [[IF_THEN_1:%.*]]
Index: llvm/test/Transforms/MemCpyOpt/lifetime.ll
===
--- llvm/test/Transforms/MemCpyOpt/lifetime.ll
+++ llvm/test/Transforms/MemCpyOpt/lifetime.ll
@@ -1,4 +1,4 @@
-; RUN: opt < %s -O1 -S | FileCheck %s
+; RUN: opt < %s -O2 -S | FileCheck %s
 
 ; performCallSlotOptzn in MemCpy should not exchange the calls to
 ; @llvm.lifetime.start and @llvm.memcpy.
Index: llvm/test/Other/new-pm-thinlto-defaults.ll
===
--- llvm/test/Other/new-pm-thinlto-defaults.ll
+++ llvm/test/Other/new-pm-thinlto-defaults.ll
@@ -108,13 +108,28 @@
 ; CHECK-O3-NEXT: Running pass: ArgumentPromotionPass
 ; CHECK-O-NEXT: Running pass: CGSCCToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
-; CHECK-O-NEXT: Running pass: SROA
+; CHECK-O2-NEXT: Running pass: SROA
+; CHECK-O3-NEXT: Running pass: SROA
+; CHECK-Os-NEXT: Running pass: SROA
+; CHECK-Oz-NEXT: Running pass: SROA
 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: MemorySSAAnalysis
-; CHECK-O-NEXT: Running pass: SpeculativeExecutionPass
-; CHECK-O-NEXT: Running pass: JumpThreadingPass
-; CHECK-O-NEXT: Running analysis: LazyValueAnalysis
-; CHECK-O-NEXT: Running pass: CorrelatedValuePropagationPass
+; CHECK-O2-NEXT: Running pass: SpeculativeExecutionPass
+; CHECK-O3-NEXT: Running pass: SpeculativeExecutionPass
+; 

[PATCH] D65343: [clang-tidy] Fix the documentation for linuxkernel-must-use-errs.

[Tom Roeder via Phabricator via cfe-commits]

tmroeder updated this revision to Diff 212201.
tmroeder added a comment.

Sync to HEAD to prepare for commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65343

Files:
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst


Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -3,14 +3,16 @@
 linuxkernel-must-use-errs
 =
 
-Checks for cases where the kernel error functions ``ERR_PTR``,
-``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
-``PTR_ERR_OR_ZERO`` are called but the results are not used. These
-functions are marked with ``__attribute__((warn_unused_result))``, but
-the compiler warning for this attribute is not always enabled.
-
-This also checks for unused values returned by functions that return
-``ERR_PTR``.
+Checks Linux kernel code to see if it uses the results from the functions in
+``linux/err.h``. Also checks to see if code uses the results from functions 
that
+directly return a value from one of these error functions.
+
+This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return
+values must be checked, since positive pointers and negative error codes are
+being used in the same context. These functions are marked with
+``__attribute__((warn_unused_result))``, but some kernel versions do not have
+this warning enabled for clang.
 
 Examples:
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -71,15 +71,7 @@
   ` check.
 
   Checks Linux kernel code to see if it uses the results from the functions in
-  ``linux/err.h``. Also checks to see if code uses the results from functions 
that
-  directly return a value from one of these error functions.
-
-  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
-  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return
-  values must be checked, since positive pointers and negative error codes are
-  being used in the same context. These functions are marked with
-  ``__attribute__((warn_unused_result))``, but some kernel versions do not have
-  this warning enabled for clang.
+  ``linux/err.h``.
 
 Improvements to include-fixer
 -


Index: clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
+++ clang-tools-extra/docs/clang-tidy/checks/linuxkernel-must-use-errs.rst
@@ -3,14 +3,16 @@
 linuxkernel-must-use-errs
 =
 
-Checks for cases where the kernel error functions ``ERR_PTR``,
-``PTR_ERR``, ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and
-``PTR_ERR_OR_ZERO`` are called but the results are not used. These
-functions are marked with ``__attribute__((warn_unused_result))``, but
-the compiler warning for this attribute is not always enabled.
-
-This also checks for unused values returned by functions that return
-``ERR_PTR``.
+Checks Linux kernel code to see if it uses the results from the functions in
+``linux/err.h``. Also checks to see if code uses the results from functions that
+directly return a value from one of these error functions.
+
+This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
+``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return
+values must be checked, since positive pointers and negative error codes are
+being used in the same context. These functions are marked with
+``__attribute__((warn_unused_result))``, but some kernel versions do not have
+this warning enabled for clang.
 
 Examples:
 
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -71,15 +71,7 @@
   ` check.
 
   Checks Linux kernel code to see if it uses the results from the functions in
-  ``linux/err.h``. Also checks to see if code uses the results from functions that
-  directly return a value from one of these error functions.
-
-  This is important in the Linux kernel because ``ERR_PTR``, ``PTR_ERR``,
-  ``IS_ERR``, ``IS_ERR_OR_NULL``, ``ERR_CAST``, and ``PTR_ERR_OR_ZERO`` return
-  values must be checked, since positive pointers and negative error codes are
-  being used in the same context. These functions are marked with
-  

[PATCH] D64793: [Driver] Properly use values-X[ca].o, values-xpg[46].o on Solaris

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

In D64793#1597765 , @ro wrote:

> In D64793#1597550 , @jyknight wrote:
>
> > How about instead just adding "values-xpg6.o" unconditionally, alongside 
> > the current unconditional "values-Xa.o", and just forget about the xpg4 and 
> > Xc modes?
>
>
> If all else fails, that would have to be the last fallback.  I'd rather do 
> things correctly, though.


I think the cost of complexity in the solaris-specific part of the driver is 
low, so if the maintainer thinks it's worth having, we can certainly add it.




Comment at: lib/Driver/ToolChains/Solaris.cpp:16
 #include "clang/Driver/Options.h"
+#include "clang/Frontend/LangStandard.h"
 #include "llvm/Option/ArgList.h"

ro wrote:
> jyknight wrote:
> > I'm not sure that's an acceptable dependency -- I think Driver probably is 
> > not supposed to depend on Frontend. If so, I guess LangStandard should move 
> > to clang/Basic/. Which also means moving InputKind from 
> > clang/include/clang/Frontend/FrontendOptions.h.
> > 
> > (Maybe someone else can weigh in on this question?)
> I wondered about this myself, including frontend code in the
> driver might be considered a layering violation.  I certainly need
> guidance here what is and isn't acceptable here.
I see there are no other includes of Frontend from Driver, so I think 
LangStandards* does need to move to Basic. The only piece of InputKind that's 
needed is the language enum. I'm surprised there isn't already one somewhere 
else, but if there isn't, I think it would be reasonable to define the input 
kind languages in LangStandard.h and use them from FrontendOptions.



Comment at: lib/Frontend/LangStandards.cpp:31-37
   Kind K = llvm::StringSwitch(Name)
 #define LANGSTANDARD(id, name, lang, desc, features) \
 .Case(name, lang_##id)
+#define LANGSTANDARD_ALIAS(id, alias) \
+.Case(alias, lang_##id)
 #include "clang/Frontend/LangStandards.def"
 .Default(lang_unspecified);

I see that this code pattern is repeated in two other places, 
lib/Tooling/InterpolatingCompilationDatabase.cpp and 
lib/Frontend/CompilerInvocation.cpp. I think it would be good to factor out a 
string-to-kind helper and use it in the three places.


Repository:
  rC Clang

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

https://reviews.llvm.org/D64793



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