Re: [clang-tools-extra] r322387 - [clangd] Code completion uses Sema for NS-level things in the current file.

[Mikhail Zolotukhin via cfe-commits]

Hi,

ClangdTests/FuzzyMatch.Matches has been failing since this commit on a 
sanitized clang build on MacOS. I've seen some later commits trying to fix it, 
but the issue is still there. Could you please take a look?

Steps to reproduce in case you need them:
> cmake -G Ninja  -DLIBCXX_SYSROOT=/Path/To/MacOSX.sdk 
> -DCOMPILER_RT_BUILD_BUILTINS=Off -DCMAKE_BUILD_TYPE=Release 
> -DLLVM_ENABLE_ASSERTIONS=Off -DLLVM_USE_SANITIZER="Address;Undefined" 
> -DLLVM_BUILD_RUNTIME=NO /Path/To/llvm
> ninja && ninja tools/clang/tools/extra/unittests/clangd/ClangdTests
> tools/clang/tools/extra/unittests/clangd/ClangdTests
[==] Running 90 tests from 19 test cases.
[--] Global test environment set-up.
[--] 8 tests from ClangdVFSTest
[ RUN  ] ClangdVFSTest.Parse
[   OK ] ClangdVFSTest.Parse (84 ms)
[ RUN  ] ClangdVFSTest.ParseWithHeader
[   OK ] ClangdVFSTest.ParseWithHeader (98 ms)
[ RUN  ] ClangdVFSTest.Reparse
[   OK ] ClangdVFSTest.Reparse (49 ms)
[ RUN  ] ClangdVFSTest.ReparseOnHeaderChange
[   OK ] ClangdVFSTest.ReparseOnHeaderChange (65 ms)
[ RUN  ] ClangdVFSTest.CheckVersions
.../tools/extra/clangd/FuzzyMatch.cpp:78:27: runtime error: division by zero
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
.../tools/extra/clangd/FuzzyMatch.cpp:78:27 in
Abort trap: 6

Thanks,
Michael

