Page MenuHomePhabricator

ychen (Yuanfang Chen)
User

Projects

User Details

User Since
Nov 19 2013, 9:02 PM (322 w, 5 d)

Recent Activity

Sat, Jan 25

ychen updated the diff for D73329: [libFuzzer] communicate through pipe to subprocess for CleanseCrashInput MinimizeCrashInput.
  • fix two linux test failure
Sat, Jan 25, 8:06 PM · Restricted Project, Restricted Project

Fri, Jan 24

ychen added a comment to D73011: [opt viewer] Python compat - decode/encode string.

ping?

Fri, Jan 24, 10:41 AM · Restricted Project
ychen planned changes to D73329: [libFuzzer] communicate through pipe to subprocess for CleanseCrashInput MinimizeCrashInput.

Somehow this caused three failures on Linux. I'll take a look.

Fri, Jan 24, 12:21 AM · Restricted Project, Restricted Project

Thu, Jan 23

ychen created D73329: [libFuzzer] communicate through pipe to subprocess for CleanseCrashInput MinimizeCrashInput.
Thu, Jan 23, 11:29 PM · Restricted Project, Restricted Project
ychen created D73327: [compiler-rt][profile] fix test/instrprof-set-filename.c on windows.
Thu, Jan 23, 11:10 PM · Restricted Project, Restricted Project
ychen retitled D73304: [compiler-rt] enable test-suite discovery in source tree for libFuzzer & profile from [libFuzzer] for lit, enable discovering test-suite in source tree to [compiler-rt] enable test-suite discovery in source tree for libFuzzer & profile.
Thu, Jan 23, 11:01 PM · Restricted Project, Restricted Project
ychen updated the diff for D73304: [compiler-rt] enable test-suite discovery in source tree for libFuzzer & profile.
  • enable profile test-suite discovery
Thu, Jan 23, 11:01 PM · Restricted Project, Restricted Project
ychen created D73312: [lit] Use canonical None test.
Thu, Jan 23, 5:00 PM · Restricted Project
ychen created D73304: [compiler-rt] enable test-suite discovery in source tree for libFuzzer & profile.
Thu, Jan 23, 4:11 PM · Restricted Project, Restricted Project

Mon, Jan 20

ychen added a comment to D73010: [llvm-lto2] By default, use two threads for ThinLTO backend..

I tend to ignore failures on clang-ppc64be-linux before... From my experience it was quite unstable and often failed with c++: internal compiler error: Killed (program cc1plus). Not sure if it has improved. If it is the only failing bot I tend to ignore it. Maybe we can ask the admin of the build bot.

Mon, Jan 20, 9:30 AM · Restricted Project
ychen added a comment to D73010: [llvm-lto2] By default, use two threads for ThinLTO backend..

Honestly, this looks weird (why 2 threads not 1 for example)?

1 feels very special for multithreading code. I've seen code with separate paths for 1 and > 1, although I'm not aware of any similar code for ThinLTO.

Mon, Jan 20, 9:21 AM · Restricted Project
ychen updated the summary of D73010: [llvm-lto2] By default, use two threads for ThinLTO backend..
Mon, Jan 20, 9:11 AM · Restricted Project

Sun, Jan 19

ychen added a comment to D73010: [llvm-lto2] By default, use two threads for ThinLTO backend..

@MaskRay @mehdi_amini I have no clue either on how this does not happen before. Adding std::__throw_system_error in ThreadPool ctor and reverting this change, I still see the crash in my machine (Ubuntu 18.04). I'm inclined to think the exception did not trigger before rather than it was triggered but somehow muted in some way.

Sun, Jan 19, 11:02 PM · Restricted Project
ychen added a comment to D73011: [opt viewer] Python compat - decode/encode string.

@MaskRay Yeah, that would be ideal. I'm also in favor of supporting python3 only. The change for open to add encoding parameter is still needed though since the default value is platform-dependent.

