[PATCH] D46052: GNUstep Objective-C ABI version 2

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added inline comments.



Comment at: lib/CodeGen/CGObjCGNU.cpp:1056
+char c = Str[i];
+if (isalpha(c) || isnumber(c))
+  StringName += c;

The isnumber() function was added to cctype.h by Apple. I don't think it can be 
used in llvm.

According to
https://stackoverflow.com/questions/39204080/what-is-the-difference-between-isdigit-and-isnumber



Repository:
  rC Clang

https://reviews.llvm.org/D46052



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a reviewer: JDevlieghere.
Ka-Ka added a comment.

Added Jonas Devlieghere as reviewer as he was involved in the review of 
https://reviews.llvm.org/rL327851 (which seems to be the reason for this patch).


https://reviews.llvm.org/D45686



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

In https://reviews.llvm.org/D45686#1113414, @aaron.ballman wrote:

> In https://reviews.llvm.org/D45686#1109295, @dstenb wrote:
>
> > Ping.
> >
> > We have added a lit reproducer for this now in clang-tools-extra: 
> > https://reviews.llvm.org/D47251.
>
>
> The above should be rolled into this patch as the test case verifying the 
> behavioral changes.


I guess the intention from @dstenb was to submit both patches together. The 
reason for a separate patch with the testcase alone is that the reproducer use 
clang-tidy and as far as I know all clang-tidy tests exist in clang-tools-extra.


https://reviews.llvm.org/D45686



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

[PATCH] D45686: [Driver] Clean up tmp files when deleting Compilation objects

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

In https://reviews.llvm.org/D45686#1113426, @lebedev.ri wrote:

> In https://reviews.llvm.org/D45686#1113425, @Ka-Ka wrote:
>
> > In https://reviews.llvm.org/D45686#1113414, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D45686#1109295, @dstenb wrote:
> > >
> > > > Ping.
> > > >
> > > > We have added a lit reproducer for this now in clang-tools-extra: 
> > > > https://reviews.llvm.org/D47251.
> > >
> > >
> > > The above should be rolled into this patch as the test case verifying the 
> > > behavioral changes.
> >
> >
> > I guess the intention from @dstenb was to submit both patches together. The 
> > reason for a separate patch with the testcase alone is that the reproducer 
> > use clang-tidy and as far as I know all clang-tidy tests exist in 
> > clang-tools-extra.
>
>
> .. and both of them exist in the **same** svn repo, or git monorepo. 
> (granted, not many use that)


I see, I didn't know that. Well, then it make sense to merge both patches into 
a single one. Thanks for clarifying that.


https://reviews.llvm.org/D45686



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

[PATCH] D55438: Assert on conflicting fixes for -fix option

[Karl-Johan Karlsson via Phabricator via cfe-commits]

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

Changed the assert and corresponding warning message for conflicting fixes
to only trigger if clang-tidy is run with -fix or -fix-errors.
The Fix and FixErrors variables are now passed as parameters when creating
respective objects.

This is not a finished patch. We need input if there are a more appropriate fix 
of this problem.

Patch by: Henric Karlsson and Björn Pettersson


Repository:
  rC Clang

https://reviews.llvm.org/D55438

Files:
  lib/StaticAnalyzer/Core/ExprEngineC.cpp
  tools/extra/clang-tidy/ClangTidy.cpp
  tools/extra/clang-tidy/ClangTidy.h
  tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  tools/extra/clang-tidy/tool/ClangTidyMain.cpp
  tools/extra/test/clang-tidy/clang-tidy-logicalexpr-crash.c

Index: tools/extra/test/clang-tidy/clang-tidy-logicalexpr-crash.c
===
--- /dev/null
+++ tools/extra/test/clang-tidy/clang-tidy-logicalexpr-crash.c
@@ -0,0 +1,7 @@
+// RUN: clang-tidy -checks='-*,clang-analyzer-*' %s
+
+a;
+fn1() {
+  int b = -a ?: a;
+  (b || 0) && 9;
+}
Index: tools/extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- tools/extra/clang-tidy/tool/ClangTidyMain.cpp
+++ tools/extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -426,7 +426,7 @@
AllowEnablingAnalyzerAlphaCheckers);
   std::vector Errors =
   runClangTidy(Context, OptionsParser.getCompilations(), PathList, BaseFS,
-   EnableCheckProfile, ProfilePrefix);
+   (Fix || FixErrors), EnableCheckProfile, ProfilePrefix);
   bool FoundErrors = llvm::find_if(Errors, [](const ClangTidyError &E) {
return E.DiagLevel == ClangTidyError::Error;
  }) != Errors.end();
Index: tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -226,7 +226,8 @@
 class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
 public:
   ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
-  bool RemoveIncompatibleErrors = true);
+  bool RemoveIncompatibleErrors = true,
+  bool FixResolutions = false);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
   // more generic and should be pulled out into a more useful Diagnostics
@@ -252,6 +253,7 @@
 
   ClangTidyContext &Context;
   bool RemoveIncompatibleErrors;
+  bool FixResolutions;
   std::vector Errors;
   std::unique_ptr HeaderFilter;
   bool LastErrorRelatesToUserCode;
Index: tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ tools/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -33,8 +33,10 @@
 public:
   ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
   DiagnosticOptions *DiagOpts,
-  ClangTidyError &Error)
-  : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
+  ClangTidyError &Error,
+  bool FixResolutions)
+  : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error),
+FixResolutions(FixResolutions) {}
 
 protected:
   void emitDiagnosticMessage(FullSourceLoc Loc, PresumedLoc PLoc,
@@ -81,10 +83,13 @@
   llvm::Error Err = Error.Fix[Replacement.getFilePath()].add(Replacement);
   // FIXME: better error handling (at least, don't let other replacements be
   // applied).
-  if (Err) {
+  if (Err && FixResolutions) {
 llvm::errs() << "Fix conflicts with existing fix! "
  << llvm::toString(std::move(Err)) << "\n";
 assert(false && "Fix conflicts with existing fix!");
+  } else if (Err) {
+// Replacements wont be used for fixing anything.
+consumeError(std::move(Err));
   }
 }
   }
@@ -104,6 +109,7 @@
 
 private:
   ClangTidyError &Error;
+  bool FixResolutions;
 };
 } // end anonymous namespace
 
@@ -264,10 +270,10 @@
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
-ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
+ClangTidyContext &Ctx, bool RemoveIncompatibleErrors , bool FixResolutions )
 : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
-  LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
-  LastErrorWasIgnored(false) {}
+  FixResolutions(FixResolutions), LastErrorRelatesToUserCode(false),
+  LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {}
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
  

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka created this revision.
Herald added a subscriber: aprantl.

As no stoppoint was generated the attributed statements got faulty debug 
locations.


https://reviews.llvm.org/D37428

Files:
  lib/CodeGen/CGStmt.cpp
  test/CodeGen/debug-info-attributed-stmt.c