> On Jan 12, 2018, at 10:30 AM, Sam McCall via cfe-commits 
>  wrote:
> 
> Author: sammccall
> Date: Fri Jan 12 10:30:08 2018
> New Revision: 322387
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=322387&view=rev
> Log:
> [clangd] Code completion uses Sema for NS-level things in the current file.
> 
> Summary:
> To stay fast, it avoids deserializing anything outside the current file, by
> disabling the LoadExternal code completion option added in r322377, when the
> index is enabled.
> 
> Reviewers: hokein
> 
> Subscribers: klimek, ilya-biryukov, cfe-commits
> 
> Differential Revision: https://reviews.llvm.org/D41996
> 
> Modified:
>clang-tools-extra/trunk/clangd/CodeComplete.cpp
>clang-tools-extra/trunk/clangd/index/FileIndex.cpp
>clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> 
> Modified: clang-tools-extra/trunk/clangd/CodeComplete.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/CodeComplete.cpp?rev=322387&r1=322386&r2=322387&view=diff
> ==
> --- clang-tools-extra/trunk/clangd/CodeComplete.cpp (original)
> +++ clang-tools-extra/trunk/clangd/CodeComplete.cpp Fri Jan 12 10:30:08 2018
> @@ -642,8 +642,10 @@ clang::CodeCompleteOptions CodeCompleteO
>   Result.IncludeGlobals = IncludeGlobals;
>   Result.IncludeBriefComments = IncludeBriefComments;
> 
> -  // Enable index-based code completion when Index is provided.
> -  Result.IncludeNamespaceLevelDecls = !Index;
> +  // When an is used, Sema is responsible for completing the main file,
> +  // the index can provide results from the preamble.
> +  // Tell Sema not to deserialize the preamble to look for results.
> +  Result.LoadExternal = !Index;
> 
>   return Result;
> }
> 
> Modified: clang-tools-extra/trunk/clangd/index/FileIndex.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/FileIndex.cpp?rev=322387&r1=322386&r2=322387&view=diff
> ==
> --- clang-tools-extra/trunk/clangd/index/FileIndex.cpp (original)
> +++ clang-tools-extra/trunk/clangd/index/FileIndex.cpp Fri Jan 12 10:30:08 
> 2018
> @@ -20,6 +20,8 @@ std::unique_ptr indexAST(AST
>  std::shared_ptr PP,
>  llvm::ArrayRef Decls) {
>   SymbolCollector::Options CollectorOpts;
> +  // Code completion gets main-file results from Sema.
> +  // But we leave this option on because features like go-to-definition want 
> it.
>   CollectorOpts.IndexMainFiles = true;
>   auto Collector = 
> std::make_shared(std::move(CollectorOpts));
>   Collector->setPreprocessor(std::move(PP));
> 
> Modified: clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp?rev=322387&r1=322386&r2=322387&view=diff
> ==
> --- clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp (original)
> +++ clang-tools-extra/trunk/unittests/clangd/CodeCompleteTests.cpp Fri Jan 12 
> 10:30:08 2018
> @@ -57,6 +57,7 @@ using ::testing::Contains;
> using ::testing::Each;
> using ::testing::ElementsAre;
> using ::testing::Not;
> +using ::testing::UnorderedElementsAre;
> 
> class IgnoreDiagnostics : public DiagnosticsConsumer {
>   void
> @@ -104,7 +105,7 @@ CompletionList completions(StringRef Tex
>   /*StorePreamblesInMemory=*/true);
>   auto File = getVirtualTestFilePath("foo.cpp");
>

Re: r322769 - [RISCV] Propagate -mabi and -march values to GNU assembler.

[Mikhail Zolotukhin via cfe-commits]

Hi,

Looks like the test is failing on MacOS [1]. Could you please take a look?

Thanks,
Michael

[1] 
http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/45726/consoleFull#9955924438254eaf0-7326-4999-85b0-388101f2d404
 



> On Jan 17, 2018, at 2:09 PM, Ana Pazos via cfe-commits 
>  wrote:
> 
> Author: apazos
> Date: Wed Jan 17 14:09:58 2018
> New Revision: 322769
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=322769&view=rev
> Log:
> [RISCV] Propagate -mabi and -march values to GNU assembler.
> 
> When using -fno-integrated-as flag, the gnu assembler produces code
> with some default march/mabi which later causes linker failure due
> to incompatible mabi/march.
> 
> In this patch we explicitly propagate -mabi and -march flags to the
> GNU assembler.
> 
> In this patch we explicitly propagate -mabi and -march flags to the GNU 
> assembler.
> 
> Differential Revision: https://reviews.llvm.org/D41271
> 
> Added:
>
> cfe/trunk/test/Driver/Inputs/multilib_riscv_linux_sdk/riscv64-unknown-linux-gnu/bin/as
>cfe/trunk/test/Driver/riscv-gnutools.c
> Modified:
>cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
> 
> Modified: cfe/trunk/lib/Driver/ToolChains/Gnu.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Driver/ToolChains/Gnu.cpp?rev=322769&r1=322768&r2=322769&view=diff
> ==
> --- cfe/trunk/lib/Driver/ToolChains/Gnu.cpp (original)
> +++ cfe/trunk/lib/Driver/ToolChains/Gnu.cpp Wed Jan 17 14:09:58 2018
> @@ -629,6 +629,18 @@ void tools::gnutools::Assembler::Constru
>   ppc::getPPCAsmModeForCPU(getCPUName(Args, getToolChain().getTriple(;
> break;
>   }
> +  case llvm::Triple::riscv32:
> +  case llvm::Triple::riscv64: {
> +StringRef ABIName = riscv::getRISCVABI(Args, getToolChain().getTriple());
> +CmdArgs.push_back("-mabi");
> +CmdArgs.push_back(ABIName.data());
> +if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
> +  StringRef MArch = A->getValue();
> +  CmdArgs.push_back("-march");
> +  CmdArgs.push_back(MArch.data());
> +}
> +break;
> +  }
>   case llvm::Triple::sparc:
>   case llvm::Triple::sparcel: {
> CmdArgs.push_back("-32");
> 
> Added: 
> cfe/trunk/test/Driver/Inputs/multilib_riscv_linux_sdk/riscv64-unknown-linux-gnu/bin/as
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/Inputs/multilib_riscv_linux_sdk/riscv64-unknown-linux-gnu/bin/as?rev=322769&view=auto
> ==
> --- 
> cfe/trunk/test/Driver/Inputs/multilib_riscv_linux_sdk/riscv64-unknown-linux-gnu/bin/as
>  (added)
> +++ 
> cfe/trunk/test/Driver/Inputs/multilib_riscv_linux_sdk/riscv64-unknown-linux-gnu/bin/as
>  Wed Jan 17 14:09:58 2018
> @@ -0,0 +1 @@
> +#!/bin/true
> 
> Added: cfe/trunk/test/Driver/riscv-gnutools.c
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Driver/riscv-gnutools.c?rev=322769&view=auto
> ==
> --- cfe/trunk/test/Driver/riscv-gnutools.c (added)
> +++ cfe/trunk/test/Driver/riscv-gnutools.c Wed Jan 17 14:09:58 2018
> @@ -0,0 +1,14 @@
> +// Check gnutools are invoked with propagated values for -mabi and -march.
> +
> +// RUN: %clang -target riscv32-linux-unknown-elf -fno-integrated-as \
> +// RUN: --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
> +// RUN: --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot %s -### \
> +// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32 %s
> +// RUN: %clang -target riscv32-linux-unknown-elf -fno-integrated-as \
> +// RUN: -march=rv32g --gcc-toolchain=%S/Inputs/multilib_riscv_linux_sdk \
> +// RUN: --sysroot=%S/Inputs/multilib_riscv_linux_sdk/sysroot %s -### \
> +// RUN: 2>&1 | FileCheck -check-prefix=MABI-ILP32-MARCH-G %s
> +
> +// MABI-ILP32: 
> "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|}}as"
>  "-mabi" "ilp32"
> +// MABI-ILP32-MARCH-G: 
> "{{.*}}/Inputs/multilib_riscv_linux_sdk/lib/gcc/riscv64-unknown-linux-gnu/7.2.0/../../../../riscv64-unknown-linux-gnu/bin{{/|}}as"
>  "-mabi" "ilp32" "-march" "rv32g"
> +
> 
> 
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

Re: [PATCH] D12785: Document __builtin_nontemporal_load and __builtin_nontemporal_store.

[Mikhail Zolotukhin via cfe-commits]

> On Sep 10, 2015, at 6:29 PM, Richard Smith  wrote:
> 
> On Thu, Sep 10, 2015 at 5:57 PM, Michael Zolotukhin  > wrote:
> mzolotukhin added inline comments.
> 
> 
> Comment at: docs/LanguageExtensions.rst:1802-1807
> @@ +1801,8 @@
> +
> +For example, on AArch64 in the following code::
> +
> +  LDR X1, [X2]
> +  LDNP X3, X4, [X1]
> +
> +the ``LDNP`` might be executed before the ``LDR``. In this case the load 
> would
> +be performed from a wrong address (see 6.3.8 in `Programmer's Guide for 
> ARMv8-A
> 
> rsmith wrote:
> > This seems to make the feature essentially useless, since you cannot 
> > guarantee that the address register is set up sufficiently far before the 
> > non-temporal load. Should the compiler not be required to insert the 
> > necessary barrier itself in this case?
> Yes, we can require targets to only use corresponding NT instructions when 
> it's safe, and then remove this remark from the documentation. For ARM64 that 
> would mean either not to emit LDNP at all, or conservatively emit barriers 
> before each LDNP (which probably removes all performance benefits of using 
> it) - that is, yes, non-temporal loads would be useless on this target.
> 
> I think this should already be the case -- according to the definition of 
> !nontemporal in the LangRef 
> (http://llvm.org/docs/LangRef.html#load-instruction 
> ), using an LDNP without 
> an accompanying barrier would not be correct on AArch64, as it does not have 
> the right semantics.
I removed the paragraph in updated patch.
>  
> But I think we want to keep the builtin for NT-load, as it's a generic 
> feature, not ARM64 specific. It can be used on other targets - e.g. we can 
> use this in x86 stream builtins, and hopefully simplify their current 
> implementation. I don't know about non-temporal operations on other targets, 
> but if there are others, they can use it too right out of the box.
> 
> Yes, I'm not arguing for removing the builtin, just that the AArch64 backend 
> needs to be very careful when mapping it to LDNP, because that will 
> frequently not be correct.
Yes, that's true, and that was the reason why we only implemented STNP for now.

Michael

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

Re: r246877 - Increase accuracy of __builtin_object_size.

[Mikhail Zolotukhin via cfe-commits]

Hi George,

After this commit we started to trap on the following case:

#include 
typedef struct {
  int n;
  char key[1];
} obj_t;

char *str = "hello";

int main() {
  obj_t* p = (obj_t*)malloc(strlen(str) + 1 + sizeof(int));
  strcpy(p->key, str);
  free(p);
  return 0;
}

As far as I understand, this might be a common pattern in pre-C99 codebase, and 
it fails when compiled with -D_FORTIFY_SOURCE=2. Is there a way we could fix it 
in clang, or the only fix is to use less strict FORTIFY_SOURCE level?

Thanks,
Michael

> On Sep 4, 2015, at 2:28 PM, George Burgess IV via cfe-commits 
>  wrote:
> 
> Author: gbiv
> Date: Fri Sep  4 16:28:13 2015
> New Revision: 246877
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=246877&view=rev
> Log:
> Increase accuracy of __builtin_object_size.
> 
> Improvements:
> 
> - For all types, we would give up in a case such as:
>__builtin_object_size((char*)&foo, N);
>  even if we could provide an answer to
>__builtin_object_size(&foo, N);
>  We now provide the same answer for both of the above examples in all
>  cases.
> 
> - For type=1|3, we now support subobjects with unknown bases, as long
>  as the designator is valid.
> 
> Thanks to Richard Smith for the review + design planning.
> 
> Review: http://reviews.llvm.org/D12169
> 
> 
> Modified:
>cfe/trunk/lib/AST/ExprConstant.cpp
>cfe/trunk/test/CodeGen/object-size.c
> 
> Modified: cfe/trunk/lib/AST/ExprConstant.cpp
> URL: 
> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/AST/ExprConstant.cpp?rev=246877&r1=246876&r2=246877&view=diff
> ==
> --- cfe/trunk/lib/AST/ExprConstant.cpp (original)
> +++ cfe/trunk/lib/AST/ExprConstant.cpp Fri Sep  4 16:28:13 2015
> @@ -492,7 +492,11 @@ namespace {
>   /// optimizer if we don't constant fold them here, but in an unevaluated
>   /// context we try to fold them immediately since the optimizer never
>   /// gets a chance to look at it.
> -  EM_PotentialConstantExpressionUnevaluated
> +  EM_PotentialConstantExpressionUnevaluated,
> +
> +  /// Evaluate as a constant expression. Continue evaluating if we find a
> +  /// MemberExpr with a base that can't be evaluated.
> +  EM_DesignatorFold,
> } EvalMode;
> 
> /// Are we checking whether the expression is a potential constant
> @@ -595,6 +599,7 @@ namespace {
>   case EM_PotentialConstantExpression:
>   case EM_ConstantExpressionUnevaluated:
>   case EM_PotentialConstantExpressionUnevaluated:
> +  case EM_DesignatorFold:
> HasActiveDiagnostic = false;
> return OptionalDiagnostic();
>   }
> @@ -674,6 +679,7 @@ namespace {
>   case EM_ConstantExpression:
>   case EM_ConstantExpressionUnevaluated:
>   case EM_ConstantFold:
> +  case EM_DesignatorFold:
> return false;
>   }
>   llvm_unreachable("Missed EvalMode case");
> @@ -702,10 +708,15 @@ namespace {
>   case EM_ConstantExpressionUnevaluated:
>   case EM_ConstantFold:
>   case EM_IgnoreSideEffects:
> +  case EM_DesignatorFold:
> return false;
>   }
>   llvm_unreachable("Missed EvalMode case");
> }
> +
> +bool allowInvalidBaseExpr() const {
> +  return EvalMode == EM_DesignatorFold;
> +}
>   };
> 
>   /// Object used to treat all foldable expressions as constant expressions.
> @@ -736,6 +747,21 @@ namespace {
> }
>   };
> 
> +  /// RAII object used to treat the current evaluation as the correct pointer
> +  /// offset fold for the current EvalMode
> +  struct FoldOffsetRAII {
> +EvalInfo &Info;
> +EvalInfo::EvaluationMode OldMode;
> +explicit FoldOffsetRAII(EvalInfo &Info, bool Subobject)
> +: Info(Info), OldMode(Info.EvalMode) {
> +  if (!Info.checkingPotentialConstantExpression())
> +Info.EvalMode = Subobject ? EvalInfo::EM_DesignatorFold
> +  : EvalInfo::EM_ConstantFold;
> +}
> +
> +~FoldOffsetRAII() { Info.EvalMode = OldMode; }
> +  };
> +
>   /// RAII object used to suppress diagnostics and side-effects from a
>   /// speculative evaluation.
>   class SpeculativeEvaluationRAII {
> @@ -917,7 +943,8 @@ namespace {
>   struct LValue {
> APValue::LValueBase Base;
> CharUnits Offset;
> -unsigned CallIndex;
> +bool InvalidBase : 1;
> +unsigned CallIndex : 31;
> SubobjectDesignator Designator;
> 
> const APValue::LValueBase getLValueBase() const { return Base; }
> @@ -938,17 +965,23 @@ namespace {
>   assert(V.isLValue());
>   Base = V.getLValueBase();
>   Offset = V.getLValueOffset();
> +  InvalidBase = false;
>   CallIndex = V.getLValueCallIndex();
>   Designator = SubobjectDesignator(Ctx, V);
> }
> 
> -void set(APValue::LValueBase B, unsigned I = 0) {
> +void set(APValue::LValueBase B, unsigned I = 0, bool BInvalid = false) {
>   Base = B;
>   Offset = CharUnits::Zero();