Sun, Jan 19, 10:43 PM · Restricted Project
ychen created D73011: [opt viewer] Python compat - decode/encode string.
Sun, Jan 19, 6:00 PM · Restricted Project
ychen created D73010: [llvm-lto2] By default, use two threads for ThinLTO backend..
Sun, Jan 19, 5:51 PM · Restricted Project

Wed, Jan 15

ychen added a comment to D67847: [Support] make report_fatal_error `abort` instead of `exit`.

There are some interesting failures that need to be investigated.
https://reviews.llvm.org/rG647c3f4e47de8a850ffcaa897db68702d8d2459a

Wed, Jan 15, 7:29 PM · Restricted Project, Restricted Project
ychen added a comment to rG647c3f4e47de: [Support] make report_fatal_error `abort` instead of `exit`.

This is going deeper...

Wed, Jan 15, 7:20 PM
ychen committed rG6e24c6037f79: Revert "[Support] make report_fatal_error `abort` instead of `exit`" (authored by ychen).
Revert "[Support] make report_fatal_error `abort` instead of `exit`"
Wed, Jan 15, 5:57 PM
ychen added a reverting change for rG647c3f4e47de: [Support] make report_fatal_error `abort` instead of `exit`: rG6e24c6037f79: Revert "[Support] make report_fatal_error `abort` instead of `exit`".
Wed, Jan 15, 5:57 PM
ychen committed rG647c3f4e47de: [Support] make report_fatal_error `abort` instead of `exit` (authored by ychen).
[Support] make report_fatal_error `abort` instead of `exit`
Wed, Jan 15, 5:11 PM
ychen closed D67847: [Support] make report_fatal_error `abort` instead of `exit`.
Wed, Jan 15, 5:10 PM · Restricted Project, Restricted Project
ychen added a comment to D44352: [Concepts] Type Constraints.

I saw a null pointer dereference for the p3-2a.cpp file added here. It is not always reproducible for me, I was able to reproduce it once offline. Any idea?

Wed, Jan 15, 4:03 PM · Restricted Project
ychen updated the diff for D67847: [Support] make report_fatal_error `abort` instead of `exit`.
  • fix tests
Wed, Jan 15, 3:15 PM · Restricted Project, Restricted Project
ychen added a comment to D67847: [Support] make report_fatal_error `abort` instead of `exit`.

Apologies that it takes so long. I'll fix the test and land it today.

Wed, Jan 15, 12:23 PM · Restricted Project, Restricted Project
ychen updated the diff for D67847: [Support] make report_fatal_error `abort` instead of `exit`.
  • rebase
Wed, Jan 15, 10:22 AM · Restricted Project, Restricted Project

Tue, Jan 14

ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.
  • fix test when clang is the host compiler
Tue, Jan 14, 4:55 PM · Restricted Project, Restricted Project
ychen added a comment to D72404: [ThinLTO/FullLTO] Support Os and Oz.

Unit tests: fail. 61858 tests passed, 1 failed and 781 were skipped.

failed: Clang.CodeGen/thinlto-debug-pm.c

I think that failure can be fixed with something like this:

diff --git a/clang/test/CodeGen/thinlto-debug-pm.c b/clang/test/CodeGen/thinlto-debug-pm.c
 index 5f449d493af..9d6d69afd13 100644
 --- a/clang/test/CodeGen/thinlto-debug-pm.c
 +++ b/clang/test/CodeGen/thinlto-debug-pm.c
 @@ -13,17 +13,17 @@
  // O0123sz-NEWPM: Running analysis: PassInstrumentationAnalysis
  // O0123sz-NEWPM: Starting llvm::Module pass manager run.
  // O0123sz-NEWPM: Running pass: WholeProgramDevirtPass
 -// O0123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O0123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O0123sz-NEWPM: Running pass: LowerTypeTestsPass
  // O0123sz-NEWPM: Invalidating all non-preserved analyses for:
 -// O0123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O0123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running pass: ForceFunctionAttrsPass
  // O123sz-NEWPM: Running pass: PassManager<llvm::Module>
  // O123sz-NEWPM: Starting llvm::Module pass manager run.
  // O123sz-NEWPM: Running pass: PGOIndirectCallPromotion
  // O123sz-NEWPM: Running analysis: ProfileSummaryAnalysis
  // O123sz-NEWPM: Running pass: InferFunctionAttrsPass
 -// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running pass: ModuleToFunctionPassAdaptor<llvm::PassManager<llvm::Function> >
  // O123sz-NEWPM: Running analysis: PassInstrumentationAnalysis on foo
  // O123sz-NEWPM: Starting llvm::Function pass manager run.
 @@ -41,9 +41,9 @@
  // O123sz-NEWPM: Running pass: CalledValuePropagationPass
  // O123sz-NEWPM: Running pass: GlobalOptPass
  // O123sz-NEWPM: Invalidating all non-preserved analyses for:
 -// O123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O123sz-NEWPM: Invalidating analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running pass: ModuleToFunctionPassAdaptor<llvm::PromotePass>
 -// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::AnalysisManager<llvm::Function>, llvm::Module>
 +// O123sz-NEWPM: Running analysis: InnerAnalysisManagerProxy<llvm::FunctionAnalysisManager, llvm::Module>
  // O123sz-NEWPM: Running analysis: DominatorTreeAnalysis on foo
  // O123sz-NEWPM: Running analysis: PassInstrumentationAnalysis on foo
  // O123sz-NEWPM: Running analysis: AssumptionAnalysis on foo
 @@ -57,7 +57,7 @@
  // O123sz-NEWPM: Running analysis: BasicAA on foo
  // O123sz-NEWPM: Running analysis: ScopedNoAliasAA on foo
  // O123sz-NEWPM: Running analysis: TypeBasedAA on foo
 -// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::AnalysisManager<llvm::Module>, llvm::Function> on foo
 +// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::ModuleAnalysisManager, llvm::Function> on foo
  // O123sz-NEWPM: Running pass: SimplifyCFGPass on foo
  // O123sz-NEWPM: Running analysis: TargetIRAnalysis on foo
  // O123sz-NEWPM: Finished llvm::Function pass manager run.
 @@ -70,7 +70,7 @@
  // O123sz-NEWPM: Running analysis: LazyCallGraphAnalysis
  // O123sz-NEWPM: Running analysis: FunctionAnalysisManagerCGSCCProxy on (foo)
  // O123sz-NEWPM: Running analysis: PassInstrumentationAnalysis on (foo)
 -// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::AnalysisManager<llvm::Module>, llvm::LazyCallGraph::SCC, llvm::LazyCallG>
 +// O123sz-NEWPM: Running analysis: OuterAnalysisManagerProxy<llvm::ModuleAnalysisManager, llvm::LazyCallGraph::SCC, llvm::LazyCallGraph&> o>
  // O123sz-NEWPM: Starting CGSCC pass manager run.
  // O123sz-NEWPM: Running pass: InlinerPass on (foo)
  // O123sz-NEWPM: Running pass: PostOrderFunctionAttrsPass on (foo)
Tue, Jan 14, 2:36 PM · Restricted Project, Restricted Project
ychen retitled D72404: [ThinLTO/FullLTO] Support Os and Oz from [WIP][ThinLTO] Support Os and Oz to [ThinLTO/FullLTO] Support Os and Oz.
Tue, Jan 14, 12:40 PM · Restricted Project, Restricted Project
ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.
  • rebase & clang-format
Tue, Jan 14, 12:40 PM · Restricted Project, Restricted Project
ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.
  • Rebase
Tue, Jan 14, 10:54 AM · Restricted Project, Restricted Project
ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.
  • fix a typo
Tue, Jan 14, 10:32 AM · Restricted Project, Restricted Project
ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.
  • when optnone is not present, add optsize for Os and minsize for Oz
  • add Os Oz function attribute test.
  • add Os Oz pipeline test
Tue, Jan 14, 10:23 AM · Restricted Project, Restricted Project

Thu, Jan 9