Index: test/CodeGen/debug-info-attributed-stmt.c
===
--- /dev/null
+++ test/CodeGen/debug-info-attributed-stmt.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -debug-info-kind=limited -emit-llvm 
%s -o - | FileCheck %s
+int data[50] = { 0 };
+
+void foo()
+{
+int i = 0;
+int x = 7;
+#pragma nounroll
+while (i < 50)
+{
+data[i] = i;
+++i;
+}
+
+// CHECK: br label %while.cond, !dbg ![[NUM:[0-9]+]]
+// CHECK: br i1 %cmp, label %while.body, label %while.end, !dbg ![[NUM]]
+// CHECK: br label %while.cond, !dbg ![[NUM]], !llvm.loop
+// CHECK: ![[NUM]] = !DILocation(line: 9, scope: !14)
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -556,6 +556,10 @@
 
 void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
   const Stmt *SubStmt = S.getSubStmt();
+
+  // Generate a stoppoint if we are emitting debug info.
+  EmitStopPoint(SubStmt);
+
   switch (SubStmt->getStmtClass()) {
   case Stmt::DoStmtClass:
 EmitDoStmt(cast(*SubStmt), S.getAttrs());


Index: test/CodeGen/debug-info-attributed-stmt.c
===
--- /dev/null
+++ test/CodeGen/debug-info-attributed-stmt.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -debug-info-kind=limited -emit-llvm %s -o - | FileCheck %s
+int data[50] = { 0 };
+
+void foo()
+{
+int i = 0;
+int x = 7;
+#pragma nounroll
+while (i < 50)
+{
+data[i] = i;
+++i;
+}
+
+// CHECK: br label %while.cond, !dbg ![[NUM:[0-9]+]]
+// CHECK: br i1 %cmp, label %while.body, label %while.end, !dbg ![[NUM]]
+// CHECK: br label %while.cond, !dbg ![[NUM]], !llvm.loop
+// CHECK: ![[NUM]] = !DILocation(line: 9, scope: !14)
+}
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -556,6 +556,10 @@
 
 void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
   const Stmt *SubStmt = S.getSubStmt();
+
+  // Generate a stoppoint if we are emitting debug info.
+  EmitStopPoint(SubStmt);
+
   switch (SubStmt->getStmtClass()) {
   case Stmt::DoStmtClass:
 EmitDoStmt(cast(*SubStmt), S.getAttrs());
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added inline comments.



Comment at: test/CodeGen/debug-info-attributed-stmt.c:18
+// CHECK: br label %while.cond, !dbg ![[NUM]], !llvm.loop
+// CHECK: ![[NUM]] = !DILocation(line: 9, scope: !14)
+}

The part "scope: !14)" should be removed from the testcase.



https://reviews.llvm.org/D37428



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

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka updated this revision to Diff 113803.
Ka-Ka edited the summary of this revision.
Ka-Ka added a reviewer: dblaikie.
Ka-Ka added a comment.

I updated the testcase according to what David Blaikie suggested.
I also rewrote to code to be a bit more robust and avoid emitting unnecessary 
stoppoints.


https://reviews.llvm.org/D37428

Files:
  lib/CodeGen/CGStmt.cpp
  lib/CodeGen/CodeGenFunction.h
  test/CodeGen/debug-info-attributed-stmt.c

Index: test/CodeGen/debug-info-attributed-stmt.c
===
--- /dev/null
+++ test/CodeGen/debug-info-attributed-stmt.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple x86_64-unk-unk -disable-llvm-passes -debug-info-kind=limited -emit-llvm %s -o - | FileCheck %s
+
+void f(_Bool b)
+{
+#pragma nounroll
+  while (b);
+}
+
+// CHECK: br label %while.cond, !dbg ![[NUM:[0-9]+]]
+// CHECK: br i1 %tobool, label %while.body, label %while.end, !dbg ![[NUM]]
+// CHECK: br label %while.cond, !dbg ![[NUM]], !llvm.loop
+// CHECK: ![[NUM]] = !DILocation(line: 6,
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2534,7 +2534,7 @@
   /// This function may clear the current insertion point; callers should use
   /// EnsureInsertPoint if they wish to subsequently generate code without first
   /// calling EmitBlock, EmitBranch, or EmitStmt.
-  void EmitStmt(const Stmt *S);
+  void EmitStmt(const Stmt *S, ArrayRef Attrs = None);
 
   /// EmitSimpleStmt - Try to emit a "simple" statement which does not
   /// necessarily require an insertion point or debug information; typically
Index: lib/CodeGen/CGStmt.cpp
===
--- lib/CodeGen/CGStmt.cpp
+++ lib/CodeGen/CGStmt.cpp
@@ -45,7 +45,7 @@
   }
 }
 
-void CodeGenFunction::EmitStmt(const Stmt *S) {
+void CodeGenFunction::EmitStmt(const Stmt *S, ArrayRef Attrs) {
   assert(S && "Null statement?");
   PGO.setCurrentStmt(S);
 
@@ -131,16 +131,16 @@
   case Stmt::IndirectGotoStmtClass:
 EmitIndirectGotoStmt(cast(*S)); break;
 
-  case Stmt::IfStmtClass:   EmitIfStmt(cast(*S)); break;
-  case Stmt::WhileStmtClass:EmitWhileStmt(cast(*S));   break;
-  case Stmt::DoStmtClass:   EmitDoStmt(cast(*S)); break;
-  case Stmt::ForStmtClass:  EmitForStmt(cast(*S));   break;
+  case Stmt::IfStmtClass:  EmitIfStmt(cast(*S));  break;
+  case Stmt::WhileStmtClass:   EmitWhileStmt(cast(*S), Attrs); break;
+  case Stmt::DoStmtClass:  EmitDoStmt(cast(*S), Attrs);   break;
+  case Stmt::ForStmtClass: EmitForStmt(cast(*S), Attrs); break;
 
-  case Stmt::ReturnStmtClass:   EmitReturnStmt(cast(*S)); break;
+  case Stmt::ReturnStmtClass:  EmitReturnStmt(cast(*S));  break;
 
-  case Stmt::SwitchStmtClass:   EmitSwitchStmt(cast(*S)); break;
-  case Stmt::GCCAsmStmtClass:   // Intentional fall-through.
-  case Stmt::MSAsmStmtClass:EmitAsmStmt(cast(*S));   break;
+  case Stmt::SwitchStmtClass:  EmitSwitchStmt(cast(*S));  break;
+  case Stmt::GCCAsmStmtClass:  // Intentional fall-through.
+  case Stmt::MSAsmStmtClass:   EmitAsmStmt(cast(*S));break;
   case Stmt::CoroutineBodyStmtClass:
 EmitCoroutineBody(cast(*S));
 break;
@@ -178,7 +178,7 @@
 EmitCXXTryStmt(cast(*S));
 break;
   case Stmt::CXXForRangeStmtClass:
-EmitCXXForRangeStmt(cast(*S));
+EmitCXXForRangeStmt(cast(*S), Attrs);
 break;
   case Stmt::SEHTryStmtClass:
 EmitSEHTryStmt(cast(*S));
@@ -555,23 +555,7 @@
 }
 
 void CodeGenFunction::EmitAttributedStmt(const AttributedStmt &S) {
-  const Stmt *SubStmt = S.getSubStmt();
-  switch (SubStmt->getStmtClass()) {
-  case Stmt::DoStmtClass:
-EmitDoStmt(cast(*SubStmt), S.getAttrs());
-break;
-  case Stmt::ForStmtClass:
-EmitForStmt(cast(*SubStmt), S.getAttrs());
-break;
-  case Stmt::WhileStmtClass:
-EmitWhileStmt(cast(*SubStmt), S.getAttrs());
-break;
-  case Stmt::CXXForRangeStmtClass:
-EmitCXXForRangeStmt(cast(*SubStmt), S.getAttrs());
-break;
-  default:
-EmitStmt(SubStmt);
-  }
+  EmitStmt(S.getSubStmt(), S.getAttrs());
 }
 
 void CodeGenFunction::EmitGotoStmt(const GotoStmt &S) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

In https://reviews.llvm.org/D37428#861140, @dblaikie wrote:

> Not sure what you mean by "avoid emitting unnecessary stop points" - do you 
> have a test case for that?


In my previous patch you could end up doing two calls to EmitStopPoint() for 
the same statement. In the previous patch I added a EmitStopPoint() in the 
method EmitAttributedStmt(), but if you took the default clause in the switch 
statement in the same method you could end up executing another EmitStopPoint() 
in EmitStmt().

The new patch do not have the issue with executing EmitStopPoint() twice for 
the same statement. I think generally that the new patch is more robust 
solution.


https://reviews.llvm.org/D37428



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

[PATCH] D37428: Debug info: Fixed faulty debug locations for attributed statements

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added inline comments.



Comment at: test/CodeGen/debug-info-attributed-stmt.c:1
+// RUN: %clang_cc1 -triple x86_64-unk-unk -disable-llvm-passes 
-debug-info-kind=limited -emit-llvm %s -o - | FileCheck %s
+

echristo wrote:
> Since we're not optimizing or generating code you should be able to remove 
> the -disable-llvm-passes I believe.
I added the option -disable-llvm-passes as David Blaikie suggested it in his 
original comment:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170904/202982.html
As I have mainly worked in llvm backends I'm not that familiar with the clang 
-cc1 options. I have no opinion about this. I wait for you and David Blaikie to 
decide if I should remove or keep the option -disable-llvm-passes.



https://reviews.llvm.org/D37428



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added inline comments.



Comment at: clang/test/CodeGen/builtins-x86.c:127-128
 
-  tmp_V2LLi = __builtin_ia32_undef128();
-  tmp_V4LLi = __builtin_ia32_undef256();
+  tmp_V2LLi = (V2LLi)__builtin_ia32_undef128();
+  tmp_V4LLi = (V4LLi)__builtin_ia32_undef256();
 

I don't like the introduced casts. Can we change the testcase to operate on the 
appropriate types instead?
The above two lines could probably be replaced by:
  tmp_V2d = __builtin_ia32_undef128();
  tmp_V4d = __builtin_ia32_undef256();
What do you think?




Comment at: clang/test/CodeGen/builtins-x86.c:228-231
+  tmp_V8s = (V8s)__builtin_ia32_packsswb128(tmp_V8s, tmp_V8s);
+  tmp_V4i = (V4i)__builtin_ia32_packssdw128(tmp_V4i, tmp_V4i);
+  tmp_V8s = (V8s)__builtin_ia32_packuswb128(tmp_V8s, tmp_V8s);
   tmp_V8s = __builtin_ia32_pmulhuw128(tmp_V8s, tmp_V8s);

same here?



Comment at: clang/test/CodeGen/builtins-x86.c:250
   tmp_V4s = __builtin_ia32_phsubsw(tmp_V4s, tmp_V4s);
-  tmp_V16c = __builtin_ia32_pmaddubsw128(tmp_V16c, tmp_V16c);
+  tmp_V16c = (V16c)__builtin_ia32_pmaddubsw128(tmp_V16c, tmp_V16c);
   tmp_V8c = __builtin_ia32_pmaddubsw(tmp_V8c, tmp_V8c);

and here?



Comment at: clang/test/CodeGen/builtins-x86.c:428
   tmp_V4i = __builtin_ia32_psradi128(tmp_V4i, tmp_i);
-  tmp_V8s = __builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);
+  tmp_V8s = (V8s)__builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);
   (void) __builtin_ia32_monitor(tmp_vp, tmp_Ui, tmp_Ui);

