[PATCH] D155824: [LoongArch] Support -march=native and -mtune=

[Steven Wu via Phabricator via cfe-commits]

steven_wu accepted this revision.
steven_wu added a comment.

In D155824#4568904 , @xen0n wrote:

> This still LGTM, @steven_wu would you please take another look so this can 
> get re-landed if confirmed working?

The driver part LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155824

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

[PATCH] D156014: [Clang][NVPTX] Permit use of the alias attribute for NVPTX targets

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D156014#4567446 , @jhuber6 wrote:

> In D156014#4567363 , @steven_wu 
> wrote:
>
>> This breaks macOS bot: 
>> https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/36900/testReport/junit/Clang/SemaCUDA/alias_cu/
>
> I should've fixed that already. Is it still broken?

9e99a4f0db0e21b68e9aab9ad6f32f34d42eb460 
 ? That is 
not yet built but looks promising. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156014

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

[PATCH] D156014: [Clang][NVPTX] Permit use of the alias attribute for NVPTX targets

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

This breaks macOS bot: 
https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/36900/testReport/junit/Clang/SemaCUDA/alias_cu/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156014

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

[PATCH] D157119: cmake: add missing dependencies on ClangDriverOptions tablegen

[Steven Wu via Phabricator via cfe-commits]

steven_wu accepted this revision.
steven_wu added subscribers: aprantl, dblaikie.
steven_wu added a comment.
This revision is now accepted and ready to land.

LGTM. @aprantl @dblaikie who cares about module builds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157119

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

[PATCH] D155824: [LoongArch] Support -march=native and -mtune=

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D155824#4548921 , @SixWeining 
wrote:

> In D155824#4548677 , @steven_wu 
> wrote:
>
>> After another look, I will need to request to revert this because this 
>> implementation doesn't work with `-fno-integrated-cc1` at all. You can't 
>> setCPU/Tune in the driver into a global variable and expect that will work 
>> during compilation.
>>
>> You can reproduce with `-fno-integrated-cc1` option or build clang with 
>> cmake option `-DCLANG_SPAWN_CC1=On`.
>
> Thanks. I’ll investigate as soon as possible. BTW, did it break any buildbot?

Yes, I only know this bootstrap job (has CMAKE configuration that sets 
CLANG_SPAWN_CC1). https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto

My local revert is clean and all tests passed. I will just go ahead and revert 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155824

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

[PATCH] D155824: [LoongArch] Support -march=native and -mtune=

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

After another look, I will need to request to revert this because this 
implementation doesn't work with `-fno-integrated-cc1` at all. You can't 
setCPU/Tune in the driver into a global variable and expect that will work 
during compilation.

You can reproduce with `-fno-integrated-cc1` option or build clang with cmake 
option `-DCLANG_SPAWN_CC1=On`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155824

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

[PATCH] D155824: [LoongArch] Support -march=native and -mtune=

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: llvm/lib/TargetParser/LoongArchTargetParser.cpp:19
 
+StringRef Arch;
+StringRef TuneCPU;

Why do we store `Arch` and `TuneCPU` as globals? We should not have as little 
global state in compiler as possible. Any reason why they are not stored in 
`LoongArchTargetInfo` instead like other Targets?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155824

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

[PATCH] D156533: [clang][DeclPrinter] Fix missing semicolon in AST print for methods that are definitions without having a body

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

Reverted in 4098e13a7146 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156533

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

[PATCH] D156533: [clang][DeclPrinter] Fix missing semicolon in AST print for methods that are definitions without having a body

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

  error: aliases are not supported on darwin

Maybe create a different testcase that is not supported on Darwin?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156533

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

[PATCH] D156533: [clang][DeclPrinter] Fix missing semicolon in AST print for methods that are definitions without having a body

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

This breaks Darwin bots: 
https://green.lab.llvm.org/green/job/clang-stage1-RA/35102/testReport/junit/Clang/AST/ast_print_method_decl_cpp/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156533

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

[PATCH] D153906: [clang] Allow disassembly of multi-module bitcode files

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I don't like this doesn't write to the output file by `-o` too. I also think 
the output should just match `llvm-dis` output, instead of splitting into 
multiple files.


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

https://reviews.llvm.org/D153906

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

[PATCH] D154191: [LTO] Replace llvm::writeFileAtomically with llvm::writeToOutput API.

[Steven Wu via Phabricator via cfe-commits]

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

This is generally fine. I don't see any error that should fall into 
`consumeError` condition that won't just error out anyway (like out of disk 
space, etc.) and I don't see those are special cased for code before switching 
to `writeFileAtomically`.




Comment at: llvm/lib/LTO/ThinLTOCodeGenerator.cpp:419
 