ychen added a comment to rG4516dc1c20d1: Don't add optnone or noinline if the function is already marked as….

Since this is -fapple-ext specific, would it be better to keep everything as is and move the always_inline setting for -fapple-ext from code emission to after the regular inline attribute decision is made on non--fapple-ext cases in SetLLVMFunctionAttributesForDefinition? It would be less diff and keep the assertion which IMHO is still useful.

Thu, Jan 9, 6:03 PM

Wed, Jan 8

ychen updated subscribers of D72404: [ThinLTO/FullLTO] Support Os and Oz.
Wed, Jan 8, 3:28 PM · Restricted Project, Restricted Project
ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.

fix test

Wed, Jan 8, 2:32 PM · Restricted Project, Restricted Project
ychen updated the diff for D72404: [ThinLTO/FullLTO] Support Os and Oz.
  • fix test
Wed, Jan 8, 2:32 PM · Restricted Project, Restricted Project
ychen added a comment to D72404: [ThinLTO/FullLTO] Support Os and Oz.
In D72404#1810489, @pcc wrote:

This is going in the wrong direction IMO. We already have a way of specifying the size optimization level, which is to specify -Os or -Oz at compile time. Why is that not sufficient?

Wed, Jan 8, 11:08 AM · Restricted Project, Restricted Project
ychen updated the summary of D72404: [ThinLTO/FullLTO] Support Os and Oz.
Wed, Jan 8, 10:49 AM · Restricted Project, Restricted Project
ychen retitled D72404: [ThinLTO/FullLTO] Support Os and Oz from [ThinLTO] Support Os and Oz to [WIP][ThinLTO] Support Os and Oz.
Wed, Jan 8, 10:40 AM · Restricted Project, Restricted Project
ychen created D72404: [ThinLTO/FullLTO] Support Os and Oz.
Wed, Jan 8, 10:40 AM · Restricted Project, Restricted Project

Mon, Jan 6

ychen added a reviewer for D72198: tools/timeit.sh: be flexible while parsing perf-stat output: kongyi.
Mon, Jan 6, 4:12 PM · Restricted Project

Sat, Jan 4

ychen abandoned D72199: [LNT] Support TEST_SUITE_SUBDIRS for test_suite.

--cmake-define works just fine.

Sat, Jan 4, 5:52 PM

Fri, Jan 3

ychen created D72199: [LNT] Support TEST_SUITE_SUBDIRS for test_suite.
Fri, Jan 3, 11:40 PM
ychen created D72198: tools/timeit.sh: be flexible while parsing perf-stat output.
Fri, Jan 3, 11:39 PM · Restricted Project
ychen added a comment to D71684: [SPECCPU2017] Add CXXPORTABILITY flags for 526.blender_r.

Thanks for the feedback. Obviously, my CMake skills needs to catch up. My
intention was to add the macro definition only for CXX compilation, but
add_definitions, add_compile_options change both C and CXX flags AFAIK at
the time.

Does defining __BOOL_DEFINED for C as well have any effect? If not, I'd define it for both languages for simplicity.

Defining __BOOL_DEFINED for C would cause build error.

Fri, Jan 3, 2:36 PM · Restricted Project
ychen updated the diff for D71684: [SPECCPU2017] Add CXXPORTABILITY flags for 526.blender_r.

address the comment

Fri, Jan 3, 2:36 PM · Restricted Project

Dec 26 2019

ychen accepted D71863: Ignore "no-frame-pointer-elim" and "no-frame-pointer-elim-non-leaf" in favor of "frame-pointer".

LGTM

Dec 26 2019, 7:33 PM · Restricted Project

Dec 24 2019

Herald added a project to D43040: gold-plugin: Do not set codegen opt level based on LTO opt level.: Restricted Project.
Dec 24 2019, 9:48 PM · Restricted Project
ychen added a comment to D71863: Ignore "no-frame-pointer-elim" and "no-frame-pointer-elim-non-leaf" in favor of "frame-pointer".

Probably need to translate old attributes to new attribute in AutoUpgrade.cpp? Otherwise, this may cause problems for existing LTO libraries.