Can we change this line to?
  tmp_V4i = __builtin_ia32_pmaddwd128(tmp_V8s, tmp_V8s);




Comment at: clang/test/CodeGen/builtins-x86.c:432
   tmp_V16c = __builtin_ia32_lddqu(tmp_cCp);
-  tmp_V2LLi = __builtin_ia32_palignr128(tmp_V2LLi, tmp_V2LLi, imm_i);
-  tmp_V1LLi = __builtin_ia32_palignr(tmp_V1LLi, tmp_V1LLi, imm_i);
+  tmp_V2LLi = (V2LLi)__builtin_ia32_palignr128((V16c)tmp_V2LLi, 
(V16c)tmp_V2LLi, imm_i);
+  tmp_V1LLi = (V1LLi)__builtin_ia32_palignr((V8c)tmp_V1LLi, (V8c)tmp_V1LLi, 
imm_i);

Can we change this line to?
  tmp_V16c = __builtin_ia32_palignr128(tmp_V16c, tmp_V16c, imm_i);




Comment at: clang/test/CodeGen/builtins-x86.c:433
+  tmp_V2LLi = (V2LLi)__builtin_ia32_palignr128((V16c)tmp_V2LLi, 
(V16c)tmp_V2LLi, imm_i);
+  tmp_V1LLi = (V1LLi)__builtin_ia32_palignr((V8c)tmp_V1LLi, (V8c)tmp_V1LLi, 
imm_i);
 #ifdef USE_SSE4

Can we change this line to?
  tmp_V8c = __builtin_ia32_palignr(tmp_V8c, tmp_V8c, imm_i);



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62580



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

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka accepted this revision.
Ka-Ka added a comment.

Thanks!


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

https://reviews.llvm.org/D62580



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

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka created this revision.
Ka-Ka added reviewers: dylanmckay, spatel, rsmith.
Herald added a subscriber: kristina.
Herald added a project: clang.

The definition of the builtins __builtin_bswap32, __builtin_bitreverse32, 
__builtin_rotateleft32 and __builtin_rotateright32 rely on that the int type is 
32 bits wide on the target.
The defintions of the builtins __builtin_bswap64, __builtin_bitreverse64, 
__builtin_rotateleft64, and __builtin_rotateright64 rely on that the long long 
type is 64 bits wide.

On targets where this is not the case (e.g. AVR) clang will generate faulty 
code (wrong llvm assembler intrinsics).

This patch add support for using 'Z' (the int32_t type) in Bultins.def. The 
builtins above are changed to be based on the int32_t type instead of the int 
type, and the int64_t type instead of the long long type.

The AVR backend (experimental) have a native int type that is only 16 bits 
wide. The supplied testcase will therefore fail if running the testcase on 
trunk as clang will convert e.g. __builtin_bitreverse32 into 
llvm.bitreverse.i16 on AVR.


Repository:
  rC Clang

https://reviews.llvm.org/D61845

Files:
  include/clang/Basic/Builtins.def
  lib/AST/ASTContext.cpp
  test/CodeGen/avr-builtins.c

Index: test/CodeGen/avr-builtins.c
===
--- /dev/null
+++ test/CodeGen/avr-builtins.c
@@ -0,0 +1,103 @@
+// REQUIRES: avr-registered-target
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+unsigned char bitrev8(unsigned char data) {
+return __builtin_bitreverse8(data);
+}
+
+// CHECK: define zeroext i8 @bitrev8
+// CHECK: i8 @llvm.bitreverse.i8(i8
+
+unsigned int bitrev16(unsigned int data) {
+return __builtin_bitreverse16(data);
+}
+
+// CHECK: define i16 @bitrev16
+// CHECK: i16 @llvm.bitreverse.i16(i16
+
+unsigned long bitrev32(unsigned long data) {
+return __builtin_bitreverse32(data);
+}
+// CHECK: define i32 @bitrev32
+// CHECK: i32 @llvm.bitreverse.i32(i32
+
+unsigned long long bitrev64(unsigned long long data) {
+return __builtin_bitreverse64(data);
+}
+
+// CHECK: define i64 @bitrev64
+// CHECK: i64 @llvm.bitreverse.i64(i64
+
+unsigned char rotleft8(unsigned char x, unsigned char y) {
+return __builtin_rotateleft8(x, y);
+}
+
+// CHECK: define zeroext i8 @rotleft8
+// CHECK: i8 @llvm.fshl.i8(i8
+
+unsigned int rotleft16(unsigned int x, unsigned int y) {
+return __builtin_rotateleft16(x, y);
+}
+
+// CHECK: define i16 @rotleft16
+// CHECK: i16 @llvm.fshl.i16(i16
+
+unsigned long rotleft32(unsigned long x, unsigned long y) {
+return __builtin_rotateleft32(x, y);
+}
+// CHECK: define i32 @rotleft32
+// CHECK: i32 @llvm.fshl.i32(i32
+
+unsigned long long rotleft64(unsigned long long x, unsigned long long y) {
+return __builtin_rotateleft64(x, y);
+}
+
+// CHECK: define i64 @rotleft64
+// CHECK: i64 @llvm.fshl.i64(i64
+
+unsigned char rotright8(unsigned char x, unsigned char y) {
+return __builtin_rotateright8(x, y);
+}
+
+// CHECK: define zeroext i8 @rotright8
+// CHECK: i8 @llvm.fshr.i8(i8
+
+unsigned int rotright16(unsigned int x, unsigned int y) {
+return __builtin_rotateright16(x, y);
+}
+
+// CHECK: define i16 @rotright16
+// CHECK: i16 @llvm.fshr.i16(i16
+
+unsigned long rotright32(unsigned long x, unsigned long y) {
+return __builtin_rotateright32(x, y);
+}
+// CHECK: define i32 @rotright32
+// CHECK: i32 @llvm.fshr.i32(i32
+
+unsigned long long rotright64(unsigned long long x, unsigned long long y) {
+return __builtin_rotateright64(x, y);
+}
+
+// CHECK: define i64 @rotright64
+// CHECK: i64 @llvm.fshr.i64(i64
+
+unsigned int byteswap16(unsigned int x) {
+return __builtin_bswap16(x);
+}
+
+// CHECK: define i16 @byteswap16
+// CHECK: i16 @llvm.bswap.i16(i16
+
+unsigned long byteswap32(unsigned long x) {
+return __builtin_bswap32(x);
+}
+// CHECK: define i32 @byteswap32
+// CHECK: i32 @llvm.bswap.i32(i32
+
+unsigned long long byteswap64(unsigned long long x) {
+return __builtin_bswap64(x);
+}
+
+// CHECK: define i64 @byteswap64
+// CHECK: i64 @llvm.bswap.i64(i64
Index: lib/AST/ASTContext.cpp
===
--- lib/AST/ASTContext.cpp
+++ lib/AST/ASTContext.cpp
@@ -9312,7 +9312,7 @@
   // Read the prefixed modifiers first.
   bool Done = false;
   #ifndef NDEBUG