-// Write to a temporary to avoid race condition
-SmallString<128> TempFilename;
-SmallString<128> CachePath(EntryPath);
-llvm::sys::path::remove_filename(CachePath);
-sys::path::append(TempFilename, CachePath, "Thin-%%.tmp.o");
-
-if (auto Err = handleErrors(
-llvm::writeFileAtomically(TempFilename, EntryPath,
-  OutputBuffer.getBuffer()),
-[](const llvm::AtomicFileWriteError ) {
-  std::string ErrorMsgBuffer;
-  llvm::raw_string_ostream S(ErrorMsgBuffer);
-  E.log(S);
-
-  if (E.Error ==
-  llvm::atomic_write_error::failed_to_create_uniq_file) {
-errs() << "Error: " << ErrorMsgBuffer << "\n";
-report_fatal_error("ThinLTO: Can't get a temporary file");
-  }
-})) {
-  // FIXME
-  consumeError(std::move(Err));
-}
+if (auto Err = handleErrors(llvm::writeToOutput(
+EntryPath, [](llvm::raw_ostream ) -> llvm::Error {

Nit: Since now we don't care about what kind of error returns, this 
`handleErrors` with no error handle does nothing. Just do:
```
auto Err = llvm::writeToOutput()
```



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154191

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

[PATCH] D154016: [clang][modules] Avoid serializing all diag mappings in non-deterministic order

[Steven Wu via Phabricator via cfe-commits]

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

LGTM


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

https://reviews.llvm.org/D154016

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

[PATCH] D154016: [clang][modules] Avoid serializing all diag mappings in non-deterministic order

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: clang/lib/Serialization/ASTWriter.cpp:3016
   for (const auto  : *State) {
-if (I.second.isPragma() || IncludeNonPragmaStates) {
-  Record.push_back(I.first);
-  Record.push_back(I.second.serialize());
-}
+// Maybe skip non-pragmas.
+if (!I.second.isPragma() && !IncludeNonPragmaStates)

Is pragma in this context refer to `#pragma diagnostics push/pop`? Do we have 
test to cover those to be deterministic?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154016

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as` on AIX

[Steven Wu via Phabricator via cfe-commits]

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

I see. Sounds fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D152924: [libLTO][AIX] Respect `-f[no]-integrated-as`

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

`lld` does not use LTOCodeGenerator.

I am also not sure how disable integrated assembler suppose to work? Do you 
have an end to end design for how to invoke assembler during linking? For 
current libLTO API, this is probably going to produce assembler file buffer to 
linker, which is not usable by linker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152924

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG0480748ea672: [DeclContext] Sort the Decls before adding 
into DeclContext (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/test/Modules/decl-params-determinisim.m

Index: clang/test/Modules/decl-params-determinisim.m
===
--- /dev/null
+++ clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,96 @@
+/// Test determinisim when serializing anonymous decls. Create enough Decls in
+/// DeclContext that can overflow the small storage of SmallPtrSet to make sure
+/// the serialization does not rely on iteration order of SmallPtrSet.
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+/// Check the order of the decls first. If LLVM_ENABLE_REVERSE_ITERATION is on,
+/// it will fail the test early if the output is depending on the order of items
+/// in containers that has non-deterministic orders.
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm | FileCheck %s
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: diff %t1.pcm %t2.pcm
+
+/// Spot check entries to make sure they are in current ordering.
+/// op13 encodes the anonymous decl number which should be in order.
+// CHECK: 
+
Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6987,6 +6987,15 @@
 continue;
   DeclsInPrototype.push_back(ND);
 }
+// Sort DeclsInPrototype based on raw encoding of the source location.
+// Scope::decls() is iterating over a SmallPtrSet so sort the Decls before
+// moving to DeclContext. This provides a stable ordering for traversing
+// Decls in DeclContext, which is important for tasks like ASTWriter for
+// deterministic output.
+llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+  return D1->getLocation().getRawEncoding() <
+ D2->getLocation().getRawEncoding();
+});
   }
 
   // Remember that we parsed a function type, and remember the attributes.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4100790 , @dblaikie wrote:

> In D141625#4100762 , @steven_wu 
> wrote:
>
>> I don't think it is feasible with current tool to write a test that check 
>> semantically the order of decls in clang module. (Let me know if that was 
>> wrong). The current test already unfortunately relies on the module layout 
>> assuming `op13` is the field for anonymous declaration number.
>
> Sure enough - having these things have semantic identifiers rather than 
> numbers would help.
>
> Ah, perhaps I'm just confused - I'm not sure why a similar test, that tested 
> a different order of the op13 fields wouldn't've shown a failure without 
> reverse iteration and then failed with reverse iteration (or the other way 
> around) - and then could be updated with a different ordering with the fix.
>
> Sorry, I'm clearly not making much sense here. Yes, I think the test should 
> be reduced further (while still showing the failure in either forward or 
> reverse iteration - but, yes, losing coverage in the real world rehashing 
> situation) it'd make the test shorter and more maintainable (to @akyrtzi's 
> point about future changes that introduce benign reordering) but it's not the 
> worst example of long tests to tickle hash instability. *shrug*
>
> I'm not insisting on it/blocking this patch from going forward without 
> addressing this issue. Carry on.

No worry. Thanks for the thorough review and feedback. Maybe clang will have a 
tool to visualize clang binary module some day. I will go ahead and commit this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I don't think it is feasible with current tool to write a test that check 
semantically the order of decls in clang module. (Let me know if that was 
wrong). The current test already unfortunately relies on the module layout 
assuming `op13` is the field for anonymous declaration number. Adding more 
dependency on the exact layout of the clang module will make the test really 
fragile to any clang AST change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141632: [Module] Respect `-fno-pch-timestamps` when building modules

[Steven Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG5cff68fca0bc: [Module] Respect `-fno-pch-timestamps` when 
building modules (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141632

Files:
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Modules/timestamps.c


Index: clang/test/Modules/timestamps.c
===
--- /dev/null
+++ clang/test/Modules/timestamps.c
@@ -0,0 +1,30 @@
+/// Verify timestamps that gets embedded in the module
+#include 
+
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs %s
+// RUN: cp %t/c_library.pcm %t1.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm > %t1.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=TIMESTAMP 
--input-file %t1.dump
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs -fno-pch-timestamp %s
+// RUN: cp %t/c_library.pcm %t2.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t2.pcm > %t2.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=NOTIMESTAMP 
--input-file %t2.dump
+// RUN: not diff %t1.dump %t2.dump
+
+
+// CHECK: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -202,7 +202,8 @@
   /*AllowASTWithErrors=*/
   +CI.getFrontendOpts().AllowPCMWithCompilerErrors,
   /*IncludeTimestamps=*/
-  +CI.getFrontendOpts().BuildingImplicitModule,
+  +CI.getFrontendOpts().BuildingImplicitModule &&
+  +CI.getFrontendOpts().IncludeTimestamps,
   /*ShouldCacheASTInMemory=*/
   +CI.getFrontendOpts().BuildingImplicitModule));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(


Index: clang/test/Modules/timestamps.c
===
--- /dev/null
+++ clang/test/Modules/timestamps.c
@@ -0,0 +1,30 @@
+/// Verify timestamps that gets embedded in the module
+#include 
+
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs %s
+// RUN: cp %t/c_library.pcm %t1.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm > %t1.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=TIMESTAMP --input-file %t1.dump
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs -fno-pch-timestamp %s
+// RUN: cp %t/c_library.pcm %t2.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t2.pcm > %t2.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=NOTIMESTAMP --input-file %t2.dump
+// RUN: not diff %t1.dump %t2.dump
+
+
+// CHECK: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -202,7 +202,8 @@
   /*AllowASTWithErrors=*/
   +CI.getFrontendOpts().AllowPCMWithCompilerErrors,
   /*IncludeTimestamps=*/
-  +CI.getFrontendOpts().BuildingImplicitModule,
+  +CI.getFrontendOpts().BuildingImplicitModule &&
+  +CI.getFrontendOpts().IncludeTimestamps,
   /*ShouldCacheASTInMemory=*/
   +CI.getFrontendOpts().BuildingImplicitModule));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG516e30175256: [NFC][Profile] Access profile through 
VirtualFileSystem (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/SampleProfReader.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/IPO/SampleProfile.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/SampleProfReader.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/PGOOptions.cpp
  llvm/lib/Target/X86/X86InsertPrefetch.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-profgen/llvm-profgen.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/ProfileData/SampleProfTest.cpp

Index: llvm/unittests/ProfileData/SampleProfTest.cpp
===
--- llvm/unittests/ProfileData/SampleProfTest.cpp
+++ llvm/unittests/ProfileData/SampleProfTest.cpp
@@ -19,6 +19,7 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
@@ -57,8 +58,9 @@
 
   void readProfile(const Module , StringRef Profile,
StringRef RemapFile = "") {
+auto FS = vfs::getRealFileSystem();
 auto ReaderOrErr = SampleProfileReader::create(
-std::string(Profile), Context, FSDiscriminatorPass::Base,
+std::string(Profile), Context, *FS, FSDiscriminatorPass::Base,
 std::string(RemapFile));
 ASSERT_TRUE(NoError(ReaderOrErr.getError()));
 Reader = std::move(ReaderOrErr.get());
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
@@ -333,22 +334,25 @@
bool EnableDebugify, bool VerifyDIPreserve) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
+  auto FS = vfs::getRealFileSystem();
   std::optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
+   PGOOptions::IRUse);
 break;
   case SampleUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile,
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
PGOOptions::SampleUse);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
-  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
- DebugInfoForProfiling, PseudoProbeForProfiling);
+  P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
+ PGOOptions::NoCSAction, DebugInfoForProfiling,
+ PseudoProbeForProfiling);
 else
   P = std::nullopt;
   }
@@ -367,7 +371,7 @@
 P->CSAction = PGOOptions::CSIRInstr;
 P->CSProfileGenFile = CSProfileGenFile;
   } else
-P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile,
+P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
PGOOptions::NoAction, PGOOptions::CSIRInstr);
 } else /* CSPGOKindFlag == CSInstrUse */ 

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

> Sorry, I'm still not really following - OK, sounds like you're saying this 
> test does fail at HEAD/without this patch in reverse iteration mode, and is a 
> bit larger than would be minimally necessary to achieve that, but also might 
> fail at HEAD without reverse iteration, providing somewhat more testing than 
> if it were fully minimized/only caught in reverse.
>
> Fair enough -I don't think it's the right tradeoff, but I'm glad it's 
> stable/provides that coverage.

The main motivations are:

- Reverse iteration coverage from bots are really low.
- The FileCheck that supposed to fail on reverse iteration is not fully 
checking the order of decls in the serialized bitcode in a semantic way. It is 
only checking an index, which also assigned based on an iteration order. If 
both iterations are iterating none deterministic container, both will be 
reversed and the test will pass :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 493768.
steven_wu added a comment.

Try fix pre-merge build failure


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/SampleProfReader.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/IPO/SampleProfile.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/SampleProfReader.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/PGOOptions.cpp
  llvm/lib/Target/X86/X86InsertPrefetch.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-profgen/llvm-profgen.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/ProfileData/SampleProfTest.cpp

Index: llvm/unittests/ProfileData/SampleProfTest.cpp
===
--- llvm/unittests/ProfileData/SampleProfTest.cpp
+++ llvm/unittests/ProfileData/SampleProfTest.cpp
@@ -19,6 +19,7 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
@@ -57,8 +58,9 @@
 
   void readProfile(const Module , StringRef Profile,
StringRef RemapFile = "") {
+auto FS = vfs::getRealFileSystem();
 auto ReaderOrErr = SampleProfileReader::create(
-std::string(Profile), Context, FSDiscriminatorPass::Base,
+std::string(Profile), Context, *FS, FSDiscriminatorPass::Base,
 std::string(RemapFile));
 ASSERT_TRUE(NoError(ReaderOrErr.getError()));
 Reader = std::move(ReaderOrErr.get());
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
@@ -333,22 +334,25 @@
bool EnableDebugify, bool VerifyDIPreserve) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
+  auto FS = vfs::getRealFileSystem();
   std::optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
+   PGOOptions::IRUse);
 break;
   case SampleUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile,
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
PGOOptions::SampleUse);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
-  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
- DebugInfoForProfiling, PseudoProbeForProfiling);
+  P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
+ PGOOptions::NoCSAction, DebugInfoForProfiling,
+ PseudoProbeForProfiling);
 else
   P = std::nullopt;
   }
@@ -367,7 +371,7 @@
 P->CSAction = PGOOptions::CSIRInstr;
 P->CSProfileGenFile = CSProfileGenFile;
   } else
-P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile,
+P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
PGOOptions::NoAction, PGOOptions::CSIRInstr);
 } else /* CSPGOKindFlag == CSInstrUse */ {
   if (!P) {
Index: llvm/tools/llvm-profgen/llvm-profgen.cpp

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4094837 , @dblaikie wrote:

> In D141625#4094742 , @steven_wu 
> wrote:
>
>> In D141625#4067225 , @dblaikie 
>> wrote:
>>
>>> In D141625#4066961 , @steven_wu 
>>> wrote:
>>>
 No, reverse iteration will not break diff test for a small number of 
 decls. Everything will be in reverse order so it is the same.
>>>
>>> Hmm, I'm not sure I'm following why that is - could you explain this in 
>>> more detail? The usual problem is that, say, we're outputting based on an 
>>> unstable order - even two items would be enough to cause a test of that to 
>>> fail in either forward or reverse iteration mode until the order is 
>>> stabilized.
>>>
>>> Is that not the case here? Is there some interaction between iteration 
>>> order that's part of the nondeterminism here?
>>
>> In order to make a test that will break before the change with reverse 
>> iteration, the test needs to check that the decls/records are serialized 
>> into the module in a pre deterministic order. It can't be just diff the 
>> output of two runs with a small input because small input will not overflow 
>> the smallptrset, thus the reverse iteration outputs from two runs will very 
>> likely to be identical, just in a different order from forward iteration.
>
> Sure, I think I'm with you there - but the current test checks that the 
> decls/records are serialized into the module in a pre-deterministic order, 
> right? So it doesn't seem like a reverse iteration-failing test would be more 
> involved/brittle/less robust than the test being added here?

Yes, exactly, I added file check so that this test is going to fail for reverse 
iteration. What I also want is keep the size of the test case since it not 
really enormous so this test also has a good chance of failing without reverse 
iteration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 493724.
steven_wu added a comment.

Rebase to main.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/SampleProfReader.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/IPO/SampleProfile.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/SampleProfReader.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/PGOOptions.cpp
  llvm/lib/Target/X86/X86InsertPrefetch.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-profgen/llvm-profgen.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/ProfileData/SampleProfTest.cpp

Index: llvm/unittests/ProfileData/SampleProfTest.cpp
===
--- llvm/unittests/ProfileData/SampleProfTest.cpp
+++ llvm/unittests/ProfileData/SampleProfTest.cpp
@@ -19,6 +19,7 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
@@ -57,8 +58,9 @@
 
   void readProfile(const Module , StringRef Profile,
StringRef RemapFile = "") {
+auto FS = vfs::getRealFileSystem();
 auto ReaderOrErr = SampleProfileReader::create(
-std::string(Profile), Context, FSDiscriminatorPass::Base,
+std::string(Profile), Context, *FS, FSDiscriminatorPass::Base,
 std::string(RemapFile));
 ASSERT_TRUE(NoError(ReaderOrErr.getError()));
 Reader = std::move(ReaderOrErr.get());
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
@@ -333,22 +334,25 @@
bool EnableDebugify, bool VerifyDIPreserve) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
+  auto FS = vfs::getRealFileSystem();
   std::optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
+   PGOOptions::IRUse);
 break;
   case SampleUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile,
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
PGOOptions::SampleUse);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
-  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
- DebugInfoForProfiling, PseudoProbeForProfiling);
+  P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
+ PGOOptions::NoCSAction, DebugInfoForProfiling,
+ PseudoProbeForProfiling);
 else
   P = std::nullopt;
   }
@@ -367,7 +371,7 @@
 P->CSAction = PGOOptions::CSIRInstr;
 P->CSProfileGenFile = CSProfileGenFile;
   } else
-P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile,
+P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
PGOOptions::NoAction, PGOOptions::CSIRInstr);
 } else /* CSPGOKindFlag == CSInstrUse */ {
   if (!P) {
Index: llvm/tools/llvm-profgen/llvm-profgen.cpp

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4067225 , @dblaikie wrote:

> In D141625#4066961 , @steven_wu 
> wrote:
>
>> No, reverse iteration will not break diff test for a small number of decls. 
>> Everything will be in reverse order so it is the same.
>
> Hmm, I'm not sure I'm following why that is - could you explain this in more 
> detail? The usual problem is that, say, we're outputting based on an unstable 
> order - even two items would be enough to cause a test of that to fail in 
> either forward or reverse iteration mode until the order is stabilized.
>
> Is that not the case here? Is there some interaction between iteration order 
> that's part of the nondeterminism here?

In order to make a test that will break before the change with reverse 
iteration, the test needs to check that the decls/records are serialized into 
the module in a pre deterministic order. It can't be just diff the output of 
two runs with a small input because small input will not overflow the 
smallptrset, thus the reverse iteration outputs from two runs will very likely 
to be identical, just in a different order from forward iteration.

It is also in the same time not easy to write a test that can check the pre 
deterministic order of the serialization because it is hard to identify which 
entry is which decl from the output of bcanalyzer. For example, an entry like 
`` means nothing 
just looking at itself. Even worse, the opcode I check `op13` in the test is 
assigned via iteration order and decls are serialized via iteration order. So 
all the entries will always kind of appear in ascending order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D136315#4069849 , @calebzulawski 
wrote:

> In my situation, at least, I am the vendor of the toolchain and my 
> configuration file contains `--sysroot=/../path/to/sysroot` with a 
> known relative path to where the sysroot is distributed.  Apple is unique in 
> this situation, since I am not distributing the sysroot, so there is no 
> equivalent to using the `` variable.  There is no way to invoke 
> `xcrun` or set `SDKROOT` from config files, only pass flags.
>
> It is true that the build system could do it, but I think it's reasonable for 
> the driver to make a guess when not provided.  I don't think it's all that 
> different than detection of libstdc++ from GCC installs.

I definitely feel like if to fix your problem, a reverse option like 
`-infer-sdkroot-from-xcrun` might be better/safer but it doesn't help the 
general case where people doesn't know `/usr/bin/clang` is different from a 
regular clang that a regular clang needs to pass -isysroot.

@arphaman Any suggestions?




Comment at: clang/test/Driver/darwin-sdk-detect.c:1
+// REQUIRES: system-darwin
+

calebzulawski wrote:
> steven_wu wrote:
> > This test won't work in all conditions. Like I said, you requires to have a 
> > Xcode/CommandLineTools installation to make `xcrun` work.
> > 
> > Maybe you can make the argument that you are compiling clang, so you must 
> > have one of those installed, but CommandLineTools definitely doesn't have 
> > iOS SDK for the second test.
> True, maybe we can just remove the iOS test if you think that's sufficient.  
> Otherwise, is there any way to make the have the test check that either the 
> `-isysroot` flag is present with that pattern, or not present at all?  I'm 
> not too familiar with llvm-lit, I just browsed a few other examples before 
> writing this.
I don't think you can write a FileCheck with that. The best way to do this test 
might be in `clang/test/Driver/lit.local.cfg` write checks to make sure:
* Darwin platform
* `xcrun` exists
* xcrun can resolve all the platforms you want to test

Then add a new feature which you can `REQUIRES` in this test case.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

This is not an apple platform problem. This is a cross compile problem where 
you have a SDK that is installed for cross compile target. In those cases, you 
just can't hard code the path to isysroot, and figuring out the sysroot is 
generally considered a problem for the build system, not the problem for 
compiler. If you have a config file, you can definitely setup `SDKROOT` in your 
configuration system after running `xcrun`. A similar but identical problem is 
if you just install clang package but didn't install dev packages on a linux 
platform where you just can't find any headers. Same error, just different 
causes.

To be clear, I am not against adding features in clang to make user experience 
better, but I think this needs some polish to addresses problem mentioned above 
transparently.




Comment at: clang/test/Driver/darwin-sdk-detect.c:1
+// REQUIRES: system-darwin
+

This test won't work in all conditions. Like I said, you requires to have a 
Xcode/CommandLineTools installation to make `xcrun` work.

Maybe you can make the argument that you are compiling clang, so you must have 
one of those installed, but CommandLineTools definitely doesn't have iOS SDK 
for the second test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D136315#4069582 , @calebzulawski 
wrote:

> That makes more sense, I thought perhaps it was using `DEFAULT_SYSROOT`.  The 
> shim isn't smart enough to choose the sysroot from the target unfortunately.
>
> It looks like the only error is unrelated to this change, something with 
> concepts in libc++.

The shim only works if you invoke `/usr/bin/clang`, and that shim is for 
building host OS. I am not sure it is a desirable behavior for clang to call 
into `xcrun` because it can be very slow on first invocation and you will get 
error if you don't have Xcode or Commandlinetools installed (on a basic macOS 
install basically).

I wonder if the better solution is print a very actionable diagnostics when 
building c++ but standard c++ library cannot be found.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D136315#4067205 , @calebzulawski 
wrote:

> One thing to throw into the mix: Apple's clang has a default sysroot 
> configured, so with the default system compiler, there is no way to replicate 
> this "build without a sysroot" scenario as far as I can tell.  For the system 
> compiler, I believe this behavior is a strict improvement.

No, apple's clang doesn't have a default sysroot. When you run 
`/usr/bin/clang`, which is a shim that calls `xcrun`, it finds the clang in 
your toolchain and invokes it with `SDKROOT` environment variable. That is the 
behavior of the shim, not the behavior of clang.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

No, reverse iteration will not break diff test for a small number of decls. 
Everything will be in reverse order so it is the same. Current test will fail 
early in reverse iteration and will fail in the end statistically for forward 
iteration. Will pass in all configurations. I think paying a bit more decls to 
make it fail in forward iteration is not a bad trade off.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D136315: [clang][Darwin] Try to guess the SDK root with xcrun when unspecified

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

This breaks macOS bot too: 
https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/33839/

I think this changes might be more disruptive than expected. While it is 
encouraged to use `-isysroot` when building anything on Darwin platform, it is 
also perfectly "fine" to build without sysroot specified. Forcing a sysroot on 
users who are not specifying the isysroot will change the header/library search 
behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136315

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 490616.
steven_wu added a comment.

Update tests according to feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/test/Modules/decl-params-determinisim.m

Index: clang/test/Modules/decl-params-determinisim.m
===
--- /dev/null
+++ clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,96 @@
+/// Test determinisim when serializing anonymous decls. Create enough Decls in
+/// DeclContext that can overflow the small storage of SmallPtrSet to make sure
+/// the serialization does not rely on iteration order of SmallPtrSet.
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+/// Check the order of the decls first. If LLVM_ENABLE_REVERSE_ITERATION is on,
+/// it will fail the test early if the output is depending on the order of items
+/// in containers that has non-deterministic orders.
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm | FileCheck %s
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: diff %t1.pcm %t2.pcm
+
+/// Spot check entries to make sure they are in current ordering.
+/// op13 encodes the anonymous decl number which should be in order.
+// CHECK: 
+
Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6986,6 +6986,15 @@
 continue;
   DeclsInPrototype.push_back(ND);
 }
+// Sort DeclsInPrototype based on raw encoding of the source location.
+// Scope::decls() is iterating over a SmallPtrSet so sort the Decls before
+// moving to DeclContext. This provides a stable ordering for traversing
+// Decls in DeclContext, which is important for tasks like ASTWriter for
+// deterministic output.
+llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+  return D1->getLocation().getRawEncoding() <
+ D2->getLocation().getRawEncoding();
+});
   }
 
   // Remember that we parsed a function type, and remember the attributes.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4066466 , @dblaikie wrote:

> In D141625#4066362 , @steven_wu 
> wrote:
>
>> @dblaikie Do you have any objection for committing the patch as it is? I 
>> don't think reverse iteration test is a proper way to test for this specific 
>> bug.
>
> I think reverse iteration is the right way to test this specific bug (& any 
> bug where behavior may be unintentionally nondeterministic due to 
> non-deterministically ordered containers) - it makes the testing more 
> reliable rather than depending on implementation details of such containers 
> that aren't guaranteed (that being the point/problem).
>
> But it's not the worst/most unwieldy test for this sort of thing & if we 
> don't have bots using it... hmm, actually I looked more closely & maybe we do 
> have reverse iteration builders: 
> https://github.com/llvm/llvm-zorg/search?q=reverse-iteration (though I'm 
> having trouble navigating the builder UI to see whether there are up-to-date 
> builds for this configuration)
>
> Could you help me understand your perspective regarding using reverse 
> iteration to test this specific bug? (are there some bugs you find reverse 
> iteration suitable for? what's different about this one from them?)

With reverse iteration, you can make sure that the we didn't iterate over the 
non-deterministic container by checking the ordering of the decls in the 
module, which is fine. But no one really runs that tests regularly.
Without the reverse iteration, the ordering check will always succeed because 
the anonymous decls will be numbered and ordered in the ascending order as 
iteration. It is not easy to decode which decl is which by analyze the output 
of llvm-bcanalyzer. Current test (which is not big at all) will put (80 - 32) 
entries into hash table of smallptrset, thus it kind of triggers the bug near 
100% of the time. I would think the current test is more valuable than a test 
only fail in reverse iteration.

I can add extra file check to make sure all decls are in order, so if you run 
reverse iteration, you will fail before hitting the diff test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

@dblaikie Do you have any objection for committing the patch as it is? I don't 
think reverse iteration test is a proper way to test for this specific bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4060460 , @dblaikie wrote:

> In D141625#4059486 , @steven_wu 
> wrote:
>
>> @dblaikie Do we have any bots running reverse iteration?
>
> Hmm, not that I can see/find at a quick glance, which is unfortunate. @mgrang 
> Are you still working on LLVM/have any knowledge of reverse iteration testing 
> being done? (@colinl - looks like maybe you're a Code Aurora person, perhaps 
> you've got some more recent context for this?)

At another look, it is not very easy to write a test that checks a strict 
ordering of the serialization. The only tool I know that can dump the module is 
`llvm-bcanalyzer` but everything in the module will appear in the order of 
anonymous decl number order. You have to decode the Decls and Types to know if 
they actually appear in the source ordering. A diff test probably makes a lot 
more sense here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4059540 , @tschuett wrote:

> EXPANSIVE_CHECKS will reshuffle the llvm::sort input: 
> https://lists.llvm.org/pipermail/llvm-dev/2018-April/122576.html

This is a slightly different concern. The problem to catch is iterating over a 
SmallPtrSet, not identical sort key.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

@dblaikie Do we have any bots running reverse iteration?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4056831 , @steven_wu wrote:

> Actually, sorting in `numberAnonymousDeclsWithin` doesn't work for some 
> reasons.

The reason for this doesn't work is `ASTWriter::WriteDeclContextLexicalBlock` 
also iterates on `DeclContext::decls()`, so there are at least two sorts needed 
in ASTWriter. I prefer the current implementation if there isn't any 
performance overhead since it makes the iterator on DeclContext to have stable 
order.

In D141625#4059186 , @dblaikie wrote:

> In D141625#4053067 , @steven_wu 
> wrote:
>
>> @akyrtzi has the good idea. It is really hard to control `Decl*` to get 
>> values
>> to get an unstable iteration order from the small tests, going beyond 32 
>> decls
>> to get out of SmallPtrSet's small model is much consistent.
>>
>> Add test.
>
> Could you make a smaller test (probably just a couple of decls) that fails 
> with LLVM_ENABLE_REVERSE_ITERATION? Such a failure would be more 
> reliable/simpler to reproduce, probably?

Sure but it is going to put a different requirement on the tests. Now ASTWriter 
only cares about a stable order for deterministic output so it is doing a diff 
on pcm. If changing to the reverse iterator test, this need to change to do 
FileCheck on a predetermined order.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

Actually, sorting in `numberAnonymousDeclsWithin` doesn't work for some reasons.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 489587.
steven_wu added a comment.

Minor touch up on the test case. Also add some comments in test to explain what 
is being tested.

I don't measure any performance regression from `-fsyntax-only` on Foundation.h 
but if there are some other performance benchmark I need to be tested first, 
let me know.
Alternatively, there can be a even safer fix by moving the sorting into 
`numberAnonymousDeclsWithin` which seems to be only used in serialization. I 
can change to that to be extra safe.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/test/Modules/decl-params-determinisim.m


Index: clang/test/Modules/decl-params-determinisim.m
===
--- /dev/null
+++ clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,70 @@
+/// Test determinisim when serializing anonymous decls. Create enough Decls in
+/// DeclContext that can overflow the small storage of SmallPtrSet to make sure
+/// the serialization does not rely on iteration order of SmallPtrSet.
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 
\
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 
\
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: diff %t1.pcm %t2.pcm
+
+//--- headers/a.h
+void f(struct A0 *a0,
+   struct A1 *a1,
+   struct A2 *a2,
+   struct A3 *a3,
+   struct A4 *a4,
+   struct A5 *a5,
+   struct A6 *a6,
+   struct A7 *a7,
+   struct A8 *a8,
+   struct A9 *a9,
+   struct A10 *a10,
+   struct A11 *a11,
+   struct A12 *a12,
+   struct A13 *a13,
+   struct A14 *a14,
+   struct A15 *a15,
+   struct A16 *a16,
+   struct A17 *a17,
+   struct A18 *a18,
+   struct A19 *a19,
+   struct A20 *a20,
+   struct A21 *a21,
+   struct A22 *a22,
+   struct A23 *a23,
+   struct A24 *a24,
+   struct A25 *a25,
+   struct A26 *a26,
+   struct A27 *a27,
+   struct A28 *a28,
+   struct A29 *a29,
+   struct A30 *a30,
+   struct A31 *a31,
+   struct A32 *a32,
+   struct A33 *a33,
+   struct A34 *a34,
+   struct A35 *a35,
+   struct A36 *a36,
+   struct A37 *a37,
+   struct A38 *a38,
+   struct A39 *a39,
+   struct A40 *a40);
+
+
+//--- headers/module.modulemap
+
+module A {
+  header "a.h"
+}
+
+//--- main.m
+
+#import 
+
Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6986,6 +6986,15 @@
 continue;
   DeclsInPrototype.push_back(ND);
 }
+// Sort DeclsInPrototype based on raw encoding of the source location.
+// Scope::decls() is iterating over a SmallPtrSet so sort the Decls before
+// moving to DeclContext. This provides a stable ordering for traversing
+// Decls in DeclContext, which is important for tasks like ASTWriter for
+// deterministic output.
+llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+  return D1->getLocation().getRawEncoding() <
+ D2->getLocation().getRawEncoding();
+});
   }
 
   // Remember that we parsed a function type, and remember the attributes.


Index: clang/test/Modules/decl-params-determinisim.m
===
--- /dev/null
+++ clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,70 @@
+/// Test determinisim when serializing anonymous decls. Create enough Decls in
+/// DeclContext that can overflow the small storage of SmallPtrSet to make sure
+/// the serialization does not rely on iteration order of SmallPtrSet.
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: diff %t1.pcm %t2.pcm
+
+//--- headers/a.h
+void f(struct A0 *a0,
+   struct A1 *a1,
+   struct A2 *a2,
+   struct A3 *a3,
+   struct 

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 489150.
steven_wu added a comment.

@akyrtzi has the good idea. It is really hard to control `Decl*` to get values
to get an unstable iteration order from the small tests, going beyond 32 decls
to get out of SmallPtrSet's small model is much consistent.

Add test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

Files:
  clang/lib/Parse/ParseDecl.cpp
  clang/test/Modules/decl-params-determinisim.m


Index: clang/test/Modules/decl-params-determinisim.m
===
--- /dev/null
+++ clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,67 @@
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 
\
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 
\
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: diff %t1.pcm %t2.pcm
+
+//--- headers/a.h
+void f(struct A1 *a0,
+   struct A1 *a1,
+   struct A2 *a2,
+   struct A3 *a3,
+   struct A4 *a4,
+   struct A5 *a5,
+   struct A6 *a6,
+   struct A7 *a7,
+   struct A8 *a8,
+   struct A9 *a9,
+   struct A10 *a10,
+   struct A11 *a11,
+   struct A12 *a12,
+   struct A13 *a13,
+   struct A14 *a14,
+   struct A15 *a15,
+   struct A16 *a16,
+   struct A17 *a17,
+   struct A18 *a18,
+   struct A19 *a19,
+   struct A20 *a20,
+   struct A21 *a21,
+   struct A22 *a22,
+   struct A23 *a23,
+   struct A24 *a24,
+   struct A25 *a25,
+   struct A26 *a26,
+   struct A27 *a27,
+   struct A28 *a28,
+   struct A29 *a29,
+   struct A30 *a30,
+   struct A31 *a31,
+   struct A32 *a32,
+   struct A33 *a33,
+   struct A34 *a34,
+   struct A35 *a35,
+   struct A36 *a36,
+   struct A37 *a37,
+   struct A38 *a38,
+   struct A39 *a39,
+   struct A40 *a40);
+
+
+//--- headers/module.modulemap
+
+module A {
+  header "a.h"
+}
+
+//--- main.m
+
+#import 
+
Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6986,6 +6986,15 @@
 continue;
   DeclsInPrototype.push_back(ND);
 }
+// Sort DeclsInPrototype based on raw encoding of the source location.
+// Scope::decls() is iterating over a SmallPtrSet so sort the Decls before
+// moving to DeclContext. This provides a stable ordering for traversing
+// Decls in DeclContext, which is important for tasks like ASTWriter for
+// deterministic output.
+llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+  return D1->getLocation().getRawEncoding() <
+ D2->getLocation().getRawEncoding();
+});
   }
 
   // Remember that we parsed a function type, and remember the attributes.


Index: clang/test/Modules/decl-params-determinisim.m
===
--- /dev/null
+++ clang/test/Modules/decl-params-determinisim.m
@@ -0,0 +1,67 @@
+// RUN: rm -rf %t.dir
+// RUN: split-file %s %t.dir
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t1.pcm
+// RUN: rm -rf %t.dir/cache
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps \
+// RUN:   -fmodules-cache-path=%t.dir/cache -triple x86_64-apple-macosx10.11.0 \
+// RUN:   -I%t.dir/headers %t.dir/main.m -fdisable-module-hash -Wno-visibility
+// RUN: mv %t.dir/cache/A.pcm %t2.pcm
+// RUN: diff %t1.pcm %t2.pcm
+
+//--- headers/a.h
+void f(struct A1 *a0,
+   struct A1 *a1,
+   struct A2 *a2,
+   struct A3 *a3,
+   struct A4 *a4,
+   struct A5 *a5,
+   struct A6 *a6,
+   struct A7 *a7,
+   struct A8 *a8,
+   struct A9 *a9,
+   struct A10 *a10,
+   struct A11 *a11,
+   struct A12 *a12,
+   struct A13 *a13,
+   struct A14 *a14,
+   struct A15 *a15,
+   struct A16 *a16,
+   struct A17 *a17,
+   struct A18 *a18,
+   struct A19 *a19,
+   struct A20 *a20,
+   struct A21 *a21,
+   struct A22 *a22,
+   struct A23 *a23,
+   struct A24 *a24,
+   struct A25 *a25,
+   struct A26 *a26,
+   struct A27 *a27,
+   struct A28 *a28,
+   struct A29 *a29,
+   struct A30 *a30,
+   struct A31 *a31,
+   struct A32 *a32,

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D141625#4052869 , @rsmith wrote:

> In D141625#4052820 , @dblaikie 
> wrote:
>
>> Yeah, might be nice to document where the instability comes from - if it 
>> comes from a DenseMap or similar, then a test that fails either in forward 
>> or reverse iteration mode would be nice to have.
>
> `Scope::decls` is iterating over a `SmallPtrSet`. This might be a little 
> annoying to create a test for, because it'll only be unstable if we have two 
> decls in the same prototype scope that get allocated into different slabs by 
> the bump ptr allocator, but maybe something like:
>
>   void f(struct A *p, int arr[1 + 0 + 0 + 0 + ... + 0], struct B *q);
>
> ... would be enough (with sufficient subexpressions to fill a whole slab). 
> Then we can build a .pch for that twice and check that it comes out identical.

Yeah, it is really hard to trigger a failure. Even the second decl gets onto a 
different slab, the second slab will most likely come after the previous one 
when there isn't much memory pressure.

I can write a test like that but it is really hard to trigger without an 
enormous module (building Darwin module from macOS SDK can reproduce very 
reliable).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141625

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

[PATCH] D141632: [Module] Respect `-fno-pch-timestamps` when building modules

[Steven Wu via Phabricator via cfe-commits]

steven_wu created this revision.
steven_wu added reviewers: benlangmuir, akyrtzi, jansvoboda11.
Herald added a subscriber: ributzka.
Herald added a project: All.
steven_wu requested review of this revision.
Herald added a project: clang.

Always respect the FrontendOption to not include timestamps in PCH or
Modules when `-fno-pch-timestamps` is specified.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141632

Files:
  clang/lib/Frontend/FrontendActions.cpp
  clang/test/Modules/timestamps.c


Index: clang/test/Modules/timestamps.c
===
--- /dev/null
+++ clang/test/Modules/timestamps.c
@@ -0,0 +1,30 @@
+/// Verify timestamps that gets embedded in the module
+#include 
+
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs %s
+// RUN: cp %t/c_library.pcm %t1.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm > %t1.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=TIMESTAMP 
--input-file %t1.dump
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs -fno-pch-timestamp %s
+// RUN: cp %t/c_library.pcm %t2.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t2.pcm > %t2.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=NOTIMESTAMP 
--input-file %t2.dump
+// RUN: not diff %t1.dump %t2.dump
+
+
+// CHECK: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -201,7 +201,8 @@
   /*AllowASTWithErrors=*/
   +CI.getFrontendOpts().AllowPCMWithCompilerErrors,
   /*IncludeTimestamps=*/
-  +CI.getFrontendOpts().BuildingImplicitModule,
+  +CI.getFrontendOpts().BuildingImplicitModule &&
+  +CI.getFrontendOpts().IncludeTimestamps,
   /*ShouldCacheASTInMemory=*/
   +CI.getFrontendOpts().BuildingImplicitModule));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(


Index: clang/test/Modules/timestamps.c
===
--- /dev/null
+++ clang/test/Modules/timestamps.c
@@ -0,0 +1,30 @@
+/// Verify timestamps that gets embedded in the module
+#include 
+
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs %s
+// RUN: cp %t/c_library.pcm %t1.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t1.pcm > %t1.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=TIMESTAMP --input-file %t1.dump
+// RUN: rm -rf %t
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fdisable-module-hash \
+// RUN:   -fmodules-cache-path=%t -I %S/Inputs -fno-pch-timestamp %s
+// RUN: cp %t/c_library.pcm %t2.pcm
+// RUN: llvm-bcanalyzer --dump --disable-histogram %t2.pcm > %t2.dump
+// RUN: FileCheck %s --check-prefix=CHECK --check-prefix=NOTIMESTAMP --input-file %t2.dump
+// RUN: not diff %t1.dump %t2.dump
+
+
+// CHECK: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
+// CHECK-NEXT: 
Index: clang/lib/Frontend/FrontendActions.cpp
===
--- clang/lib/Frontend/FrontendActions.cpp
+++ clang/lib/Frontend/FrontendActions.cpp
@@ -201,7 +201,8 @@
   /*AllowASTWithErrors=*/
   +CI.getFrontendOpts().AllowPCMWithCompilerErrors,
   /*IncludeTimestamps=*/
-  +CI.getFrontendOpts().BuildingImplicitModule,
+  +CI.getFrontendOpts().BuildingImplicitModule &&
+  +CI.getFrontendOpts().IncludeTimestamps,
   /*ShouldCacheASTInMemory=*/
   +CI.getFrontendOpts().BuildingImplicitModule));
   Consumers.push_back(CI.getPCHContainerWriter().CreatePCHContainerGenerator(
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D141625: [DeclContext] Sort the Decls before adding into DeclContext

[Steven Wu via Phabricator via cfe-commits]

steven_wu created this revision.
steven_wu added reviewers: jansvoboda11, akyrtzi, benlangmuir, vsapsai, rnk, 
dblaikie.
Herald added subscribers: ributzka, mgrang.
Herald added a project: All.
steven_wu requested review of this revision.
Herald added a project: clang.

Fix a non-deterministic issue in clang module generation, which the
anonymous declaration number from a function context is not
deterministic. This is due to the unstable iteration order for decls in
scope so the order after moving the decls into function decl context is
not deterministic.

>From https://reviews.llvm.org/D135118, we can't use a set that preserves
the order without the performance penalty. Fix the issue by sorting the
decls based on raw encoding of their source location.

rdar://104097976


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141625

Files:
  clang/lib/Parse/ParseDecl.cpp


Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6986,6 +6986,13 @@
 continue;
   DeclsInPrototype.push_back(ND);
 }
+// Sort DeclsInPrototype based on raw encoding of the source location. This
+// provides a stable iterating order in DeclContext, which is important for
+// tasks like ASTWriter for deterministic output.
+llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+  return D1->getLocation().getRawEncoding() <
+ D2->getLocation().getRawEncoding();
+});
   }
 
   // Remember that we parsed a function type, and remember the attributes.


Index: clang/lib/Parse/ParseDecl.cpp
===
--- clang/lib/Parse/ParseDecl.cpp
+++ clang/lib/Parse/ParseDecl.cpp
@@ -6986,6 +6986,13 @@
 continue;
   DeclsInPrototype.push_back(ND);
 }
+// Sort DeclsInPrototype based on raw encoding of the source location. This
+// provides a stable iterating order in DeclContext, which is important for
+// tasks like ASTWriter for deterministic output.
+llvm::sort(DeclsInPrototype, [](Decl *D1, Decl *D2) {
+  return D1->getLocation().getRawEncoding() <
+ D2->getLocation().getRawEncoding();
+});
   }
 
   // Remember that we parsed a function type, and remember the attributes.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 487597.
steven_wu added a comment.

Address review feedback. Remove NFC from title.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/SampleProfReader.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/IPO/SampleProfile.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/SampleProfReader.cpp
  llvm/lib/Support/CMakeLists.txt
  llvm/lib/Support/PGOOptions.cpp
  llvm/lib/Target/X86/X86InsertPrefetch.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-profgen/llvm-profgen.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/ProfileData/SampleProfTest.cpp

Index: llvm/unittests/ProfileData/SampleProfTest.cpp
===
--- llvm/unittests/ProfileData/SampleProfTest.cpp
+++ llvm/unittests/ProfileData/SampleProfTest.cpp
@@ -19,6 +19,7 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
@@ -57,8 +58,9 @@
 
   void readProfile(const Module , StringRef Profile,
StringRef RemapFile = "") {
+auto FS = vfs::getRealFileSystem();
 auto ReaderOrErr = SampleProfileReader::create(
-std::string(Profile), Context, FSDiscriminatorPass::Base,
+std::string(Profile), Context, *FS, FSDiscriminatorPass::Base,
 std::string(RemapFile));
 ASSERT_TRUE(NoError(ReaderOrErr.getError()));
 Reader = std::move(ReaderOrErr.get());
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
@@ -333,22 +334,25 @@
bool EnableDebugify, bool VerifyDIPreserve) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
+  auto FS = vfs::getRealFileSystem();
   std::optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
+   PGOOptions::IRUse);
 break;
   case SampleUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile,
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
PGOOptions::SampleUse);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
-  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
- DebugInfoForProfiling, PseudoProbeForProfiling);
+  P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
+ PGOOptions::NoCSAction, DebugInfoForProfiling,
+ PseudoProbeForProfiling);
 else
   P = std::nullopt;
   }
@@ -363,7 +367,7 @@
 P->CSAction = PGOOptions::CSIRInstr;
 P->CSProfileGenFile = CSProfileGenFile;
   } else
-P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile,
+P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
PGOOptions::NoAction, PGOOptions::CSIRInstr);
 } else /* CSPGOKindFlag == CSInstrUse */ {
   if (!P)
Index: llvm/tools/llvm-profgen/llvm-profgen.cpp

[PATCH] D139938: [clang] Don't spuriously pass -stdlib=libc++ to CC1 on Darwin

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D139938#4008232 , @hans wrote:

> This seems to have broken the instrprof-darwin-exports.c test, see e.g. 
> https://green.lab.llvm.org/green/job/clang-stage1-RA/32351/
>
> I'll revert for now.

It seems you still need to claim `OPT_stdlib_EQ` so it doesn't complain about 
unused argument when you pass stdlib when compiling C code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139938

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

[PATCH] D137838: [Support] Move TargetParsers to new component

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

Also needed a follow up fix to completely fix module `9cd6fbee7ed8`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137838

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

[PATCH] D140164: [clang/Lexer] Enhance `Lexer::getImmediateMacroNameForDiagnostics` to return a result from non-file buffers

[Steven Wu via Phabricator via cfe-commits]

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

LGTM




Comment at: clang/lib/Lex/Lexer.cpp:1050
 
-  // If the macro's spelling has no FileID, then it's actually a token paste
-  // or stringization (or similar) and not a macro at all.
-  if (!SM.getFileEntryForID(SM.getFileID(SM.getSpellingLoc(Loc
+  // If the macro's spelling isn't FileID or came from the scratch space, then
+  // it's actually a token paste or stringization (or similar) and not a macro

nit: isn't FileID or from scratch space


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140164

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

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

The workaround in https://reviews.llvm.org/D139956 can get llvm-project 
bootstrap correctly, doesn't fix the underlying issue.

The new failures is in building `SupportTests` where it issues a weird warning 
followed by missing symbol. It might be related to this commit too:
https://green.lab.llvm.org/green/job/clang-stage2-Rthinlto/6488/

  
/Users/buildslave/jenkins/workspace/clang-stage2-Rthinlto/llvm-project/llvm/unittests/Support/LinearPolyBaseTest.cpp:173:13:
 note: used here
EXPECT_EQ(-Univariate3D(4, 0), Univariate3D(-4, 0));
  ^
  1 warning generated.



  Undefined symbols for architecture x86_64:
"std::__1::enable_if::value, Univariate3D>::type 
llvm::operator-(Univariate3D const&)", referenced from:
UnivariateLinearPolyBase_Univariate3D_Invert_Test::TestBody() in lto.o
  ld: symbol(s) not found for architecture x86_64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D139956: Workaround an assertion failure during module build

[Steven Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe77e14ecf17b: Workaround an assertion failure during module 
build (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139956

Files:
  llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
  llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
  llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h


Index: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
+++ llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
@@ -13,7 +13,6 @@
 #ifndef LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 #define LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 
-#include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/Speculation.h"
 
@@ -21,6 +20,8 @@
 
 namespace llvm {
 
+class BranchProbabilityInfo;
+
 namespace orc {
 
 // Provides common code.
Index: llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
+++ llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
@@ -18,10 +18,11 @@
 #include "llvm/ExecutionEngine/JITSymbol.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/OrcABISupport.h"
+#include "llvm/IR/ValueHandle.h"
+#include "llvm/IR/ValueMap.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Process.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 
@@ -47,6 +48,9 @@
 class Value;
 class MCDisassembler;
 class MCInstrAnalysis;
+class ValueMaterializer;
+
+using ValueToValueMapTy = ValueMap;
 
 namespace jitlink {
 class LinkGraph;
Index: llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
+++ llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
@@ -39,7 +39,6 @@
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 


Index: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
+++ llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
@@ -13,7 +13,6 @@
 #ifndef LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 #define LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 
-#include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/Speculation.h"
 
@@ -21,6 +20,8 @@
 
 namespace llvm {
 
+class BranchProbabilityInfo;
+
 namespace orc {
 
 // Provides common code.
Index: llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
+++ llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
@@ -18,10 +18,11 @@
 #include "llvm/ExecutionEngine/JITSymbol.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/OrcABISupport.h"
+#include "llvm/IR/ValueHandle.h"
+#include "llvm/IR/ValueMap.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Process.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 
@@ -47,6 +48,9 @@
 class Value;
 class MCDisassembler;
 class MCInstrAnalysis;
+class ValueMaterializer;
+
+using ValueToValueMapTy = ValueMap;
 
 namespace jitlink {
 class LinkGraph;
Index: llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
+++ llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
@@ -39,7 +39,6 @@
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D139956: Workaround an assertion failure during module build

[Steven Wu via Phabricator via cfe-commits]

steven_wu created this revision.
steven_wu added reviewers: mizvekov, arphaman.
Herald added a subscriber: ributzka.
Herald added a project: All.
steven_wu requested review of this revision.
Herald added a project: LLVM.

After the change in https://reviews.llvm.org/D131858, clang cannot
bootstrap itself with modules due to assertion failure:
(lvaluePath->getType() == elemTy && "Unexpected type reference!")

Workaround the assertion by converting some of the includes into forward
declares.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139956

Files:
  llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
  llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
  llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h


Index: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
+++ llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
@@ -13,7 +13,6 @@
 #ifndef LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 #define LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 
-#include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/Speculation.h"
 
@@ -21,6 +20,8 @@
 
 namespace llvm {
 
+class BranchProbabilityInfo;
+
 namespace orc {
 
 // Provides common code.
Index: llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
+++ llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
@@ -18,10 +18,11 @@
 #include "llvm/ExecutionEngine/JITSymbol.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/OrcABISupport.h"
+#include "llvm/IR/ValueHandle.h"
+#include "llvm/IR/ValueMap.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Process.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 
@@ -47,6 +48,9 @@
 class Value;
 class MCDisassembler;
 class MCInstrAnalysis;
+class ValueMaterializer;
+
+using ValueToValueMapTy = ValueMap;
 
 namespace jitlink {
 class LinkGraph;
Index: llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
+++ llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
@@ -39,7 +39,6 @@
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 


Index: llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
+++ llvm/include/llvm/ExecutionEngine/Orc/SpeculateAnalyses.h
@@ -13,7 +13,6 @@
 #ifndef LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 #define LLVM_EXECUTIONENGINE_ORC_SPECULATEANALYSES_H
 
-#include "llvm/Analysis/BranchProbabilityInfo.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/Speculation.h"
 
@@ -21,6 +20,8 @@
 
 namespace llvm {
 
+class BranchProbabilityInfo;
+
 namespace orc {
 
 // Provides common code.
Index: llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
+++ llvm/include/llvm/ExecutionEngine/Orc/IndirectionUtils.h
@@ -18,10 +18,11 @@
 #include "llvm/ExecutionEngine/JITSymbol.h"
 #include "llvm/ExecutionEngine/Orc/Core.h"
 #include "llvm/ExecutionEngine/Orc/OrcABISupport.h"
+#include "llvm/IR/ValueHandle.h"
+#include "llvm/IR/ValueMap.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/Memory.h"
 #include "llvm/Support/Process.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 
@@ -47,6 +48,9 @@
 class Value;
 class MCDisassembler;
 class MCInstrAnalysis;
+class ValueMaterializer;
+
+using ValueToValueMapTy = ValueMap;
 
 namespace jitlink {
 class LinkGraph;
Index: llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
===
--- llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
+++ llvm/include/llvm/ExecutionEngine/Orc/CompileOnDemandLayer.h
@@ -39,7 +39,6 @@
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Casting.h"
 #include "llvm/Support/raw_ostream.h"
-#include "llvm/Transforms/Utils/ValueMapper.h"
 #include 
 #include 
 #include 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D131858: [clang] Track the templated entity in type substitution.

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

> Can you tell if this has been fixed by 
> https://github.com/llvm/llvm-project/commit/303f20a2cab4554a10ec225fd853709146f8c51c
>  ?

I can still reproduce this with latest commit. It is not fixed. At least it can 
easily reproduce on Darwin platform with bootstrap (stage 2 with module 
enabled).

Looking at the debugger output:

  (lldb) p lvaluePath->getType().dump()
  FunctionProtoType 0x1424badb0 'void (const unsigned long long &, 
llvm::raw_ostream &, StringRef)' imported cdecl
  |-BuiltinType 0x12886e870 'void'
  |-LValueReferenceType 0x1424bad80 'const unsigned long long &' imported
  | `-QualType 0x1424bad21 'const unsigned long long' const
  |   `-SubstTemplateTypeParmType 0x1424bad20 'unsigned long long' sugar 
imported typename depth 0 index 0 T
  | |-ClassTemplateSpecialization 0x1424bab70 'format_provider'
  | `-BuiltinType 0x12886e9f0 'unsigned long long'
  |-LValueReferenceType 0x13926fd00 'llvm::raw_ostream &' imported
  | `-ElaboratedType 0x13926fcd0 'llvm::raw_ostream' sugar imported
  |   `-RecordType 0x128c396a0 'class llvm::raw_ostream' imported
  | `-CXXRecord 0x128c39758 'raw_ostream'
  `-ElaboratedType 0x128bf6c20 'StringRef' sugar imported
`-RecordType 0x128bf6c00 'class llvm::StringRef' imported
  `-CXXRecord 0x128c02570 'StringRef'
  
  (lldb) p elemTy.dump()
  FunctionProtoType 0x1392702d0 'void (const unsigned long long &, 
llvm::raw_ostream &, StringRef)' cdecl
  |-BuiltinType 0x12886e870 'void'
  |-LValueReferenceType 0x139270140 'const unsigned long long &'
  | `-QualType 0x139270111 'const unsigned long long' const
  |   `-SubstTemplateTypeParmType 0x139270110 'unsigned long long' sugar 
typename depth 0 index 0 T
  | |-ClassTemplateSpecialization 0x13924aed8 'format_provider'
  | `-BuiltinType 0x12886e9f0 'unsigned long long'
  |-LValueReferenceType 0x13926fd00 'llvm::raw_ostream &' imported
  | `-ElaboratedType 0x13926fcd0 'llvm::raw_ostream' sugar imported
  |   `-RecordType 0x128c396a0 'class llvm::raw_ostream' imported
  | `-CXXRecord 0x128c39758 'raw_ostream'
  `-ElaboratedType 0x128bf6c20 'StringRef' sugar imported
`-RecordType 0x128bf6c00 'class llvm::StringRef' imported
  `-CXXRecord 0x128c02570 'StringRef'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131858

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

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1729
+  if (!Opts.ProfileInstrumentUsePath.empty()) {
+auto FS = llvm::vfs::getRealFileSystem();
+setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath, *FS, Diags);

akyrtzi wrote:
> steven_wu wrote:
> > akyrtzi wrote:
> > > Could we propagate the VFS that the `CompilerInvocation` is going to 
> > > create here? Otherwise this seems like a hidden "landmine" that someone 
> > > is bound to trip on later on.
> > Currently, Profile look up doesn't go through any VFS, so vfs overlay has 
> > no effect on profiles. Putting through VFS is changing behavior, even I 
> > think it is for good.
> > 
> > It also has the potential to make code harder to read because creating VFS 
> > relies on a compiler invocation which we are creating here.
> > 
> > Let me add a fixme here first. 
> > Currently, Profile look up doesn't go through any VFS, so vfs overlay has 
> > no effect on profiles. Putting through VFS is changing behavior, even I 
> > think it is for good.
> 
> Your patch is already changing behavior; CodeGen will load profiles using 
> clang's VFS, so vfs overlay will affect how profiles are loaded. The mismatch 
> is that CodeGen will load the path using clang's VFS but 
> `setPGOUseInstrumentor` will load it directly from real file-system, so they 
> can be out-of-sync.
> 
> On second look, `setPGOUseInstrumentor` seems to create a throw-away 
> `IndexedInstrProfReader` just for reading a couple of settings to set some 
> flags. This seems redundant, could we move the determination of these flags 
> at the point where `CodeGenModule` is creating its own 
> `IndexedInstrProfReader` and remove `setPGOUseInstrumentor` from 
> `CompilerInvocation::ParseCodeGenArgs` entirely?
I will take a look. The current implementation is not very good as it just read 
it to determine the type of the profile to set the action. I agree the 
disconnect between two reads is bad.



Comment at: llvm/include/llvm/Support/PGOOptions.h:18
 #include "llvm/Support/Error.h"
+#include "llvm/Support/VirtualFileSystem.h"
 

akyrtzi wrote:
> akyrtzi wrote:
> > I'd suggest to consider moving the `PGOOptions` constructor out-of-line, 
> > along with
> > ```
> >   PGOOptions =(const PGOOptions );
> >   PGOOptions(const PGOOptions&);
> >   ~PGOOptions();
> > ```
> >  in order to forward-declare here and avoid including the header, avoiding 
> > the additional include dependencies for many cpp files.
> > The default parameter needs to instantiate here and pretty much all 
> > references to `PGOOptions` has `Optional` which requires 
> > including VFS. I will leave this one since `PGOOptions.h` is a rare header 
> > to include.
> 
> `PGOOptions.h` is transitively included by many files, if I touch that 
> header, and then compile `clang`, there are 225 files that need to be 
> re-compiled.
> 
> I thought that moving the `PGOOptions` members I mentioned out-of-line would 
> be enough to avoid the need to include `VirtualFileSystem.h`, is this not the 
> case?
I see. It is included in two different headers in the backend, both of them has 
`Optional`. During my experiment, to instantiate 
`Optional`, it needs full declaration for PGOOptions, thus 
FileSystem. I could be wrong but I don't see a way around that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

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

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:164
   LLVMIRGenerationRefCount(0),
-  Gen(CreateLLVMCodeGen(Diags, InFile, std::move(FS), HeaderSearchOpts,
-PPOpts, CodeGenOpts, C, CoverageInfo)),
+  Gen(CreateLLVMCodeGen(Diags, InFile, FS, HeaderSearchOpts, PPOpts,
+CodeGenOpts, C, CoverageInfo)),

benlangmuir wrote:
> Wouldn't `move` be fine here since it's already copied to `this->FS`?
I think I confused myself for which `FS` is moved here. I renamed the parameter 
so it is clear move is applied to the FileSystem passed in.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1729
+  if (!Opts.ProfileInstrumentUsePath.empty()) {
+auto FS = llvm::vfs::getRealFileSystem();
+setPGOUseInstrumentor(Opts, Opts.ProfileInstrumentUsePath, *FS, Diags);

akyrtzi wrote:
> Could we propagate the VFS that the `CompilerInvocation` is going to create 
> here? Otherwise this seems like a hidden "landmine" that someone is bound to 
> trip on later on.
Currently, Profile look up doesn't go through any VFS, so vfs overlay has no 
effect on profiles. Putting through VFS is changing behavior, even I think it 
is for good.

It also has the potential to make code harder to read because creating VFS 
relies on a compiler invocation which we are creating here.

Let me add a fixme here first. 



Comment at: llvm/include/llvm/CodeGen/MIRSampleProfile.h:20
 #include "llvm/Support/Discriminator.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include 

akyrtzi wrote:
> Recommend to forward declare instead of including the header.
The default parameter needs to instantiate here and pretty much all references 
to `PGOOptions` has `Optional` which requires including VFS. I will 
leave this one since `PGOOptions.h` is a rare header to include.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

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

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 480990.
steven_wu marked 13 inline comments as done.
steven_wu added a comment.

Address review feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139052

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/SampleProfReader.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/IPO/SampleProfile.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/SampleProfReader.cpp
  llvm/lib/Target/X86/X86InsertPrefetch.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-profgen/llvm-profgen.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/ProfileData/SampleProfTest.cpp

Index: llvm/unittests/ProfileData/SampleProfTest.cpp
===
--- llvm/unittests/ProfileData/SampleProfTest.cpp
+++ llvm/unittests/ProfileData/SampleProfTest.cpp
@@ -19,6 +19,7 @@
 #include "llvm/Support/ErrorOr.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Testing/Support/SupportHelpers.h"
 #include "gtest/gtest.h"
@@ -57,8 +58,9 @@
 
   void readProfile(const Module , StringRef Profile,
StringRef RemapFile = "") {
+auto FS = vfs::getRealFileSystem();
 auto ReaderOrErr = SampleProfileReader::create(
-std::string(Profile), Context, FSDiscriminatorPass::Base,
+std::string(Profile), Context, *FS, FSDiscriminatorPass::Base,
 std::string(RemapFile));
 ASSERT_TRUE(NoError(ReaderOrErr.getError()));
 Reader = std::move(ReaderOrErr.get());
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
@@ -333,22 +334,25 @@
bool EnableDebugify, bool VerifyDIPreserve) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
+  auto FS = vfs::getRealFileSystem();
   Optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
+   PGOOptions::IRUse);
 break;
   case SampleUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile,
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
PGOOptions::SampleUse);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
-  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
- DebugInfoForProfiling, PseudoProbeForProfiling);
+  P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
+ PGOOptions::NoCSAction, DebugInfoForProfiling,
+ PseudoProbeForProfiling);
 else
   P = None;
   }
@@ -363,7 +367,7 @@
 P->CSAction = PGOOptions::CSIRInstr;
 P->CSProfileGenFile = CSProfileGenFile;
   } else
-P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile,
+P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
PGOOptions::NoAction, PGOOptions::CSIRInstr);
 } else /* CSPGOKindFlag == CSInstrUse */ {
   if (!P)
Index: llvm/tools/llvm-profgen/llvm-profgen.cpp

[PATCH] D139052: [NFC][Profile] Access profile through VirtualFileSystem

[Steven Wu via Phabricator via cfe-commits]

steven_wu created this revision.
steven_wu added reviewers: davidxl, tejohnson, xur, akyrtzi.
Herald added subscribers: Enna1, ormris, wenlei, ributzka, pengfei, hiraditya.
Herald added a project: All.
steven_wu requested review of this revision.
Herald added projects: clang, LLVM.
Herald added a subscriber: cfe-commits.

Make the access to profile data going through virtual file system so the
inputs can be remapped. In the context of the caching, it can make sure
we capture the inputs and provided an immutable input as profile data.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D139052

Files:
  clang/include/clang/CodeGen/BackendUtil.h
  clang/lib/CodeGen/BackendUtil.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  llvm/include/llvm/CodeGen/MIRSampleProfile.h
  llvm/include/llvm/CodeGen/Passes.h
  llvm/include/llvm/Passes/PassBuilder.h
  llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
  llvm/include/llvm/ProfileData/InstrProf.h
  llvm/include/llvm/ProfileData/InstrProfReader.h
  llvm/include/llvm/ProfileData/SampleProfReader.h
  llvm/include/llvm/Support/PGOOptions.h
  llvm/include/llvm/Transforms/IPO/SampleProfile.h
  llvm/include/llvm/Transforms/Instrumentation/PGOInstrumentation.h
  llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h
  llvm/lib/CodeGen/MIRSampleProfile.cpp
  llvm/lib/CodeGen/TargetPassConfig.cpp
  llvm/lib/LTO/LTOBackend.cpp
  llvm/lib/Passes/PassBuilderPipelines.cpp
  llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
  llvm/lib/ProfileData/InstrProf.cpp
  llvm/lib/ProfileData/InstrProfReader.cpp
  llvm/lib/ProfileData/SampleProfReader.cpp
  llvm/lib/Target/X86/X86InsertPrefetch.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp
  llvm/tools/llvm-cov/CodeCoverage.cpp
  llvm/tools/llvm-profdata/llvm-profdata.cpp
  llvm/tools/llvm-profgen/llvm-profgen.cpp
  llvm/tools/opt/NewPMDriver.cpp
  llvm/unittests/ProfileData/SampleProfTest.cpp

Index: llvm/unittests/ProfileData/SampleProfTest.cpp
===
--- llvm/unittests/ProfileData/SampleProfTest.cpp
+++ llvm/unittests/ProfileData/SampleProfTest.cpp
@@ -57,8 +57,9 @@
 
   void readProfile(const Module , StringRef Profile,
StringRef RemapFile = "") {
+auto FS = vfs::getRealFileSystem();
 auto ReaderOrErr = SampleProfileReader::create(
-std::string(Profile), Context, FSDiscriminatorPass::Base,
+std::string(Profile), Context, *FS, FSDiscriminatorPass::Base,
 std::string(RemapFile));
 ASSERT_TRUE(NoError(ReaderOrErr.getError()));
 Reader = std::move(ReaderOrErr.get());
Index: llvm/tools/opt/NewPMDriver.cpp
===
--- llvm/tools/opt/NewPMDriver.cpp
+++ llvm/tools/opt/NewPMDriver.cpp
@@ -31,6 +31,7 @@
 #include "llvm/Passes/StandardInstrumentations.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO/ThinLTOBitcodeWriter.h"
@@ -333,22 +334,25 @@
bool EnableDebugify, bool VerifyDIPreserve) {
   bool VerifyEachPass = VK == VK_VerifyEachPass;
 
+  auto FS = vfs::getRealFileSystem();
   Optional P;
   switch (PGOKindFlag) {
   case InstrGen:
-P = PGOOptions(ProfileFile, "", "", PGOOptions::IRInstr);
+P = PGOOptions(ProfileFile, "", "", FS, PGOOptions::IRInstr);
 break;
   case InstrUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile, PGOOptions::IRUse);
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
+   PGOOptions::IRUse);
 break;
   case SampleUse:
-P = PGOOptions(ProfileFile, "", ProfileRemappingFile,
+P = PGOOptions(ProfileFile, "", ProfileRemappingFile, FS,
PGOOptions::SampleUse);
 break;
   case NoPGO:
 if (DebugInfoForProfiling || PseudoProbeForProfiling)
-  P = PGOOptions("", "", "", PGOOptions::NoAction, PGOOptions::NoCSAction,
- DebugInfoForProfiling, PseudoProbeForProfiling);
+  P = PGOOptions("", "", "", nullptr, PGOOptions::NoAction,
+ PGOOptions::NoCSAction, DebugInfoForProfiling,
+ PseudoProbeForProfiling);
 else
   P = None;
   }
@@ -363,7 +367,7 @@
 P->CSAction = PGOOptions::CSIRInstr;
 P->CSProfileGenFile = CSProfileGenFile;
   } else
-P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile,
+P = PGOOptions("", CSProfileGenFile, ProfileRemappingFile, FS,
PGOOptions::NoAction, PGOOptions::CSIRInstr);
 } else /* CSPGOKindFlag == CSInstrUse */ {
   if (!P)
Index: 

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Steven Wu via Phabricator via cfe-commits]

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

LGTM except the comments from @MaskRay


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

https://reviews.llvm.org/D136888

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

[PATCH] D136888: Move getenv for AS_SECURE_LOG_FILE to clang

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

If we have to fix it, I slightly prefer just give it a proper option in 
CodeGenOption and a `cc1` and `cc1as` flag.




Comment at: llvm/lib/MC/MCContext.cpp:62
 
-static cl::opt
+static cl::opt
 AsSecureLogFileName("as-secure-log-file-name",

benlangmuir wrote:
> Interestingly, `opt` doesn't have a parser; this code only worked with 
> the default value before.
ha, maybe we should just deprecate the directive since this hasn't been working 
for more than 6 years!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136888

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

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

> Yes, we can. I pondered doing it in the original patch but didn't see a 
> reason this would evolve in the future. Of course, I can't predict the 
> future, so maybe a version field is the safe approach.

I think a version number should be good enough for now. For the portability, it 
is probably not too big a concern. We can add that in future versions if it is 
needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651

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

[PATCH] D136651: [Clang] Give Clang the ability to use a shared stat cache

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: llvm/lib/Support/VirtualFileSystem.cpp:3002
+// The format of the stat cache is (pseudo-code):
+//  struct stat_cache {
+//char Magic[4];   // "STAT" or "Stat"

Few comments about stats representation.
1. Can we version the stat cache file so we can evolve it in the future if 
needed?
2. I wonder if we need to have a more flexible representation for DataType 
other than `sys::fs::file_status`. Current entry is locked with the endian of 
the host and can't be used to encode more information than file_status. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136651

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG4ba360d499f6: [CMake] Fix FindGRPC cmake module to allow 
different layering (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  cmake/Modules/FindGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+function(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,11 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  # Take the first two args and forward the remaining to 
generate_proto_sources.
+  cmake_parse_arguments(PARSE_ARGV 2 PROTO "" "" "")
+  generate_proto_sources(ProtoSource ${ProtoFile} ${PROTO_UNPARSED_ARGUMENTS})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+function(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ 

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 467247.
steven_wu added a comment.

Turn generate_proto_sources back to function and forward argument more 
elegantly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  cmake/Modules/FindGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+function(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,11 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  # Take the first two args and forward the remaining to 
generate_proto_sources.
+  cmake_parse_arguments(PARSE_ARGV 2 PROTO "" "" "")
+  generate_proto_sources(ProtoSource ${ProtoFile} ${PROTO_UNPARSED_ARGUMENTS})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+function(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,11 @@
+include(FindGRPC)
+

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: cmake/Modules/FindGRPC.cmake:111
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")

sammccall wrote:
> steven_wu wrote:
> > sammccall wrote:
> > > sammccall wrote:
> > > > changing from function to macro means all the local variables here are 
> > > > done in the parent scope, which is hard to reason about.
> > > > 
> > > > I think the only reason you're doing that here is to return the 
> > > > filename in the `ProtoSource` variable, in which case 
> > > > `set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)` should 
> > > > do the job from a function.
> > > nit: the plural is in the wrong place: rather `generate_proto_sources`?
> > > this generates the multiple sources for a single proto, rather than the 
> > > other way around.
> > > 
> > > (I'm not sure why the original was plural, but this makes it somehow more 
> > > confusing)
> > The reason why it is a macro is I don't know any elegant way to pass all 
> > the DEPEND down to `add_*_library`.
> > The reason why it is a macro is I don't know any elegant way to pass all 
> > the DEPEND down to add_*_library.
> 
> ... oh, yikes, we're passing through by pretending they're `${ProtoFile}`, I 
> didn't see that :-\
> 
> I guess the easiest option is just to change that name to something like 
> `ProtoArgs` in the wrapping function, and add a bit of documentation.
> I think using CMAKE_PARSE_ARGUMENTS in the wrapping function is an option 
> though, and then pass the unparsed arguments through?
Ah, I just learnt the unparsed_argument option. Then it is not too bad. Fixing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 467243.
steven_wu added a comment.

Minor rename


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  cmake/Modules/FindGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,4 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
-endfunction()
+endmacro()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,9 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  generate_proto_sources(ProtoSource ${ProtoFile})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_proto_sources GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,4 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
-endfunction()
+endmacro()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,9 

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu added inline comments.



Comment at: cmake/Modules/FindGRPC.cmake:111
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")

sammccall wrote:
> sammccall wrote:
> > changing from function to macro means all the local variables here are done 
> > in the parent scope, which is hard to reason about.
> > 
> > I think the only reason you're doing that here is to return the filename in 
> > the `ProtoSource` variable, in which case `set(${GeneratedSource} 
> > ${GeneratedProtoSource} PARENT_SCOPE)` should do the job from a function.
> nit: the plural is in the wrong place: rather `generate_proto_sources`?
> this generates the multiple sources for a single proto, rather than the other 
> way around.
> 
> (I'm not sure why the original was plural, but this makes it somehow more 
> confusing)
The reason why it is a macro is I don't know any elegant way to pass all the 
DEPEND down to `add_*_library`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 467217.
steven_wu added a comment.

Was missing the latest feedback. Bump FindGRPC module to top layer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  cmake/Modules/FindGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,4 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
-endfunction()
+endmacro()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,9 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  generate_protos_source(ProtoSource ${ProtoFile})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: cmake/Modules/FindGRPC.cmake
===
--- cmake/Modules/FindGRPC.cmake
+++ cmake/Modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos_source GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,4 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
-endfunction()
+endmacro()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- 

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 467215.
steven_wu added a comment.
Herald added a subscriber: bmahjour.
Herald added a project: clang.

Address review feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/CMakeLists.txt
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  clang/cmake/modules/AddGRPC.cmake
  llvm/cmake/modules/FindGRPC.cmake


Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
@@ -154,4 +152,13 @@
 PROPERTIES OBJECT_DEPENDS "${ImportedHeader}")
 endforeach(Generated)
   endforeach(ImportedProto)
+endmacro()
+
+function(generate_llvm_protos_library LibraryName ProtoFile)
+  cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
+  generate_protos(ProtoSource ${ProtoFile})
+
+  add_llvm_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
 endfunction()
Index: clang/cmake/modules/AddGRPC.cmake
===
--- /dev/null
+++ clang/cmake/modules/AddGRPC.cmake
@@ -0,0 +1,9 @@
+include(FindGRPC)
+
+function(generate_clang_protos_library LibraryName ProtoFile)
+  generate_protos(ProtoSource ${ProtoFile})
+
+  add_clang_library(${LibraryName} ${ProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+endfunction()
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,8 +1,8 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_clang_protos_library(clangdRemoteIndexProto "Index.proto")
+  generate_clang_protos_library(clangdMonitoringServiceProto 
"MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_clang_protos_library(clangdRemoteIndexServiceProto "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
   # FIXME: Move this into generate_protos. Currently we only mention proto
Index: clang-tools-extra/clangd/CMakeLists.txt
===
--- clang-tools-extra/clangd/CMakeLists.txt
+++ clang-tools-extra/clangd/CMakeLists.txt
@@ -194,7 +194,7 @@
 endif ()
 
 if (CLANGD_ENABLE_REMOTE)
-  include(FindGRPC)
+  include(AddGRPC)
 endif()
 
 if(CLANG_INCLUDE_TESTS)


Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+macro(generate_protos GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource})
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're 

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

[Steven Wu via Phabricator via cfe-commits]

steven_wu created this revision.
steven_wu added reviewers: sammccall, akyrtzi, rymiel.
Herald added subscribers: ributzka, kadircet, arphaman.
Herald added a project: All.
steven_wu requested review of this revision.
Herald added projects: LLVM, clang-tools-extra.
Herald added a subscriber: cfe-commits.

Take the library target out of `generate_protos` function so the caller
can decide where to layer the library using the source generated from
the function.

Fixes: https://github.com/llvm/llvm-project/issues/58075


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135712

Files:
  clang-tools-extra/clangd/index/remote/CMakeLists.txt
  llvm/cmake/modules/FindGRPC.cmake


Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+function(generate_protos GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath 
"${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- clang-tools-extra/clangd/index/remote/CMakeLists.txt
+++ clang-tools-extra/clangd/index/remote/CMakeLists.txt
@@ -1,17 +1,32 @@
 if (CLANGD_ENABLE_REMOTE)
-  generate_protos(clangdRemoteIndexProto "Index.proto")
-  generate_protos(clangdMonitoringServiceProto "MonitoringService.proto"
+  generate_protos(clangdRemoteIndexProtoSource "Index.proto")
+  generate_protos(clangdMonitoringServiceProtoSource "MonitoringService.proto"
 GRPC)
-  generate_protos(clangdRemoteIndexServiceProto "Service.proto"
+  generate_protos(clangdRemoteIndexServiceProtoSource "Service.proto"
 DEPENDS "Index.proto"
 GRPC)
-  # FIXME: Move this into generate_protos. Currently we only mention proto
-  # filename as a dependency, but linking requires target name.
-  target_link_libraries(clangdRemoteIndexServiceProto
+
+  add_clang_library(clangdRemoteIndexProto ${clangdRemoteIndexProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+  add_clang_library(clangdMonitoringServiceProto 
${clangdMonitoringServiceProtoSource}
+PARTIAL_SOURCES_INTENDED
+LINK_LIBS PUBLIC grpc++ protobuf)
+
+  add_clang_library(clangdRemoteIndexServiceProto
+${clangdRemoteIndexServiceProtoSource}
+PARTIAL_SOURCES_INTENDED
+
+LINK_LIBS
+PUBLIC
+grpc++
+protobuf
+
 PRIVATE
 clangdRemoteIndexProto
 clangdMonitoringServiceProto
 )
+
   include_directories(${CMAKE_CURRENT_BINARY_DIR})
 
   # FIXME(kirillbobyrev): target_compile_definitions is not working with


Index: llvm/cmake/modules/FindGRPC.cmake
===
--- llvm/cmake/modules/FindGRPC.cmake
+++ llvm/cmake/modules/FindGRPC.cmake
@@ -108,7 +108,7 @@
 # If the "GRPC" argument is given, services are also generated.
 # The DEPENDS list should name *.proto source files that are imported.
 # They may be relative to the source dir or absolute (for generated protos).
-function(generate_protos LibraryName ProtoFile)
+function(generate_protos GeneratedSource ProtoFile)
   cmake_parse_arguments(PARSE_ARGV 2 PROTO "GRPC" "" "DEPENDS")
   get_filename_component(ProtoSourceAbsolutePath "${CMAKE_CURRENT_SOURCE_DIR}/${ProtoFile}" ABSOLUTE)
   get_filename_component(ProtoSourcePath ${ProtoSourceAbsolutePath} PATH)
@@ -132,9 +132,7 @@
 ARGS ${Flags} "${ProtoSourceAbsolutePath}"
 DEPENDS "${ProtoSourceAbsolutePath}")
 
-  add_llvm_library(${LibraryName} ${GeneratedProtoSource}
-PARTIAL_SOURCES_INTENDED
-LINK_LIBS PUBLIC grpc++ protobuf)
+  set(${GeneratedSource} ${GeneratedProtoSource} PARENT_SCOPE)
 
   # Ensure dependency headers are generated before dependent protos are built.
   # DEPENDS arg is a list of "Foo.proto". While they're logically relative to
Index: clang-tools-extra/clangd/index/remote/CMakeLists.txt
===
--- 

[PATCH] D135490: [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic

[Steven Wu via Phabricator via cfe-commits]

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

Ok with me


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135490

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

[PATCH] D135490: [clang/Sema] Follow-up for fix of non-deterministic order of `-Wunused-variable` diagnostic

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I don't know too much about this. I guess have a DiagReceiver to force the 
emitting order is a good thing but it is kind of weird to have this just for 
unused warning.

I am guessing my suspect of removing decl from the scope is the cause of the 
slow down. Maybe we just force order on iteration?




Comment at: clang/include/clang/Sema/Scope.h:310
 
   decl_range decls() const {
 return decl_range(DeclsInScope.begin(), DeclsInScope.end());

I wonder if it would be easy if you just sort here and provide a consistent 
ordering when iterating?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135490

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

[PATCH] D135118: [clang/Sema] Fix non-deterministic order for certain kind of diagnostics

[Steven Wu via Phabricator via cfe-commits]

steven_wu accepted this revision.
steven_wu added a comment.

LGTM.

`RemoveDecl` does become more expensive but I don't have better solution. I am 
also wondering if as follow up we should add an option to 
`VerifyDiagnosticConsumer` to be location aware (so the diagnostics from a file 
is emitted in order) to prevent more problem like this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135118

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

[PATCH] D95497: Frontend: Respect -working-directory when checking if output files can be written

[Steven Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG493766e06847: Frontend: Respect -working-directory when 
checking if output files can be… (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95497

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Frontend/output-paths.c


Index: clang/test/Frontend/output-paths.c
===
--- clang/test/Frontend/output-paths.c
+++ clang/test/Frontend/output-paths.c
@@ -2,3 +2,12 @@
 // RUN: FileCheck -check-prefix=OUTPUTFAIL -DMSG=%errc_ENOENT -input-file=%t %s
 
 // OUTPUTFAIL: error: unable to open output file 
'{{.*}}doesnotexist{{.}}somename': '[[MSG]]'
+
+// Check that -working-directory is respected when diagnosing output failures.
+//
+// RUN: rm -rf %t.d && mkdir -p %t.d/%basename_t-inner.d
+// RUN: %clang_cc1 -emit-llvm -working-directory %t.d -E -o 
%basename_t-inner.d/somename %s -verify
+// expected-no-diagnostics
+
+// RUN: %clang_cc1 -working-directory %t.d -E %s -o - | FileCheck %s
+// CHECK: # 1
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -781,12 +781,7 @@
   continue;
 }
 
-// If '-working-directory' was passed, the output filename should be
-// relative to that.
-SmallString<128> NewOutFile(OF.Filename);
-FileMgr->FixupRelativePath(NewOutFile);
-
-llvm::Error E = OF.File->keep(NewOutFile);
+llvm::Error E = OF.File->keep(OF.Filename);
 if (!E)
   continue;
 
@@ -849,6 +844,15 @@
   assert((!CreateMissingDirectories || UseTemporary) &&
  "CreateMissingDirectories is only allowed when using temporary 
files");
 
+  // If '-working-directory' was passed, the output filename should be
+  // relative to that.
+  Optional> AbsPath;
+  if (OutputPath != "-" && !llvm::sys::path::is_absolute(OutputPath)) {
+AbsPath.emplace(OutputPath);
+FileMgr->FixupRelativePath(*AbsPath);
+OutputPath = *AbsPath;
+  }
+
   std::unique_ptr OS;
   Optional OSFile;
 


Index: clang/test/Frontend/output-paths.c
===
--- clang/test/Frontend/output-paths.c
+++ clang/test/Frontend/output-paths.c
@@ -2,3 +2,12 @@
 // RUN: FileCheck -check-prefix=OUTPUTFAIL -DMSG=%errc_ENOENT -input-file=%t %s
 
 // OUTPUTFAIL: error: unable to open output file '{{.*}}doesnotexist{{.}}somename': '[[MSG]]'
+
+// Check that -working-directory is respected when diagnosing output failures.
+//
+// RUN: rm -rf %t.d && mkdir -p %t.d/%basename_t-inner.d
+// RUN: %clang_cc1 -emit-llvm -working-directory %t.d -E -o %basename_t-inner.d/somename %s -verify
+// expected-no-diagnostics
+
+// RUN: %clang_cc1 -working-directory %t.d -E %s -o - | FileCheck %s
+// CHECK: # 1
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -781,12 +781,7 @@
   continue;
 }
 
-// If '-working-directory' was passed, the output filename should be
-// relative to that.
-SmallString<128> NewOutFile(OF.Filename);
-FileMgr->FixupRelativePath(NewOutFile);
-
-llvm::Error E = OF.File->keep(NewOutFile);
+llvm::Error E = OF.File->keep(OF.Filename);
 if (!E)
   continue;
 
@@ -849,6 +844,15 @@
   assert((!CreateMissingDirectories || UseTemporary) &&
  "CreateMissingDirectories is only allowed when using temporary files");
 
+  // If '-working-directory' was passed, the output filename should be
+  // relative to that.
+  Optional> AbsPath;
+  if (OutputPath != "-" && !llvm::sys::path::is_absolute(OutputPath)) {
+AbsPath.emplace(OutputPath);
+FileMgr->FixupRelativePath(*AbsPath);
+OutputPath = *AbsPath;
+  }
+
   std::unique_ptr OS;
   Optional OSFile;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95497: Frontend: Respect -working-directory when checking if output files can be written

[Steven Wu via Phabricator via cfe-commits]

steven_wu updated this revision to Diff 458823.
steven_wu edited the summary of this revision.
steven_wu added a comment.

Rebase patch and fix the problem when the input is '-'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95497

Files:
  clang/lib/Frontend/CompilerInstance.cpp
  clang/test/Frontend/output-paths.c


Index: clang/test/Frontend/output-paths.c
===
--- clang/test/Frontend/output-paths.c
+++ clang/test/Frontend/output-paths.c
@@ -2,3 +2,12 @@
 // RUN: FileCheck -check-prefix=OUTPUTFAIL -DMSG=%errc_ENOENT -input-file=%t %s
 
 // OUTPUTFAIL: error: unable to open output file 
'{{.*}}doesnotexist{{.}}somename': '[[MSG]]'
+
+// Check that -working-directory is respected when diagnosing output failures.
+//
+// RUN: rm -rf %t.d && mkdir -p %t.d/%basename_t-inner.d
+// RUN: %clang_cc1 -emit-llvm -working-directory %t.d -E -o 
%basename_t-inner.d/somename %s -verify
+// expected-no-diagnostics
+
+// RUN: %clang_cc1 -working-directory %t.d -E %s -o - | FileCheck %s
+// CHECK: # 1
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -781,12 +781,7 @@
   continue;
 }
 
-// If '-working-directory' was passed, the output filename should be
-// relative to that.
-SmallString<128> NewOutFile(OF.Filename);
-FileMgr->FixupRelativePath(NewOutFile);
-
-llvm::Error E = OF.File->keep(NewOutFile);
+llvm::Error E = OF.File->keep(OF.Filename);
 if (!E)
   continue;
 
@@ -849,6 +844,15 @@
   assert((!CreateMissingDirectories || UseTemporary) &&
  "CreateMissingDirectories is only allowed when using temporary 
files");
 
+  // If '-working-directory' was passed, the output filename should be
+  // relative to that.
+  Optional> AbsPath;
+  if (OutputPath != "-" && !llvm::sys::path::is_absolute(OutputPath)) {
+AbsPath.emplace(OutputPath);
+FileMgr->FixupRelativePath(*AbsPath);
+OutputPath = *AbsPath;
+  }
+
   std::unique_ptr OS;
   Optional OSFile;
 


Index: clang/test/Frontend/output-paths.c
===
--- clang/test/Frontend/output-paths.c
+++ clang/test/Frontend/output-paths.c
@@ -2,3 +2,12 @@
 // RUN: FileCheck -check-prefix=OUTPUTFAIL -DMSG=%errc_ENOENT -input-file=%t %s
 
 // OUTPUTFAIL: error: unable to open output file '{{.*}}doesnotexist{{.}}somename': '[[MSG]]'
+
+// Check that -working-directory is respected when diagnosing output failures.
+//
+// RUN: rm -rf %t.d && mkdir -p %t.d/%basename_t-inner.d
+// RUN: %clang_cc1 -emit-llvm -working-directory %t.d -E -o %basename_t-inner.d/somename %s -verify
+// expected-no-diagnostics
+
+// RUN: %clang_cc1 -working-directory %t.d -E %s -o - | FileCheck %s
+// CHECK: # 1
Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -781,12 +781,7 @@
   continue;
 }
 
-// If '-working-directory' was passed, the output filename should be
-// relative to that.
-SmallString<128> NewOutFile(OF.Filename);
-FileMgr->FixupRelativePath(NewOutFile);
-
-llvm::Error E = OF.File->keep(NewOutFile);
+llvm::Error E = OF.File->keep(OF.Filename);
 if (!E)
   continue;
 
@@ -849,6 +844,15 @@
   assert((!CreateMissingDirectories || UseTemporary) &&
  "CreateMissingDirectories is only allowed when using temporary files");
 
+  // If '-working-directory' was passed, the output filename should be
+  // relative to that.
+  Optional> AbsPath;
+  if (OutputPath != "-" && !llvm::sys::path::is_absolute(OutputPath)) {
+AbsPath.emplace(OutputPath);
+FileMgr->FixupRelativePath(*AbsPath);
+OutputPath = *AbsPath;
+  }
+
   std::unique_ptr OS;
   Optional OSFile;
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D95497: Frontend: Respect -working-directory when checking if output files can be written

[Steven Wu via Phabricator via cfe-commits]

steven_wu requested changes to this revision.
steven_wu added a comment.
This revision now requires changes to proceed.

Actually, we need to fix the case when output is '-'.


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

https://reviews.llvm.org/D95497

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

[PATCH] D95497: Frontend: Respect -working-directory when checking if output files can be written

[Steven Wu via Phabricator via cfe-commits]

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

LGTM. I will commit this if no objection.


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

https://reviews.llvm.org/D95497

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

[PATCH] D133509: Frontend: Adopt llvm::vfs::OutputBackend in CompilerInstance

[Steven Wu via Phabricator via cfe-commits]

steven_wu created this revision.
steven_wu added reviewers: sammccall, benlangmuir, raghavmedicherla, kzhuravl, 
dexonsmith.
Herald added a subscriber: ributzka.
Herald added a project: All.
steven_wu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Adopt new virtual output backend in CompilerInstance.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D133509

Files:
  clang/include/clang/Frontend/CompilerInstance.h
  clang/lib/Frontend/CompilerInstance.cpp

Index: clang/lib/Frontend/CompilerInstance.cpp
===
--- clang/lib/Frontend/CompilerInstance.cpp
+++ clang/lib/Frontend/CompilerInstance.cpp
@@ -53,6 +53,7 @@
 #include "llvm/Support/Signals.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/Timer.h"
+#include "llvm/Support/VirtualOutputBackends.h"
 #include "llvm/Support/raw_ostream.h"
 #include 
 #include 
@@ -521,6 +522,10 @@
 collectVFSEntries(*this, ModuleDepCollector);
   }
 
+  // Modules need an output manager.
+  if (!hasOutputBackend())
+createOutputBackend();
+
   for (auto  : DependencyCollectors)
 Listener->attachToPreprocessor(*PP);
 
@@ -764,36 +769,19 @@
   // The ASTConsumer can own streams that write to the output files.
   assert(!hasASTConsumer() && "ASTConsumer should be reset");
   // Ignore errors that occur when trying to discard the temp file.
-  for (OutputFile  : OutputFiles) {
-if (EraseFiles) {
-  if (OF.File)
-consumeError(OF.File->discard());
-  if (!OF.Filename.empty())
-llvm::sys::fs::remove(OF.Filename);
-  continue;
-}
-
-if (!OF.File)
-  continue;
-
-if (OF.File->TmpName.empty()) {
-  consumeError(OF.File->discard());
-  continue;
-}
-
-// If '-working-directory' was passed, the output filename should be
-// relative to that.
-SmallString<128> NewOutFile(OF.Filename);
-FileMgr->FixupRelativePath(NewOutFile);
-
-llvm::Error E = OF.File->keep(NewOutFile);
-if (!E)
-  continue;
-
-getDiagnostics().Report(diag::err_unable_to_rename_temp)
-<< OF.File->TmpName << OF.Filename << std::move(E);
-
-llvm::sys::fs::remove(OF.File->TmpName);
+  if (!EraseFiles) {
+for (auto  : OutputFiles)
+  llvm::handleAllErrors(
+  O.keep(),
+  [&](const llvm::vfs::TempFileOutputError ) {
+getDiagnostics().Report(diag::err_unable_to_rename_temp)
+<< E.getTempPath() << E.getOutputPath()
+<< E.convertToErrorCode().message();
+  },
+  [&](const llvm::vfs::OutputError ) {
+getDiagnostics().Report(diag::err_fe_unable_to_open_output)
+<< E.getOutputPath() << E.convertToErrorCode().message();
+  });
   }
   OutputFiles.clear();
   if (DeleteBuiltModules) {
@@ -827,6 +815,30 @@
   return std::make_unique();
 }
 
+void CompilerInstance::setOutputBackend(
+IntrusiveRefCntPtr NewOutputs) {
+  assert(!TheOutputBackend && "Already has an output manager");
+  TheOutputBackend = std::move(NewOutputs);
+}
+
+void CompilerInstance::createOutputBackend() {
+  assert(!TheOutputBackend && "Already has an output manager");
+  TheOutputBackend =
+  llvm::makeIntrusiveRefCnt();
+}
+
+llvm::vfs::OutputBackend ::getOutputBackend() {
+  assert(TheOutputBackend);
+  return *TheOutputBackend;
+}
+
+llvm::vfs::OutputBackend ::getOrCreateOutputBackend() {
+  if (!hasOutputBackend())
+createOutputBackend();
+  return getOutputBackend();
+}
+
+
 std::unique_ptr
 CompilerInstance::createOutputFile(StringRef OutputPath, bool Binary,
bool RemoveFileOnSignal, bool UseTemporary,
@@ -849,94 +861,29 @@
   assert((!CreateMissingDirectories || UseTemporary) &&
  "CreateMissingDirectories is only allowed when using temporary files");
 
-  std::unique_ptr OS;
-  Optional OSFile;
-
-  if (UseTemporary) {
-if (OutputPath == "-")
-  UseTemporary = false;
-else {
-  llvm::sys::fs::file_status Status;
-  llvm::sys::fs::status(OutputPath, Status);
-  if (llvm::sys::fs::exists(Status)) {
-// Fail early if we can't write to the final destination.
-if (!llvm::sys::fs::can_write(OutputPath))
-  return llvm::errorCodeToError(
-  make_error_code(llvm::errc::operation_not_permitted));
-
-// Don't use a temporary if the output is a special file. This handles
-// things like '-o /dev/null'
-if (!llvm::sys::fs::is_regular_file(Status))
-  UseTemporary = false;
-  }
-}
-  }
-
-  Optional Temp;
-  if (UseTemporary) {
-// Create a temporary file.
-// Insert - before the extension (if any), and because some tools
-// (noticeable, clang's own GlobalModuleIndex.cpp) glob for build
-// artifacts, also append .tmp.
-StringRef OutputExtension = llvm::sys::path::extension(OutputPath);
-

[PATCH] D131469: [Clang] change default storing path of `-ftime-trace`

[Steven Wu via Phabricator via cfe-commits]

steven_wu accepted this revision.
steven_wu added a comment.

Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131469

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

[PATCH] D131469: [Clang] change default storing path of `-ftime-trace`

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D131469#3768500 , @dongjunduo 
wrote:

> These related commits have been reverted temporarily.

Thanks. Another way to do this is that as you don't really care what linker 
does in this test case, you just need to fake a linker with a shell script then 
let the clang driver to invoke that script as a linker.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131469

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

[PATCH] D131469: [Clang] change default storing path of `-ftime-trace`

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D131469#3768308 , @dongjunduo 
wrote:

> In D131469#3768288 , @dyung wrote:
>
>> In D131469#3768283 , @dongjunduo 
>> wrote:
>>
>>> In D131469#3768282 , @dyung wrote:
>>>
 In D131469#3768274 , @dongjunduo 
 wrote:

> In D131469#3768260 , @dyung 
> wrote:
>
>> The test you added is failing on the PS4 linux bot because the PS4 
>> platform requires an external linker that isn't present. Is linking 
>> necessary for your test? Or can -S or even -c work instead to accomplish 
>> what you are trying to test?
>
> Yeah the new test case is designed to test the compiling jobs with a 
> linking stage.
>
> Are there any options or measures to avoid this test running on the PS4?

 You could mark it as XFAIL: ps4

 Your change also seems to have possibly the same issue when run on our PS5 
 Windows bot:
 https://lab.llvm.org/buildbot/#/builders/216/builds/9260

   $ ":" "RUN: at line 2"
   $ "z:\test\build\bin\clang.exe" "--driver-mode=g++" "-ftime-trace" 
 "-ftime-trace-granularity=0" "-o" 
 "Z:\test\build\tools\clang\test\Driver\Output/exe/check-time-trace" 
 "Z:\test\llvm-project\clang\test\Driver\check-time-trace.cpp"
   # command stderr:
   clang: error: unable to execute command: program not executable
   clang: error: linker command failed with exit code 1 (use -v to see 
 invocation)
>>>
>>> How about "**UNSUPPORTED: ps4, ps5**"
>>
>> That would likely work I think.
>
> Done with the commit 39221ad 
> .

I don't like how this done. This generally won't work for Darwin platform as 
well since you don't know where SDK is. Whether you have a linker or not has 
nothing to do with the host platform, you need to check for if the linker is 
available.

This is also a regression in test coverage since all the supported tests in 
lines after you added are no longer executed on the platform you excludes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131469

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

[PATCH] D130205: [Darwin toolchain] Tune the logic for finding arclite.

[Steven Wu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGd0728260577d: [Darwin toolchain] Tune the logic for finding 
arclite. (authored by steven_wu).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130205

Files:
  clang/lib/Driver/ToolChains/Darwin.cpp


Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1141,25 +1141,38 @@
   SmallString<128> P(getDriver().ClangExecutable);
   llvm::sys::path::remove_filename(P); // 'clang'
   llvm::sys::path::remove_filename(P); // 'bin'
+  llvm::sys::path::append(P, "lib", "arc");
 
   // 'libarclite' usually lives in the same toolchain as 'clang'. However, the
   // Swift open source toolchains for macOS distribute Clang without 
libarclite.
   // In that case, to allow the linker to find 'libarclite', we point to the
   // 'libarclite' in the XcodeDefault toolchain instead.
-  if (getXcodeDeveloperPath(P).empty()) {
-if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
+  if (!getVFS().exists(P)) {
+auto updatePath = [&](const Arg *A) {
   // Try to infer the path to 'libarclite' in the toolchain from the
   // specified SDK path.
   StringRef XcodePathForSDK = getXcodeDeveloperPath(A->getValue());
-  if (!XcodePathForSDK.empty()) {
-P = XcodePathForSDK;
-llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr");
-  }
+  if (XcodePathForSDK.empty())
+return false;
+
+  P = XcodePathForSDK;
+  llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr",
+  "lib", "arc");
+  return getVFS().exists(P);
+};
+
+bool updated = false;
+if (const Arg *A = Args.getLastArg(options::OPT_isysroot))
+  updated = updatePath(A);
+
+if (!updated) {
+  if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ))
+updatePath(A);
 }
   }
 
   CmdArgs.push_back("-force_load");
-  llvm::sys::path::append(P, "lib", "arc", "libarclite_");
+  llvm::sys::path::append(P, "libarclite_");
   // Mash in the platform.
   if (isTargetWatchOSSimulator())
 P += "watchsimulator";


Index: clang/lib/Driver/ToolChains/Darwin.cpp
===
--- clang/lib/Driver/ToolChains/Darwin.cpp
+++ clang/lib/Driver/ToolChains/Darwin.cpp
@@ -1141,25 +1141,38 @@
   SmallString<128> P(getDriver().ClangExecutable);
   llvm::sys::path::remove_filename(P); // 'clang'
   llvm::sys::path::remove_filename(P); // 'bin'
+  llvm::sys::path::append(P, "lib", "arc");
 
   // 'libarclite' usually lives in the same toolchain as 'clang'. However, the
   // Swift open source toolchains for macOS distribute Clang without libarclite.
   // In that case, to allow the linker to find 'libarclite', we point to the
   // 'libarclite' in the XcodeDefault toolchain instead.
-  if (getXcodeDeveloperPath(P).empty()) {
-if (const Arg *A = Args.getLastArg(options::OPT_isysroot)) {
+  if (!getVFS().exists(P)) {
+auto updatePath = [&](const Arg *A) {
   // Try to infer the path to 'libarclite' in the toolchain from the
   // specified SDK path.
   StringRef XcodePathForSDK = getXcodeDeveloperPath(A->getValue());
-  if (!XcodePathForSDK.empty()) {
-P = XcodePathForSDK;
-llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr");
-  }
+  if (XcodePathForSDK.empty())
+return false;
+
+  P = XcodePathForSDK;
+  llvm::sys::path::append(P, "Toolchains/XcodeDefault.xctoolchain/usr",
+  "lib", "arc");
+  return getVFS().exists(P);
+};
+
+bool updated = false;
+if (const Arg *A = Args.getLastArg(options::OPT_isysroot))
+  updated = updatePath(A);
+
+if (!updated) {
+  if (const Arg *A = Args.getLastArg(options::OPT__sysroot_EQ))
+updatePath(A);
 }
   }
 
   CmdArgs.push_back("-force_load");
-  llvm::sys::path::append(P, "lib", "arc", "libarclite_");
+  llvm::sys::path::append(P, "libarclite_");
   // Mash in the platform.
   if (isTargetWatchOSSimulator())
 P += "watchsimulator";
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D129220: [clang] Cleanup ASTContext before output files in crash recovery for modules

[Steven Wu via Phabricator via cfe-commits]

steven_wu accepted this revision.
steven_wu added a comment.

LGTM. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129220

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

[PATCH] D125974: [clang] Limit bitcode option ignorelist to Darwin

[Steven Wu via Phabricator via cfe-commits]

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

Thanks! LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125974

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

[PATCH] D125847: LTO: Decide upfront whether to use opaque/non-opaque pointer types

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D125847#3528111 , @aeubanks wrote:

> I'm still looking into a whole program devirtualization bug that only repros 
> with opaque pointers, and there are still potential performance issues to 
> look into

Thanks. Maybe here is another way to look at this problem, instead of giving 
people a temporary switch to use opaque_pointer if their first bitcode is 
no_opaque_pointer, we opt as aggressive as possible into opaque_pointer and 
give people a temporary switch to go back to old behavior just like how clang 
is configured.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125847

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

[PATCH] D125847: LTO: Decide upfront whether to use opaque/non-opaque pointer types

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

Is there any issues currently if we just always use opaque pointer in LTO?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125847

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

[PATCH] D125974: [clang] Limit bitcode option ignorelist to Darwin

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

LGTM. Can you add a test for your usecase?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125974

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

[PATCH] D121332: Cleanup includes: DebugInfo & CodeGen

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

This breaks implicit module on macOS bots: 
https://green.lab.llvm.org/green/job/lldb-cmake/42098/console

Error message:

  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/CodeGen/MachinePipeliner.h:136:3:
 error: missing '#include "llvm/ADT/SetVector.h"'; 'SetVector' must be declared 
before it is used
SetVector NodeOrder;
^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/include/llvm/ADT/SetVector.h:40:7:
 note: declaration here is not visible
  class SetVector {
^
  
/Users/buildslave/jenkins/workspace/lldb-cmake/llvm-project/llvm/lib/CodeGen/AtomicExpandPass.cpp:21:10:
 fatal error: could not build module 'LLVM_Backend'
  #include "llvm/CodeGen/AtomicExpandUtils.h"
   ^~
  2 errors generated.

This can be fixed by adding `ADT/SetVector.h` to 
`include/llvm/CodeGen/MachinePipeliner.h`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121332

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

[PATCH] D118352: [clang][ABI] New c++20 modules mangling scheme

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D118352#3368922 , @ChuanqiXu wrote:

> In D118352#3368919 , @phosek wrote:
>
>> We're also seeing this issue on our Mac bots, is it possible to revert it?
>
> I've reverted it. Since this is the new feature, I think it wouldn't so hurry 
> to land it. BTW, I think it would be helpful to provide more information to 
> fix it.

It breaks the macOS bot for LLVM, you can click the test case for its output: 
https://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/27745/testReport/

You can see the error output. The problem might be that `dso_local` does not 
get codegen for macho targets.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118352

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

[PATCH] D118862: [clang][driver] add clang driver support for emitting macho files with two build version load commands

[Steven Wu via Phabricator via cfe-commits]

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

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118862

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

[PATCH] D72404: [ThinLTO/FullLTO] Support Os and Oz

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I just saw this. I know it is a good idea to have choice during link time for 
the pipeline configuration and from your benchmark it also has size impact on 
the output. I also feel like this is going in the wrong direction as if I have 
part of the object files built with -O3 and part built with -Oz, I need to make 
a choice of unoptimized part of the program.

Before I say yes or no to this patch, have you figured out what are the passes 
that causes the most size regression? Ideally, with function attributes on the 
function, it shouldn't be much size impact on the output.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72404

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

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D111863#3071328 , @housel wrote:

> In D111863#3069279 , @lhames wrote:
>
>> I think the ORC runtime provides a much more natural way to test this. Did 
>> you manage to come up with some ORC-runtime based tests in the end?
>
> My current plan is to automate what I've been doing manually, namely running 
> a test of C++ exception handling using `llvm-jitlink`, both with the default 
> configuration (using libgcc-provided unwinding), and with libunwind 
> `LD_PRELOAD`ed to force it as the unwinding provider.

@lhames  The other possibility is to lift JIT runtime to top level in 
llvm-project so it can link libunwind into its runtime so you don't need to:

- Create a public API in libunwind to support JIT
- Worry about deploying a compatible libunwind together with JIT (or worry 
about back-deploy)
- You can write all the tests in ORCJIT context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111863

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

[PATCH] D111863: [libunwind] Add an interface for dynamic .eh_frame registration

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I don't know enough about Dwarf unwinding but the implementation looks 
generally good. Can you please add a testcase indicating how ORCJIT is planning 
to use it?




Comment at: libunwind/src/DwarfParser.hpp:158
+   FDE_Info *fdeInfo, CIE_Info *cieInfo,
+   bool useCIEInfo = false);
   static bool parseFDEInstructions(A , const FDE_Info ,

Can you document how and when this parameter should be used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111863

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

[PATCH] D108881: [clang][driver] Honor the last -flto(=.*)? argument

[Steven Wu via Phabricator via cfe-commits]

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

I am not familiar with offloading side of LTO driver but the change looks 
functionally the same to me. Thanks for doing this! Remember to update the 
commit message with the correct info.


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

https://reviews.llvm.org/D108881

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

[PATCH] D108881: [clang][driver] Honor the last -flto(=.*)? argument

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D108881#2988026 , @tejohnson wrote:

> In D108881#2988016 , @steven_wu 
> wrote:
>
>> In D108881#2987990 , @mnadeem 
>> wrote:
>>
>>> In D108881#2973735 , @steven_wu 
>>> wrote:
>>>
 In D108881#2973719 , @mnadeem 
 wrote:

> In D108881#2973516 , @steven_wu 
> wrote:
>
>> I will do a cleanup of `parseLTOMode` function since we don't need a 
>> `OptPos` parameter anymore. There are few minor places references 
>> `OPT_flto` or `OPT_foffload_lto` can be cleaned up too.
>
> Will you incorporate the functional changes in this patch? Or is there 
> still a need for this change?

 The current change set in this review is functional change while the 
 cleanup I want is not functional after the rewrite the old option as 
 Alias. Once flto is the alias, there is no need to handle that in the 
 driver and those might actually become source of bug in the future.

 I think it would be good to do the cleanup in the same commit unless you 
 have compelling reason not to.
>>>
>>> Hi @steven_wu any idea about the timeline?
>>>
>>> This issue is blocking some internal work, and assuming that it will take 
>>> longer to get a full fix, I would prefer it if this change could go in on 
>>> its own.
>>> Otherwise I am good with doing everything in the same commit.
>>
>> I expect the cleanup is very very simple and that is why I am suggested to 
>> do in the same commit. I am ok with a followup fix as well.
>
> I think there might be some high level confusion. @steven_wu earlier in the 
> thread you said "I will do a cleanup", but I think you were asking @mnadeem 
> to do the cleanup here in this patch.

Ah, I see. I probably type the reply from my phone do I didn't double check. I 
really mean "I would suggest @mnadeem to do a cleanup in the same commit". 
Sorry about the confusion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108881

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

[PATCH] D108881: [clang][driver] Honor the last -flto(=.*)? argument

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

If you didn't quite get what I mean, I am suggesting to cleanup all the 
reference to the old flag. Since `-flto` is rewrite by the driver to 
`-flto=full`, clang should not query `OPT_flto` anymore. All references to the 
old flag needs to be audited and removed while preserving the correct behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108881

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

[PATCH] D108881: [clang][driver] Honor the last -flto(=.*)? argument

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D108881#2987990 , @mnadeem wrote:

> In D108881#2973735 , @steven_wu 
> wrote:
>
>> In D108881#2973719 , @mnadeem 
>> wrote:
>>
>>> In D108881#2973516 , @steven_wu 
>>> wrote:
>>>
 I will do a cleanup of `parseLTOMode` function since we don't need a 
 `OptPos` parameter anymore. There are few minor places references 
 `OPT_flto` or `OPT_foffload_lto` can be cleaned up too.
>>>
>>> Will you incorporate the functional changes in this patch? Or is there 
>>> still a need for this change?
>>
>> The current change set in this review is functional change while the cleanup 
>> I want is not functional after the rewrite the old option as Alias. Once 
>> flto is the alias, there is no need to handle that in the driver and those 
>> might actually become source of bug in the future.
>>
>> I think it would be good to do the cleanup in the same commit unless you 
>> have compelling reason not to.
>
> Hi @steven_wu any idea about the timeline?
>
> This issue is blocking some internal work, and assuming that it will take 
> longer to get a full fix, I would prefer it if this change could go in on its 
> own.
> Otherwise I am good with doing everything in the same commit.

I expect the cleanup is very very simple and that is why I am suggested to do 
in the same commit. I am ok with a followup fix as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108881

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

[PATCH] D108881: [clang][driver] Honor the last -flto(=.*)? argument

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

In D108881#2973719 , @mnadeem wrote:

> In D108881#2973516 , @steven_wu 
> wrote:
>
>> I will do a cleanup of `parseLTOMode` function since we don't need a 
>> `OptPos` parameter anymore. There are few minor places references `OPT_flto` 
>> or `OPT_foffload_lto` can be cleaned up too.
>
> Will you incorporate the functional changes in this patch? Or is there still 
> a need for this change?

The current change set in this review is functional change while the cleanup I 
want is not functional after the rewrite the old option as Alias. Once flto is 
the alias, there is no need to handle that in the driver and those might 
actually become source of bug in the future.

I think it would be good to do the cleanup in the same commit unless you have 
compelling reason not to.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108881

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

[PATCH] D108881: [clang][driver] Honor the last -flto(=.*)? argument

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I will do a cleanup of `parseLTOMode` function since we don't need a `OptPos` 
parameter anymore. There are few minor places references `OPT_flto` or 
`OPT_foffload_lto` can be cleaned up too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108881

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

[PATCH] D108881: [clang][driver] Honor the last -flto= flag even if an earlier -flto is present

[Steven Wu via Phabricator via cfe-commits]

steven_wu added a comment.

I agree that it might be good to change to the behavior that last arg wins but 
it might be simpler to set `-flto` to be an alias of `-flto=full`:

  diff --git a/clang/include/clang/Driver/Options.td 
b/clang/include/clang/Driver/Options.td
  index 1a3d1dcceec7..a796fb7fe3ac 100644
  --- a/clang/include/clang/Driver/Options.td
  +++ b/clang/include/clang/Driver/Options.td
  @@ -2086,7 +2086,7 @@ def flto_EQ : Joined<["-"], "flto=">, 
Flags<[CoreOption, CC1Option]>, Group, Group;
   def flto_EQ_auto : Flag<["-"], "flto=auto">, Group;
   def flto : Flag<["-"], "flto">, Flags<[CoreOption, CC1Option]>, 
Group,
  -  HelpText<"Enable LTO in 'full' mode">;
  +  Alias, AliasArgs<["full"]>, HelpText<"Enable LTO in 'full' mode">;
   def fno_lto : Flag<["-"], "fno-lto">, Flags<[CoreOption, CC1Option]>, 
Group,
 HelpText<"Disable LTO mode (default)">;
   def foffload_lto_EQ : Joined<["-"], "foffload-lto=">, Flags<[CoreOption]>, 
Group,

Of course, you want to clean up the option processing of `flto`.


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

https://reviews.llvm.org/D108881

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

[PATCH] D107667: [clang/test] Run thinlto-clang-diagnostic-handler-in-be.c on x86

[Steven Wu via Phabricator via cfe-commits]

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

LGTM. Thanks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107667

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