Dec 24 2019, 7:23 PM · Restricted Project

Dec 20 2019

ychen updated the diff for D71684: [SPECCPU2017] Add CXXPORTABILITY flags for 526.blender_r.

Thanks for the feedback. Obviously, my CMake skills needs to catch up. My
intention was to add the macro definition only for CXX compilation, but
add_definitions, add_compile_options change both C and CXX flags AFAIK at
the time. After some digging, it seems add_compile_options and
add_compile_definitions support cmake-generator-expressions that could be
used to implement this perfectly. However, add_compile_definitions was not
available until CMake v3.12, so I use add_compile_options here.

Dec 20 2019, 6:30 PM · Restricted Project
Herald updated subscribers of D71786: RFC: [Support] On Windows, add optional support for rpmalloc.
Dec 20 2019, 2:28 PM · Restricted Project

Dec 18 2019

ychen created D71684: [SPECCPU2017] Add CXXPORTABILITY flags for 526.blender_r.
Dec 18 2019, 6:09 PM · Restricted Project
GitHub <noreply@github.com> committed rG6218696bc938: [Docs] Fix a typo (authored by ychen).
[Docs] Fix a typo
Dec 18 2019, 3:26 PM

Dec 17 2019

ychen added a comment to D71261: [ThinLTO] upgrade IR symtab in parallel ahead of time.

This sounds like a good idea. Have you thought about just write out upgraded bitcode file on disk (since you named it a "SymTabCache"), maybe into thinLTO cache? This will speed up the incremental build as well. I think most of the upgrade path are coming from linking a bitcode static library, which is probably not changing during incremental build so we just need to upgrade once.

Dec 17 2019, 11:36 AM · Restricted Project

Dec 10 2019

ychen edited reviewers for D71261: [ThinLTO] upgrade IR symtab in parallel ahead of time, added: pcc, tejohnson, steven_wu; removed: espindola.
Dec 10 2019, 5:25 AM · Restricted Project
ychen created D71261: [ThinLTO] upgrade IR symtab in parallel ahead of time.
Dec 10 2019, 4:57 AM · Restricted Project

Dec 9 2019

ychen updated the diff for D67687: [CodeGen] Define an interface for the new pass manager. (new).

fix a bug

Dec 9 2019, 10:00 PM · Restricted Project

Oct 24 2019

ychen committed rGef7a154d17f2: [clang][ThinLTO] Promote cc1 -fthin_link_bitcode to driver… (authored by ychen).
[clang][ThinLTO] Promote cc1 -fthin_link_bitcode to driver…
Oct 24 2019, 5:46 PM
ychen closed D69406: [clang][ThinLTO] Promote cc1 -fthin_link_bitcode to driver -fthinlto_link_bitcode.
Oct 24 2019, 5:46 PM · Restricted Project
ychen updated the diff for D69406: [clang][ThinLTO] Promote cc1 -fthin_link_bitcode to driver -fthinlto_link_bitcode.

Address reviewer's comments.

Oct 24 2019, 3:37 PM · Restricted Project
ychen created D69406: [clang][ThinLTO] Promote cc1 -fthin_link_bitcode to driver -fthinlto_link_bitcode.
Oct 24 2019, 2:26 PM · Restricted Project
ychen added a comment to D69327: [Clang][ThinLTO] Add a cache for compile phase output..

Thank you @steven_wu @tejohnson. I created D69406 to promote the flag to the driver.

Oct 24 2019, 2:26 PM · Restricted Project, Restricted Project
ychen added a comment to D69327: [Clang][ThinLTO] Add a cache for compile phase output..

Sorry for the confusion @steven_wu. By stable I mean the probability that the -fthin-link-bitcode option is replaced with some other thinlink mechanism under the distributed build environment. Since at least for ccache, the compilation output caching depends on the semantics of options ("-o" is assumed to be compilation output). For the case of -fthin-link-bitcode, both -Xclang -fthin-link-bitcode and -o are the output. I'm not familiar with compiler cache tools, but having the caching depends on a cc1 option feels not right since it is not an option of any other compilers, so most caching tools don't recognize it.