-  bool IsSpecialLong = false;
+  bool IsSpecial = false;
   #endif
   while (!Done) {
 switch (*Str++) {
@@ -9331,26 +9331,26 @@
   Unsigned = true;
   break;
 case 'L':
-  assert(!IsSpecialLong && "Can't use 'L' with 'W' or 'N' modifiers");
+  assert(!IsSpecial && "Can't use 'L' with 'W', 'N' or 'Z' modifiers");
   assert(HowLong <= 2 && "Can't have  modifier");
   ++HowLong;
   break;
 case 'N':
   // 'N' behaves like 'L' for all non LP64 targets and 'int' otherwise.
-  assert(!IsSpecialLong && "Can't use two 'N' or 'W' modifiers!");
+  assert(!IsSpecial 

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka updated this revision to Diff 199385.
Ka-Ka marked an inline comment as done.
Ka-Ka added a comment.

Added a new C++ testcase.
Removed the REQUIRES: avr-registered-target in the avr-builtins.c testcase.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61845

Files:
  include/clang/Basic/Builtins.def
  lib/AST/ASTContext.cpp
  test/CodeGen/avr-builtins.c
  test/CodeGen/builtins.cpp

Index: test/CodeGen/builtins.cpp
===
--- /dev/null
+++ test/CodeGen/builtins.cpp
@@ -0,0 +1,55 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple i686-pc-linux-gnu -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple avr-unknown-unknown -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple powerpc-pc-linux -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple arm-linux-gnueabi -verify %s
+
+// Test that checks that the builtins return the same type as the stdint.h type
+// withe the same witdh. This is done by first declaring a variable of a stdint
+// type of the correct width and the redeclaring the variable with the type that
+// the builting return. If the types are different you should the an error from
+// clang of the form:
+// "redefinition of '' with a different type: '' vs ''
+// (with gcc you get an error message
+// "conflicting declaration ' '").
+// If the types are the same you only get an error message of the form
+// "redefinition of ''"
+// with both clang and gcc.
+
+#include 
+
+uint16_t bswap16; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap16(0)) bswap16 = 42; // expected-error-re{{redefinition of 'bswap16'{{$
+uint32_t bswap32; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap32(0)) bswap32 = 42; // expected-error-re{{redefinition of 'bswap32'{{$
+uint64_t bswap64; // expected-note{{previous definition is here}}
+decltype(__builtin_bswap64(0)) bswap64 = 42; // expected-error-re{{redefinition of 'bswap64'{{$
+
+#ifdef __clang__
+uint8_t bitrev8; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse8(0)) bitrev8 = 42; // expected-error-re{{redefinition of 'bitrev8'{{$
+uint16_t bitrev16; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse16(0)) bitrev16 = 42; // expected-error-re{{redefinition of 'bitrev16'{{$
+uint32_t bitrev32; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse32(0)) bitrev32 = 42; // expected-error-re{{redefinition of 'bitrev32'{{$
+uint64_t bitrev64; // expected-note{{previous definition is here}}
+decltype(__builtin_bitreverse64(0)) bitrev64 = 42; // expected-error-re{{redefinition of 'bitrev64'{{$
+
+uint8_t rotl8; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft8(0,0)) rotl8 = 42; // expected-error-re{{redefinition of 'rotl8'{{$
+uint16_t rotl16; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft16(0,0)) rotl16 = 42; // expected-error-re{{redefinition of 'rotl16'{{$
+uint32_t rotl32; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft32(0,0)) rotl32 = 42; // expected-error-re{{redefinition of 'rotl32'{{$
+uint64_t rotl64; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateleft64(0,0)) rotl64 = 42; // expected-error-re{{redefinition of 'rotl64'{{$
+
+uint8_t rotr8; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright8(0,0)) rotr8 = 42; // expected-error-re{{redefinition of 'rotr8'{{$
+uint16_t rotr16; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright16(0,0)) rotr16 = 42; // expected-error-re{{redefinition of 'rotr16'{{$
+uint32_t rotr32; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright32(0,0)) rotr32 = 42; // expected-error-re{{redefinition of 'rotr32'{{$
+uint64_t rotr64; // expected-note{{previous definition is here}}
+decltype(__builtin_rotateright64(0,0)) rotr64 = 42; // expected-error-re{{redefinition of 'rotr64'{{$
+#endif
Index: test/CodeGen/avr-builtins.c
===
--- /dev/null
+++ test/CodeGen/avr-builtins.c
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+unsigned char bitrev8(unsigned char data) {
+return __builtin_bitreverse8(data);
+}
+
+// CHECK: define zeroext i8 @bitrev8
+// CHECK: i8 @llvm.bitreverse.i8(i8
+
+unsigned int bitrev16(unsigned int data) {
+return __builtin_bitreverse16(data);
+}
+
+// CHECK: define i16 @bitrev16
+// CHECK: i16 @llvm.bitreverse.i16(i16
+
+unsigned long bitrev32(unsigned long data) {
+return __builtin_bitreverse32(data);
+}
+// CHECK: define i32 @bitrev32
+// CHECK: i32 @llvm.bitreverse.i32(i32
+
+unsigned long long bitrev64(unsigned long long data) {
+   

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka updated this revision to Diff 199559.
Ka-Ka marked 2 inline comments as done.
Ka-Ka added a comment.

Updated according to review comments


Repository:
  rC Clang

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

https://reviews.llvm.org/D61845

Files:
  include/clang/Basic/Builtins.def
  lib/AST/ASTContext.cpp
  test/CodeGen/avr-builtins.c
  test/CodeGen/builtins.cpp

Index: test/CodeGen/builtins.cpp
===
--- /dev/null
+++ test/CodeGen/builtins.cpp
@@ -0,0 +1,52 @@
+// RUN: %clang_cc1 -std=c++11 -triple x86_64-linux-gnu -ffreestanding -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple i686-pc-linux-gnu -ffreestanding -verify %s
+// RUN: %clang_cc1 -std=c++11 -triple avr-unknown-unknown -ffreestanding -verify %s
+
+// expected-no-diagnostics
+
+// Test that checks that the builtins return the same type as the stdint.h type
+// withe the same witdh. This is done by first declaring a variable of a stdint
+// type of the correct width and the redeclaring the variable with the type that
+// the builting return. If the types are different you should the an error from
+// clang of the form:
+// "redefinition of '' with a different type: '' vs ''
+// (with gcc you get an error message
+// "conflicting declaration ' '").
+
+#include 
+
+extern uint16_t bswap16;
+decltype(__builtin_bswap16(0)) bswap16 = 42;
+extern uint32_t bswap32;
+decltype(__builtin_bswap32(0)) bswap32 = 42;
+extern uint64_t bswap64;
+decltype(__builtin_bswap64(0)) bswap64 = 42;
+
+#ifdef __clang__
+extern uint8_t bitrev8;
+decltype(__builtin_bitreverse8(0)) bitrev8 = 42;
+extern uint16_t bitrev16;
+decltype(__builtin_bitreverse16(0)) bitrev16 = 42;
+extern uint32_t bitrev32;
+decltype(__builtin_bitreverse32(0)) bitrev32 = 42;
+extern uint64_t bitrev64;
+decltype(__builtin_bitreverse64(0)) bitrev64 = 42;
+
+extern uint8_t rotl8;
+decltype(__builtin_rotateleft8(0,0)) rotl8 = 42;
+extern uint16_t rotl16;
+decltype(__builtin_rotateleft16(0,0)) rotl16 = 42;
+extern uint32_t rotl32;
+decltype(__builtin_rotateleft32(0,0)) rotl32 = 42;
+extern uint64_t rotl64;
+decltype(__builtin_rotateleft64(0,0)) rotl64 = 42;
+
+extern uint8_t rotr8;
+decltype(__builtin_rotateright8(0,0)) rotr8 = 42;
+extern uint16_t rotr16;
+decltype(__builtin_rotateright16(0,0)) rotr16 = 42;
+extern uint32_t rotr32;
+decltype(__builtin_rotateright32(0,0)) rotr32 = 42;
+extern uint64_t rotr64;
+decltype(__builtin_rotateright64(0,0)) rotr64 = 42;
+#endif
Index: test/CodeGen/avr-builtins.c
===
--- /dev/null
+++ test/CodeGen/avr-builtins.c
@@ -0,0 +1,102 @@
+// RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
+
+unsigned char bitrev8(unsigned char data) {
+return __builtin_bitreverse8(data);
+}
+
+// CHECK: define zeroext i8 @bitrev8
+// CHECK: i8 @llvm.bitreverse.i8(i8
+
+unsigned int bitrev16(unsigned int data) {
+return __builtin_bitreverse16(data);
+}
+
+// CHECK: define i16 @bitrev16
+// CHECK: i16 @llvm.bitreverse.i16(i16
+
+unsigned long bitrev32(unsigned long data) {
+return __builtin_bitreverse32(data);
+}
+// CHECK: define i32 @bitrev32
+// CHECK: i32 @llvm.bitreverse.i32(i32
+
+unsigned long long bitrev64(unsigned long long data) {
+return __builtin_bitreverse64(data);
+}
+
+// CHECK: define i64 @bitrev64
+// CHECK: i64 @llvm.bitreverse.i64(i64
+
+unsigned char rotleft8(unsigned char x, unsigned char y) {
+return __builtin_rotateleft8(x, y);
+}
+
+// CHECK: define zeroext i8 @rotleft8
+// CHECK: i8 @llvm.fshl.i8(i8
+
+unsigned int rotleft16(unsigned int x, unsigned int y) {
+return __builtin_rotateleft16(x, y);
+}
+
+// CHECK: define i16 @rotleft16
+// CHECK: i16 @llvm.fshl.i16(i16
+
+unsigned long rotleft32(unsigned long x, unsigned long y) {
+return __builtin_rotateleft32(x, y);
+}
+// CHECK: define i32 @rotleft32
+// CHECK: i32 @llvm.fshl.i32(i32
+
+unsigned long long rotleft64(unsigned long long x, unsigned long long y) {
+return __builtin_rotateleft64(x, y);
+}
+
+// CHECK: define i64 @rotleft64
+// CHECK: i64 @llvm.fshl.i64(i64
+
+unsigned char rotright8(unsigned char x, unsigned char y) {
+return __builtin_rotateright8(x, y);
+}
+
+// CHECK: define zeroext i8 @rotright8
+// CHECK: i8 @llvm.fshr.i8(i8
+
+unsigned int rotright16(unsigned int x, unsigned int y) {
+return __builtin_rotateright16(x, y);
+}
+
+// CHECK: define i16 @rotright16
+// CHECK: i16 @llvm.fshr.i16(i16
+
+unsigned long rotright32(unsigned long x, unsigned long y) {
+return __builtin_rotateright32(x, y);
+}
+// CHECK: define i32 @rotright32
+// CHECK: i32 @llvm.fshr.i32(i32
+
+unsigned long long rotright64(unsigned long long x, unsigned long long y) {
+return __builtin_rotateright64(x, y);
+}
+
+// CHECK: define i64 @rotright64
+// CHECK: i64 @llvm.fshr.i64(i64
+
+unsigned int byteswap16(unsigned int x) {
+return __builtin_bswap16(x);
+}
+
+// CHECK: define i16 @byteswap16
+

[PATCH] D61845: [builtin] Fixed definitions of builtins that rely on the int/long long type is 32/64 bits

[Karl-Johan Karlsson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL360863: [builtin] Fixed definitions of builtins that rely on 
the int/long long type is… (authored by karka, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61845

Files:
  cfe/trunk/include/clang/Basic/Builtins.def
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/test/CodeGen/avr-builtins.c
  cfe/trunk/test/CodeGen/builtins.cpp

Index: cfe/trunk/lib/AST/ASTContext.cpp
===
--- cfe/trunk/lib/AST/ASTContext.cpp
+++ cfe/trunk/lib/AST/ASTContext.cpp
@@ -9286,7 +9286,7 @@
   // Read the prefixed modifiers first.
   bool Done = false;
   #ifndef NDEBUG
-  bool IsSpecialLong = false;
+  bool IsSpecial = false;
   #endif
   while (!Done) {
 switch (*Str++) {
@@ -9305,26 +9305,26 @@
   Unsigned = true;
   break;
 case 'L':
-  assert(!IsSpecialLong && "Can't use 'L' with 'W' or 'N' modifiers");
+  assert(!IsSpecial && "Can't use 'L' with 'W', 'N' or 'Z' modifiers");
   assert(HowLong <= 2 && "Can't have  modifier");
   ++HowLong;
   break;
 case 'N':
   // 'N' behaves like 'L' for all non LP64 targets and 'int' otherwise.
-  assert(!IsSpecialLong && "Can't use two 'N' or 'W' modifiers!");
+  assert(!IsSpecial && "Can't use two 'N', 'W' or 'Z' modifiers!");
   assert(HowLong == 0 && "Can't use both 'L' and 'N' modifiers!");
   #ifndef NDEBUG
-  IsSpecialLong = true;
+  IsSpecial = true;
   #endif
   if (Context.getTargetInfo().getLongWidth() == 32)
 ++HowLong;
   break;
 case 'W':
   // This modifier represents int64 type.
-  assert(!IsSpecialLong && "Can't use two 'N' or 'W' modifiers!");
+  assert(!IsSpecial && "Can't use two 'N', 'W' or 'Z'  modifiers!");
   assert(HowLong == 0 && "Can't use both 'L' and 'W' modifiers!");
   #ifndef NDEBUG
-  IsSpecialLong = true;
+  IsSpecial = true;
   #endif
   switch (Context.getTargetInfo().getInt64Type()) {
   default:
@@ -9337,6 +9337,27 @@
 break;
   }
   break;
+case 'Z':
+  // This modifier represents int32 type.
+  assert(!IsSpecial && "Can't use two 'N', 'W' or 'Z' modifiers!");
+  assert(HowLong == 0 && "Can't use both 'L' and 'Z' modifiers!");
+  #ifndef NDEBUG
+  IsSpecial = true;
+  #endif
+  switch (Context.getTargetInfo().getIntTypeByWidth(32, true)) {
+  default:
+llvm_unreachable("Unexpected integer type");
+  case TargetInfo::SignedInt:
+HowLong = 0;
+break;
+  case TargetInfo::SignedLong:
+HowLong = 1;
+break;
+  case TargetInfo::SignedLongLong:
+HowLong = 2;
+break;
+  }
+  break;
 }
   }
 
Index: cfe/trunk/include/clang/Basic/Builtins.def
===
--- cfe/trunk/include/clang/Basic/Builtins.def
+++ cfe/trunk/include/clang/Basic/Builtins.def
@@ -50,7 +50,8 @@
 //  L   -> long (e.g. Li for 'long int', Ld for 'long double')
 //  LL  -> long long (e.g. LLi for 'long long int', LLd for __float128)
 //  LLL -> __int128_t (e.g. LLLi)
-//  W   -> int64_t
+//  Z   -> int32_t (require a native 32-bit integer type on the target)
+//  W   -> int64_t (require a native 64-bit integer type on the target)
 //  N   -> 'int' size if target is LP64, 'L' otherwise.
 //  S   -> signed
 //  U   -> unsigned
@@ -418,25 +419,27 @@
 BUILTIN(__builtin_clrsbl , "iLi" , "nc")
 BUILTIN(__builtin_clrsbll, "iLLi", "nc")
 
-// FIXME: These type signatures are not correct for targets with int != 32-bits
-// or with ULL != 64-bits.
+// The following builtins rely on that char == 8 bits, short == 16 bits and that
+// there exists native types on the target that are 32- and 64-bits wide, unless
+// these conditions are fulfilled these builtins will operate on a not intended
+// bitwidth.
 BUILTIN(__builtin_bswap16, "UsUs", "nc")
-BUILTIN(__builtin_bswap32, "UiUi", "nc")
-BUILTIN(__builtin_bswap64, "ULLiULLi", "nc")
+BUILTIN(__builtin_bswap32, "UZiUZi", "nc")
+BUILTIN(__builtin_bswap64, "UWiUWi", "nc")
 
 BUILTIN(__builtin_bitreverse8, "UcUc", "nc")
 BUILTIN(__builtin_bitreverse16, "UsUs", "nc")
-BUILTIN(__builtin_bitreverse32, "UiUi", "nc")
-BUILTIN(__builtin_bitreverse64, "ULLiULLi", "nc")
+BUILTIN(__builtin_bitreverse32, "UZiUZi", "nc")
+BUILTIN(__builtin_bitreverse64, "UWiUWi", "nc")
 
 BUILTIN(__builtin_rotateleft8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateleft16, "UsUsUs", "nc")
-BUILTIN(__builtin_rotateleft32, "UiUiUi", "nc")
-BUILTIN(__builtin_rotateleft64, "ULLiULLiULLi", "nc")
+BUILTIN(__builtin_rotateleft32, "UZiUZiUZi", "nc")
+BUILTIN(__builtin_rotateleft64, "UWiUWiUWi", "nc")
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, 

[PATCH] D62580: [OpenCL] Use long instead of long long in x86 builtins

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

What about a testcase? It shouldn't be hard to add a small testcase that 
demonstrate that the changed builtins now work when compiling OpenCL C code for 
x86. I don't think you have to add all changed builtins to the testcase (but a 
few to demonstrate the change).


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

https://reviews.llvm.org/D62580



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

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

Ping?

Do you think this small lit testcase below work as a testcase for the 
-ffile-macro-prefix-to-remove option?

// RUN: %clang_cc1 -emit-llvm %s  -o - -ffile-macro-prefix-to-remove=%s | 
FileCheck %s
char this_file[] = __FILE__;
// CHECK: @this_file = global [1 x i8] zeroinitializer


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

https://reviews.llvm.org/D17741



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

[PATCH] D17741: adds __FILE_BASENAME__ builtin macro

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

Ping?


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

https://reviews.llvm.org/D17741



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

[PATCH] D67606: Change signature of __builtin_rotateright64 back to unsigned

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka created this revision.
Ka-Ka added a reviewer: efriedma.
Herald added a subscriber: kristina.
Herald added a project: clang.

The signature of __builtin_rotateright64 was by misstake changed from
unsigned to signed in r360863, this patch will change it back to
unsigned as intended.

This fixes pr43309


Repository:
  rC Clang

https://reviews.llvm.org/D67606

Files:
  include/clang/Basic/Builtins.def
  test/AST/builtins.c


Index: test/AST/builtins.c
===
--- /dev/null
+++ test/AST/builtins.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s
+
+unsigned char rotright8(unsigned char x, unsigned char y) {
+return __builtin_rotateright8(x, y);
+}
+// CHECK: __builtin_rotateright8 'unsigned char (unsigned char, unsigned char)'
+
+unsigned int rotright16(unsigned int x, unsigned int y) {
+return __builtin_rotateright16(x, y);
+}
+// CHECK: __builtin_rotateright16 'unsigned short (unsigned short, unsigned 
short)'
+
+unsigned long rotright32(unsigned long x, unsigned long y) {
+return __builtin_rotateright32(x, y);
+}
+// CHECK: __builtin_rotateright32 'unsigned int (unsigned int, unsigned int)'
+
+unsigned long long rotright64(unsigned long long x, unsigned long long y) {
+return __builtin_rotateright64(x, y);
+}
+// This verifies pr43309
+// CHECK: __builtin_rotateright64 'unsigned long (unsigned long, unsigned 
long)'
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -461,7 +461,7 @@
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, "UsUsUs", "nc")
 BUILTIN(__builtin_rotateright32, "UZiUZiUZi", "nc")