Oct 24 2019, 1:10 AM · Restricted Project, Restricted Project
ychen added a comment to D69327: [Clang][ThinLTO] Add a cache for compile phase output..

Thanks for the inputs @steven_wu @tejohnson. Totally agree with the points you brought up. One last thing I'm not quite sure is the caching of -fthin-link-bitcode. It is a -cc1 option since it is a kind of implementation of ThinLTO, right? I'm a little hesitant to begin writing up patches to teach build system/caching tool (I could think of at least three for our workload) to recognize this option because of that. If there are any changes to the option, the same thing needs to be done again. Do you have any thoughts on that? Is the option in use for your workload and do you think it is stable enough to have build systems caching for it? (Another option is to produce -fthin-link-bitcode output post compile time which I assume having total build time impact to some degree).

-fthin-link-bitcode option is used to run distributed thin link. The format is not stable but it is deterministic for a fixed compiler version. You should be able to cache the thin-link-bitcode and expected it to be used only by the same compiler version.

For any build system that implements caching, it must take compiler version into consideration because different compiler will produce different output. I don't think the rule to cache thin-link-bitcode is any different from any other output during the build.

Oct 24 2019, 12:47 AM · Restricted Project, Restricted Project

Oct 23 2019

ychen added a comment to D69327: [Clang][ThinLTO] Add a cache for compile phase output..

Thanks for the inputs @steven_wu @tejohnson. Totally agree with the points you brought up. One last thing I'm not quite sure is the caching of -fthin-link-bitcode. It is a -cc1 option since it is a kind of implementation of ThinLTO, right? I'm a little hesitant to begin writing up patches to teach build system/caching tool (I could think of at least three for our workload) to recognize this option because of that. If there are any changes to the option, the same thing needs to be done again. Do you have any thoughts on that? Is the option in use for your workload and do you think it is stable enough to have build systems caching for it? (Another option is to produce -fthin-link-bitcode output post compile time which I assume having total build time impact to some degree).

Oct 23 2019, 9:22 PM · Restricted Project, Restricted Project
ychen added a comment to D69327: [Clang][ThinLTO] Add a cache for compile phase output..

I haven't read through the patch in detail yet, but from the description it sounds like a cache is being added for the compile step outputs, e.g. the bitcode output object of the "clang -flto=thin -c" step? Typically the build system takes care of incremental build handling for these explicit outputs, just as it would for a non-LTO clang compile output. Can you clarify the motivation?

Oct 23 2019, 2:14 PM · Restricted Project, Restricted Project
ychen created D69327: [Clang][ThinLTO] Add a cache for compile phase output..
Oct 23 2019, 12:56 AM · Restricted Project, Restricted Project

Oct 4 2019

ychen committed rG442ddffe138a: [clang] fix a typo from r372531 (authored by ychen).
[clang] fix a typo from r372531
Oct 4 2019, 2:40 PM
ychen created D68482: [clang] fix a typo from r372531.
Oct 4 2019, 1:35 PM · Restricted Project, Restricted Project

Oct 2 2019

ychen updated the summary of D67847: [Support] make report_fatal_error `abort` instead of `exit`.
Oct 2 2019, 4:38 PM · Restricted Project, Restricted Project
ychen updated the summary of D67847: [Support] make report_fatal_error `abort` instead of `exit`.
Oct 2 2019, 4:38 PM · Restricted Project, Restricted Project
ychen added a comment to D67847: [Support] make report_fatal_error `abort` instead of `exit`.

The abort() function raises SIGABRT, for which the default behavior is to trigger a coredump. Do we actually want that behavior?

Either _exit() (long available extension, which lld already uses) or quick_exit() (the new C standard way) seem possibly preferable?

Oct 2 2019, 2:04 PM · Restricted Project, Restricted Project
ychen updated the summary of D67847: [Support] make report_fatal_error `abort` instead of `exit`.
Oct 2 2019, 11:12 AM · Restricted Project, Restricted Project