-BUILTIN(__builtin_rotateright64, "UWiUWiWi", "nc")
+BUILTIN(__builtin_rotateright64, "UWiUWiUWi", "nc")
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")


Index: test/AST/builtins.c
===
--- /dev/null
+++ test/AST/builtins.c
@@ -0,0 +1,22 @@
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -ast-dump %s | FileCheck %s
+
+unsigned char rotright8(unsigned char x, unsigned char y) {
+return __builtin_rotateright8(x, y);
+}
+// CHECK: __builtin_rotateright8 'unsigned char (unsigned char, unsigned char)'
+
+unsigned int rotright16(unsigned int x, unsigned int y) {
+return __builtin_rotateright16(x, y);
+}
+// CHECK: __builtin_rotateright16 'unsigned short (unsigned short, unsigned short)'
+
+unsigned long rotright32(unsigned long x, unsigned long y) {
+return __builtin_rotateright32(x, y);
+}
+// CHECK: __builtin_rotateright32 'unsigned int (unsigned int, unsigned int)'
+
+unsigned long long rotright64(unsigned long long x, unsigned long long y) {
+return __builtin_rotateright64(x, y);
+}
+// This verifies pr43309
+// CHECK: __builtin_rotateright64 'unsigned long (unsigned long, unsigned long)'
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -461,7 +461,7 @@
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, "UsUsUs", "nc")
 BUILTIN(__builtin_rotateright32, "UZiUZiUZi", "nc")
-BUILTIN(__builtin_rotateright64, "UWiUWiWi", "nc")
+BUILTIN(__builtin_rotateright64, "UWiUWiUWi", "nc")
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67606: Change signature of __builtin_rotateright64 back to unsigned

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka updated this revision to Diff 220294.
Ka-Ka added a comment.
Herald added subscribers: Jim, dylanmckay.

Update testcases according to review comment.


Repository:
  rC Clang

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

https://reviews.llvm.org/D67606

Files:
  include/clang/Basic/Builtins.def
  test/CodeGen/avr-builtins.c


Index: test/CodeGen/avr-builtins.c
===
--- test/CodeGen/avr-builtins.c
+++ test/CodeGen/avr-builtins.c
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck 
%s
 
+// Check that the parameter types match.
+// RUN: %clang_cc1 -triple avr-unknown-unknown -Wconversion -verify %s
+// expected-no-diagnostics
+
 unsigned char bitrev8(unsigned char data) {
 return __builtin_bitreverse8(data);
 }
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -461,7 +461,7 @@
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, "UsUsUs", "nc")
 BUILTIN(__builtin_rotateright32, "UZiUZiUZi", "nc")
-BUILTIN(__builtin_rotateright64, "UWiUWiWi", "nc")
+BUILTIN(__builtin_rotateright64, "UWiUWiUWi", "nc")
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")


Index: test/CodeGen/avr-builtins.c
===
--- test/CodeGen/avr-builtins.c
+++ test/CodeGen/avr-builtins.c
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
 
+// Check that the parameter types match.
+// RUN: %clang_cc1 -triple avr-unknown-unknown -Wconversion -verify %s
+// expected-no-diagnostics
+
 unsigned char bitrev8(unsigned char data) {
 return __builtin_bitreverse8(data);
 }
Index: include/clang/Basic/Builtins.def
===
--- include/clang/Basic/Builtins.def
+++ include/clang/Basic/Builtins.def
@@ -461,7 +461,7 @@
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, "UsUsUs", "nc")
 BUILTIN(__builtin_rotateright32, "UZiUZiUZi", "nc")
-BUILTIN(__builtin_rotateright64, "UWiUWiWi", "nc")
+BUILTIN(__builtin_rotateright64, "UWiUWiUWi", "nc")
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D67606: Change signature of __builtin_rotateright64 back to unsigned