Oct 1 2019

ychen updated the diff for D67847: [Support] make report_fatal_error `abort` instead of `exit`.

Fix tests

Oct 1 2019, 5:35 PM · Restricted Project, Restricted Project

Sep 30 2019

ychen added a comment to D68164: [Commandline] Clear error on raw_ostream when there is parsing error..

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&-) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

sorry, I'm really confused now. Based on the comment in close-stderr.ll, the test is to make sure abort does not happen? Why is it checking the undefined behavior?

close-stderr.ll was added to test a particular behavior of an undefined behavior.

By undefined I don't know what kind of consistent behavior could be checked.

Sep 30 2019, 11:11 PM · Restricted Project
ychen added a comment to D68164: [Commandline] Clear error on raw_ostream when there is parsing error..

The exit is called twice and somehow the test exit cleanly.

The current behavior is undefined. POSIX says "If exit() is called more than once, the behavior is undefined." Changing exit to abort in D67847 will fix the issue. Thanks.

The test ; RUN: sh -c 'opt --reject-this-option 2>&-; echo $?; opt -o /dev/null /dev/null 2>&-; echo $?;' is testing an undefined behavior, but I can see 2>&- is a simple alternative to a full-fledged test that makes write operations fail with some other errno. My feeling is that you can just update close-stderr.ll to (not --crash opt --reject-this-option 2>&1) in D67847 and be prepared to update it if build bots on some platforms have different behaviors.

Sep 30 2019, 10:39 PM · Restricted Project
ychen added a comment to D68164: [Commandline] Clear error on raw_ostream when there is parsing error..

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

The new behavior is not distinguishable from the existing behavior from the user's perspective. The existing behavior does exit(1)-> report_fatal_error -> exit(1) and new behavior just exit(1) cleanly.

Sep 30 2019, 9:45 PM · Restricted Project
ychen added a comment to D68164: [Commandline] Clear error on raw_ostream when there is parsing error..

So, could you add a testcase to demonstrate the issue?

If an error occurs, I believe this piece of code will try to call report_fatal_error and that will fail too for the same reason (because if the code fails, it is likely that we can't write anything to stderr), and the process will exit with exit code 1. So, is the new behavior implemented in this patch distinguishable from the existing behavior? If a command behaves differently with this patch, please add a test to demonstrate it.

Sep 30 2019, 9:32 PM · Restricted Project
ychen added a comment to D68164: [Commandline] Clear error on raw_ostream when there is parsing error..

Can you add a testcase to demonstrate the issue?

Sep 30 2019, 9:02 PM · Restricted Project
ychen added a child revision for D68164: [Commandline] Clear error on raw_ostream when there is parsing error.: D67847: [Support] make report_fatal_error `abort` instead of `exit`.
Sep 30 2019, 8:52 PM · Restricted Project
ychen added a parent revision for D67847: [Support] make report_fatal_error `abort` instead of `exit`: D68164: [Commandline] Clear error on raw_ostream when there is parsing error..
Sep 30 2019, 8:52 PM · Restricted Project, Restricted Project
ychen added a comment to D64183: [NewPM] Port MachineModuleInfo to the new pass manager..

Removed in r373297. Thanks.

Sep 30 2019, 7:09 PM · Restricted Project
ychen committed rGb169ee2eca0e: Remove a undefined constructor introduced by r373244. (authored by ychen).
Remove a undefined constructor introduced by r373244.
Sep 30 2019, 7:08 PM
ychen added a comment to D64183: [NewPM] Port MachineModuleInfo to the new pass manager..

Fixed in r373244. Thanks!

Sep 30 2019, 11:57 AM · Restricted Project
ychen committed rGf0ca10f2abf3: Fix build warning for r373240. (authored by ychen).
Fix build warning for r373240.
Sep 30 2019, 11:33 AM
ychen committed rGcc382cf72736: [NewPM] Port MachineModuleInfo to the new pass manager. (authored by ychen).
[NewPM] Port MachineModuleInfo to the new pass manager.
Sep 30 2019, 10:55 AM
ychen added a comment to D68164: [Commandline] Clear error on raw_ostream when there is parsing error..

POSIX has stated that file descriptors 0, 1 or 2 should not be closed. The is stated in many parts, e.g.

Applications should not execute programs with file descriptor 0 not open for reading or with file descriptor 1 or 2 not open for writing, as this might cause the executed program to misbehave. In order not to pass on these file descriptors to an executed program, applications should not just close them but should reopen them on, for example, /dev/null.

The change is justified if you can give an example of a different file descriptor.

I believe that sentence is talking about stdin/stdout/stderr and forking a new process. As long as you don't create a new process, I think you are free to close 0, 1 and 2.

That said I agree with you that we shouldn't close 0, 1 and 2 on exit. I'm not sure if we are doing this at the moment, but if we do, I think we should stop doing that.

Sep 30 2019, 10:50 AM · Restricted Project

Sep 27 2019

ychen created D68164: [Commandline] Clear error on raw_ostream when there is parsing error..
Sep 27 2019, 3:11 PM · Restricted Project

Sep 20 2019

ychen added a comment to D67847: [Support] make report_fatal_error `abort` instead of `exit`.
In D67847#1677251, @rnk wrote:

This has been proposed before by @MatzeB:
https://reviews.llvm.org/D33960

At the time, there was an issue that some people had been "fixing" fuzzer bugs by replacing asserts with if (cond) report_fatal_error(...);. Moving to abort will reopen all of those fuzzer bugs. However, I think we have consensus that that is the desired behavior: every call to report_fatal_error is essentially a bug,. Code should be using the error handlers on MCContext and LLVMContext instead and recovering.

Sep 20 2019, 5:20 PM · Restricted Project, Restricted Project
ychen added a comment to D67847: [Support] make report_fatal_error `abort` instead of `exit`.

Here is another case showing the issue http://lists.llvm.org/pipermail/llvm-dev/2019-September/135283.html

Sep 20 2019, 11:02 AM · Restricted Project, Restricted Project
ychen created D67847: [Support] make report_fatal_error `abort` instead of `exit`.
Sep 20 2019, 11:00 AM · Restricted Project, Restricted Project

Sep 19 2019

ychen committed rGe03663fbb846: [llvm-readobj] flush output before crash (authored by ychen).
[llvm-readobj] flush output before crash
Sep 19 2019, 11:34 PM

Sep 18 2019

ychen updated the diff for D67687: [CodeGen] Define an interface for the new pass manager. (new).

update

Sep 18 2019, 11:06 AM · Restricted Project
ychen updated the diff for D67687: [CodeGen] Define an interface for the new pass manager. (new).

Pass in instead of manage IR unit analysis managers in MachineFunctionIRAnalysisManager

Sep 18 2019, 11:06 AM · Restricted Project

Sep 17 2019

ychen added a comment to D64179: [CodeGen] Define an interface for the new pass manager..

Hello, Charlie went back to school after the internship. I'm trying the address the comments in a new patch. https://reviews.llvm.org/D67687

Sep 17 2019, 8:53 PM · Restricted Project
ychen added reviewers for D67687: [CodeGen] Define an interface for the new pass manager. (new): fedor.sergeev, philip.pfaffe, chandlerc.
Sep 17 2019, 8:44 PM · Restricted Project
ychen created D67687: [CodeGen] Define an interface for the new pass manager. (new).
Sep 17 2019, 7:22 PM · Restricted Project
ychen updated the diff for D64183: [NewPM] Port MachineModuleInfo to the new pass manager..
  • Missing assignment of TheModule in move constructor.
Sep 17 2019, 7:00 PM · Restricted Project

Sep 16 2019

ychen added a comment to D64183: [NewPM] Port MachineModuleInfo to the new pass manager..

ping?

Sep 16 2019, 10:44 AM · Restricted Project