[Karl-Johan Karlsson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL371969: Change signature of __builtin_rotateright64 back to 
unsigned (authored by karka, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D67606?vs=220294&id=220300#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D67606

Files:
  cfe/trunk/include/clang/Basic/Builtins.def
  cfe/trunk/test/CodeGen/avr-builtins.c


Index: cfe/trunk/include/clang/Basic/Builtins.def
===
--- cfe/trunk/include/clang/Basic/Builtins.def
+++ cfe/trunk/include/clang/Basic/Builtins.def
@@ -461,7 +461,7 @@
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, "UsUsUs", "nc")
 BUILTIN(__builtin_rotateright32, "UZiUZiUZi", "nc")
-BUILTIN(__builtin_rotateright64, "UWiUWiWi", "nc")
+BUILTIN(__builtin_rotateright64, "UWiUWiUWi", "nc")
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")
Index: cfe/trunk/test/CodeGen/avr-builtins.c
===
--- cfe/trunk/test/CodeGen/avr-builtins.c
+++ cfe/trunk/test/CodeGen/avr-builtins.c
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck 
%s
 
+// Check that the parameter types match. This verifies pr43309.
+// RUN: %clang_cc1 -triple avr-unknown-unknown -Wconversion -verify %s
+// expected-no-diagnostics
+
 unsigned char bitrev8(unsigned char data) {
 return __builtin_bitreverse8(data);
 }


Index: cfe/trunk/include/clang/Basic/Builtins.def
===
--- cfe/trunk/include/clang/Basic/Builtins.def
+++ cfe/trunk/include/clang/Basic/Builtins.def
@@ -461,7 +461,7 @@
 BUILTIN(__builtin_rotateright8, "UcUcUc", "nc")
 BUILTIN(__builtin_rotateright16, "UsUsUs", "nc")
 BUILTIN(__builtin_rotateright32, "UZiUZiUZi", "nc")
-BUILTIN(__builtin_rotateright64, "UWiUWiWi", "nc")
+BUILTIN(__builtin_rotateright64, "UWiUWiUWi", "nc")
 
 // Random GCC builtins
 BUILTIN(__builtin_constant_p, "i.", "nctu")
Index: cfe/trunk/test/CodeGen/avr-builtins.c
===
--- cfe/trunk/test/CodeGen/avr-builtins.c
+++ cfe/trunk/test/CodeGen/avr-builtins.c
@@ -1,5 +1,9 @@
 // RUN: %clang_cc1 -triple avr-unknown-unknown -emit-llvm -o - %s | FileCheck %s
 
+// Check that the parameter types match. This verifies pr43309.
+// RUN: %clang_cc1 -triple avr-unknown-unknown -Wconversion -verify %s
+// expected-no-diagnostics
+
 unsigned char bitrev8(unsigned char data) {
 return __builtin_bitreverse8(data);
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94640: adds more checks to -Wfree-nonheap-object

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

This patch seems to introduce warnings for the case

#include 
#include 
void foo(void* ptr)
{

  free((void*)(intptr_t) ptr);

}

something that gcc don't warn for (see https://godbolt.org/z/1WT9c6 ).

As intptr_t is suppose to be used to pass pointers in I think its a bit strange 
that clang warns in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94640

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

[PATCH] D94640: adds more checks to -Wfree-nonheap-object

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

Thanks for the information. I found the review now in 
https://reviews.llvm.org/D97512


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94640

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

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka created this revision.
Ka-Ka added reviewers: rsmith, hans, rnk, ikudrin.
Herald added a project: clang.

In the current implementation of clang the canonicalization of paths in
diagnostic messages (when using -fdiagnostics-absolute-paths) only works
if the symbolic link is in the directory part of the filename, not if
the file itself is a symbolic link to another file.

This patch adds support to canonicalize the complete path including the
file.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70527

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-symlinks.c

Index: clang/test/Frontend/absolute-paths-symlinks.c
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-symlinks.c
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: cp "%s" "test.c"
+// RUN: ln -sf "test.c" "link.c"
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-absolute-paths "link.c" 2>&1|FileCheck %s
+
+// Verify that -fdiagnostics-absolute-paths resolve symbolic links in
+// diagnostics messages.
+
+// CHECK: test.c
+// CHECK-SAME: error: unknown type name
+This do not compile
+
+// REQUIRES: shell
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -761,11 +761,12 @@
 }
 
 void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
-  SmallVector AbsoluteFilename;
+#ifdef _WIN32
+  SmallString<4096> TmpFilename;
+#endif
   if (DiagOpts->AbsolutePath) {
-auto Dir = SM.getFileManager().getDirectory(
-llvm::sys::path::parent_path(Filename));
-if (Dir) {
+auto File = SM.getFileManager().getFile(Filename);
+if (File) {
   // We want to print a simplified absolute path, i. e. without "dots".
   //
   // The hardest part here are the paths like "//../".
@@ -781,16 +782,14 @@
   // on Windows we can just use llvm::sys::path::remove_dots(), because,
   // on that system, both aforementioned paths point to the same place.
 #ifdef _WIN32
-  SmallString<4096> DirName = (*Dir)->getName();
-  llvm::sys::fs::make_absolute(DirName);
-  llvm::sys::path::native(DirName);
-  llvm::sys::path::remove_dots(DirName, /* remove_dot_dot */ true);
+  TmpFilename = (*File)->getName();
+  llvm::sys::fs::make_absolute(TmpFilename);
+  llvm::sys::path::native(TmpFilename);
+  llvm::sys::path::remove_dots(TmpFilename, /* remove_dot_dot */ true);
+  Filename = StringRef(TmpFilename.data(), TmpFilename.size());
 #else
-  StringRef DirName = SM.getFileManager().getCanonicalName(*Dir);
+  Filename = SM.getFileManager().getCanonicalName(*File);
 #endif
-  llvm::sys::path::append(AbsoluteFilename, DirName,
-  llvm::sys::path::filename(Filename));
-  Filename = StringRef(AbsoluteFilename.data(), AbsoluteFilename.size());
 }
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -564,6 +564,23 @@
   return CanonicalName;
 }
 
+StringRef FileManager::getCanonicalName(const FileEntry *File) {
+  // FIXME: use llvm::sys::fs::canonical() when it gets implemented
+  llvm::DenseMap::iterator Known
+= CanonicalFileNames.find(File);
+  if (Known != CanonicalFileNames.end())
+return Known->second;
+
+  StringRef CanonicalName(File->getName());
+
+  SmallString<4096> CanonicalNameBuf;
+  if (!FS->getRealPath(File->getName(), CanonicalNameBuf))
+CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+
+  CanonicalFileNames.insert({File, CanonicalName});
+  return CanonicalName;
+}
+
 void FileManager::PrintStats() const {
   llvm::errs() << "\n*** File Manager Stats:\n";
   llvm::errs() << UniqueRealFiles.size() << " real files found, "
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -225,6 +225,9 @@
   /// The canonical names of directories.
   llvm::DenseMap CanonicalDirNames;
 
+  /// The canonical names of files.
+  llvm::DenseMap CanonicalFileNames;
+
   /// Storage for canonical names that we have computed.
   llvm::BumpPtrAllocator CanonicalNameStorage;
 
@@ -421,6 +424,13 @@
   /// required, which is (almost) never.
   StringRef getCanonicalName(const DirectoryEntry *Dir);
 
+  /// Retrieve the canonical name for a given file.
+  ///
+  /// This is a very expensive operation, despite its results being cached,
+  /// and should only be used when the physical layout of the file system is
+  /// required, which is (almost) never.
+  StringRef getCan

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

Ping


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70527



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

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added a comment.

In D70527#1786718 , @rnk wrote:

> In D70527#1785552 , @ikudrin wrote:
>
> > Personally, I would prefer to see the file name and path to be changed as 
> > little as possible because that would help to recognize the files better. 
> > We cannot use `remove_dots()` on POSIX OSes to simplify paths, because it 
> > may return an invalid path; thus we have to use `getRealPath()`. If I 
> > understand it right, there is no similar problem with the file name itself.
> >
> > So, which issues this patch is going to solve?
>
>
> It seems clear to me, the filename could be an absolute symlink to a real 
> file somewhere far removed from the realpath of the parent directory. It 
> seems reasonable that -fdiagnostics-absolute-paths would look through 
> symlinks in this case.


This fix is important when building with Bazel (google build system) as Bazel 
set up a temporary sandbox (with a series of symbolic links) to be be able to 
control the dependencies and keep a valid build cache. Currently when using the 
clang option -fdiagnostics-absolute-paths it will only resolve the sybolic 
names in the dir-path of the filename which end up with warning- and 
error-messages to files inside the sandbox instead of the correct source inside 
the repo you are working in.

In D70527#1786698 , @rnk wrote:

> From auditing the call sites, it seems that almost all of them could be 
> simplified by using the new API:
>  
> http://llvm-cs.pcc.me.uk/tools/clang/lib/Basic/FileManager.cpp/rgetCanonicalName


I agree, that they can be simplified, but I guess it is easier to do those 
changes in separate patches.




Comment at: clang/include/clang/Basic/FileManager.h:226
   /// The canonical names of directories.
   llvm::DenseMap CanonicalDirNames;
 

rnk wrote:
> You could make the key `void*` and save a DenseMap here. Nobody ever iterates 
> CanonicalDirNames to look at the keys.
Sure, I will update the patch ...



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70527



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

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka updated this revision to Diff 234268.
Ka-Ka marked 2 inline comments as done.
Ka-Ka added a comment.

Updated patch according to review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70527

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-symlinks.c

Index: clang/test/Frontend/absolute-paths-symlinks.c
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-symlinks.c
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: cp "%s" "test.c"
+// RUN: ln -sf "test.c" "link.c"
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-absolute-paths "link.c" 2>&1|FileCheck %s
+
+// Verify that -fdiagnostics-absolute-paths resolve symbolic links in
+// diagnostics messages.
+
+// CHECK: test.c
+// CHECK-SAME: error: unknown type name
+This do not compile
+
+// REQUIRES: shell
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -761,11 +761,12 @@
 }
 
 void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
-  SmallVector AbsoluteFilename;
+#ifdef _WIN32
+  SmallString<4096> TmpFilename;
+#endif
   if (DiagOpts->AbsolutePath) {
-auto Dir = SM.getFileManager().getDirectory(
-llvm::sys::path::parent_path(Filename));
-if (Dir) {
+auto File = SM.getFileManager().getFile(Filename);
+if (File) {
   // We want to print a simplified absolute path, i. e. without "dots".
   //
   // The hardest part here are the paths like "//../".
@@ -781,16 +782,14 @@
   // on Windows we can just use llvm::sys::path::remove_dots(), because,
   // on that system, both aforementioned paths point to the same place.
 #ifdef _WIN32
-  SmallString<4096> DirName = (*Dir)->getName();
-  llvm::sys::fs::make_absolute(DirName);
-  llvm::sys::path::native(DirName);
-  llvm::sys::path::remove_dots(DirName, /* remove_dot_dot */ true);
+  TmpFilename = (*File)->getName();
+  llvm::sys::fs::make_absolute(TmpFilename);
+  llvm::sys::path::native(TmpFilename);
+  llvm::sys::path::remove_dots(TmpFilename, /* remove_dot_dot */ true);
+  Filename = StringRef(TmpFilename.data(), TmpFilename.size());
 #else
-  StringRef DirName = SM.getFileManager().getCanonicalName(*Dir);
+  Filename = SM.getFileManager().getCanonicalName(*File);
 #endif
-  llvm::sys::path::append(AbsoluteFilename, DirName,
-  llvm::sys::path::filename(Filename));
-  Filename = StringRef(AbsoluteFilename.data(), AbsoluteFilename.size());
 }
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -549,9 +549,9 @@
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
   // FIXME: use llvm::sys::fs::canonical() when it gets implemented
-  llvm::DenseMap::iterator Known
-= CanonicalDirNames.find(Dir);
-  if (Known != CanonicalDirNames.end())
+  llvm::DenseMap::iterator Known
+= CanonicalNames.find(Dir);
+  if (Known != CanonicalNames.end())
 return Known->second;
 
   StringRef CanonicalName(Dir->getName());
@@ -560,7 +560,24 @@
   if (!FS->getRealPath(Dir->getName(), CanonicalNameBuf))
 CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
 
-  CanonicalDirNames.insert({Dir, CanonicalName});
+  CanonicalNames.insert({Dir, CanonicalName});
+  return CanonicalName;
+}
+
+StringRef FileManager::getCanonicalName(const FileEntry *File) {
+  // FIXME: use llvm::sys::fs::canonical() when it gets implemented
+  llvm::DenseMap::iterator Known
+= CanonicalNames.find(File);
+  if (Known != CanonicalNames.end())
+return Known->second;
+
+  StringRef CanonicalName(File->getName());
+
+  SmallString<4096> CanonicalNameBuf;
+  if (!FS->getRealPath(File->getName(), CanonicalNameBuf))
+CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+
+  CanonicalNames.insert({File, CanonicalName});
   return CanonicalName;
 }
 
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -222,8 +222,8 @@
   llvm::BumpPtrAllocator>
   SeenFileEntries;
 
-  /// The canonical names of directories.
-  llvm::DenseMap CanonicalDirNames;
+  /// The canonical names of files and directories .
+  llvm::DenseMap CanonicalNames;
 
   /// Storage for canonical names that we have computed.
   llvm::BumpPtrAllocator CanonicalNameStorage;
@@ -421,6 +421,13 @@
   /// required, which

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Ka-Ka marked 2 inline comments as done.
Closed by commit rGe8efac4b1530: [clang] Fix the canonicalization of paths in 
-fdiagnostics-absolute-paths (authored by Ka-Ka).

Changed prior to commit:
  https://reviews.llvm.org/D70527?vs=234268&id=234832#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70527

Files:
  clang/include/clang/Basic/FileManager.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/test/Frontend/absolute-paths-symlinks.c

Index: clang/test/Frontend/absolute-paths-symlinks.c
===
--- /dev/null
+++ clang/test/Frontend/absolute-paths-symlinks.c
@@ -0,0 +1,15 @@
+// RUN: rm -rf %t
+// RUN: mkdir %t
+// RUN: cd %t
+// RUN: cp "%s" "test.c"
+// RUN: ln -sf "test.c" "link.c"
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-absolute-paths "link.c" 2>&1|FileCheck %s
+
+// Verify that -fdiagnostics-absolute-paths resolve symbolic links in
+// diagnostics messages.
+
+// CHECK: test.c
+// CHECK-SAME: error: unknown type name
+This do not compile
+
+// REQUIRES: shell
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -761,11 +761,12 @@
 }
 
 void TextDiagnostic::emitFilename(StringRef Filename, const SourceManager &SM) {
-  SmallVector AbsoluteFilename;
+#ifdef _WIN32
+  SmallString<4096> TmpFilename;
+#endif
   if (DiagOpts->AbsolutePath) {
-auto Dir = SM.getFileManager().getDirectory(
-llvm::sys::path::parent_path(Filename));
-if (Dir) {
+auto File = SM.getFileManager().getFile(Filename);
+if (File) {
   // We want to print a simplified absolute path, i. e. without "dots".
   //
   // The hardest part here are the paths like "//../".
@@ -781,16 +782,14 @@
   // on Windows we can just use llvm::sys::path::remove_dots(), because,
   // on that system, both aforementioned paths point to the same place.
 #ifdef _WIN32
-  SmallString<4096> DirName = (*Dir)->getName();
-  llvm::sys::fs::make_absolute(DirName);
-  llvm::sys::path::native(DirName);
-  llvm::sys::path::remove_dots(DirName, /* remove_dot_dot */ true);
+  TmpFilename = (*File)->getName();
+  llvm::sys::fs::make_absolute(TmpFilename);
+  llvm::sys::path::native(TmpFilename);
+  llvm::sys::path::remove_dots(TmpFilename, /* remove_dot_dot */ true);
+  Filename = StringRef(TmpFilename.data(), TmpFilename.size());
 #else
-  StringRef DirName = SM.getFileManager().getCanonicalName(*Dir);
+  Filename = SM.getFileManager().getCanonicalName(*File);
 #endif
-  llvm::sys::path::append(AbsoluteFilename, DirName,
-  llvm::sys::path::filename(Filename));
-  Filename = StringRef(AbsoluteFilename.data(), AbsoluteFilename.size());
 }
   }
 
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -548,10 +548,9 @@
 }
 
 StringRef FileManager::getCanonicalName(const DirectoryEntry *Dir) {
-  // FIXME: use llvm::sys::fs::canonical() when it gets implemented
-  llvm::DenseMap::iterator Known
-= CanonicalDirNames.find(Dir);
-  if (Known != CanonicalDirNames.end())
+  llvm::DenseMap::iterator Known
+= CanonicalNames.find(Dir);
+  if (Known != CanonicalNames.end())
 return Known->second;
 
   StringRef CanonicalName(Dir->getName());
@@ -560,7 +559,23 @@
   if (!FS->getRealPath(Dir->getName(), CanonicalNameBuf))
 CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
 
-  CanonicalDirNames.insert({Dir, CanonicalName});
+  CanonicalNames.insert({Dir, CanonicalName});
+  return CanonicalName;
+}
+
+StringRef FileManager::getCanonicalName(const FileEntry *File) {
+  llvm::DenseMap::iterator Known
+= CanonicalNames.find(File);
+  if (Known != CanonicalNames.end())
+return Known->second;
+
+  StringRef CanonicalName(File->getName());
+
+  SmallString<4096> CanonicalNameBuf;
+  if (!FS->getRealPath(File->getName(), CanonicalNameBuf))
+CanonicalName = StringRef(CanonicalNameBuf).copy(CanonicalNameStorage);
+
+  CanonicalNames.insert({File, CanonicalName});
   return CanonicalName;
 }
 
Index: clang/include/clang/Basic/FileManager.h
===
--- clang/include/clang/Basic/FileManager.h
+++ clang/include/clang/Basic/FileManager.h
@@ -222,8 +222,8 @@
   llvm::BumpPtrAllocator>
   SeenFileEntries;
 
-  /// The canonical names of directories.
-  llvm::DenseMap CanonicalDirNames;
+  /// The canonical names of files and directories .
+  llvm::DenseMap CanonicalNames;
 
   /// Storage for canonical names that 

[PATCH] D64573: [Syntax] Allow to mutate syntax trees

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka added inline comments.



Comment at: clang/lib/Tooling/Syntax/Mutations.cpp:74
+
+  if (auto *Parent = llvm::dyn_cast(S->parent())) {
+// A child of CompoundStatement can just be safely removed.

This line introduce a buildbot fail:

/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/lib/Tooling/Syntax/Mutations.cpp:74:13:
 warning: unused variable ‘Parent’ [-Wunused-variable]




Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:70
   assert(F.file() == L.file() && "tokens from different files");
-  assert(F.endOffset() <= L.beginOffset() && "wrong order of tokens");
+  assert(F == L || F.endOffset() <= L.beginOffset() && "wrong order of 
tokens");
   return FileRange(F.file(), F.beginOffset(), L.endOffset());

This line introduce a buildbot failure:

/home/buildbots/ppc64be-clang-multistage-test/clang-ppc64be-multistage/llvm/clang/lib/Tooling/Syntax/Tokens.cpp:70:53:
 warning: suggest parentheses around ‘&&’ within ‘||’ [-Wparentheses]



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64573



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

[PATCH] D70527: [clang] Fix the canonicalization of paths in -fdiagnostics-absolute-paths

[Karl-Johan Karlsson via Phabricator via cfe-commits]

Ka-Ka marked 3 inline comments as done.
Ka-Ka added a comment.

I have now updated the testcase according to comments by @MaskRay in commit 
073cdb239044 

Thanks for post-commit review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70527



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