[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D67847#1847757 , @rnk wrote:

> If we can reland this patch, it should fix this new clang bug:
>  https://bugs.llvm.org/show_bug.cgi?id=44705
>
> This was the list of failures you noted on the commit:
>
> In rG647c3f4e47de8a850ffcaa897db68702d8d2459a#885042 
> , 
> @ychen wrote:
>
> > This is going deeper...
> >
> > - Host compiler crash (looks like ppc64be-specific) 
> > http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23016
> >  http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43016 
> > http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34286
> > - clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp becomes 
> > pass/fail randomly
> > - test timeout 
> > http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/2170/steps/test-openmp/logs/stdio
> > - ThinLTO test failures on Redhat (passes on my ubuntu 18.04) 
> > http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/592
> >
> >   Will investigate.
>
>
> The ppc64be stuff seems like only someone with hardware will be able to debug 
> it.
>  For `clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp`, you are 
> saying you can reproduce that locally?
>  The timeout seems like it could be flakiness or something unrelated.
>  The thinlto failures... I'm not sure either, but again it's PPC64, so it 
> could be flakiness.
>
> It seems like it would be best to alert the owners of those bots and reland 
> and get new results, assuming things pass on less exotic bot configurations.


`p3-2a.cpp` random failure is fixed.
Thinlto / PPC issues are OOM related. PPC admin suggested recommit.

I've confirmed locally that failures on sanitier_windows are real (Patches are 
below), but I have no clue about how they are related.
D73329 
D73327 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

If we can reland this patch, it should fix this new clang bug:
https://bugs.llvm.org/show_bug.cgi?id=44705

This was the list of failures you noted on the commit:

In rG647c3f4e47de8a850ffcaa897db68702d8d2459a#885042 
, 
@ychen wrote:

> This is going deeper...
>
> - Host compiler crash (looks like ppc64be-specific) 
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/23016 
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/43016 
> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-lnt/builds/34286
> - clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp becomes pass/fail 
> randomly
> - test timeout 
> http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/2170/steps/test-openmp/logs/stdio
> - ThinLTO test failures on Redhat (passes on my ubuntu 18.04) 
> http://lab.llvm.org:8011/builders/clang-ppc64le-rhel/builds/592
>
>   Will investigate.


The ppc64be stuff seems like only someone with hardware will be able to debug 
it.
For `clang/test/CXX/temp/temp.arg/temp.arg.template/p3-2a.cpp`, you are saying 
you can reproduce that locally?
The timeout seems like it could be flakiness or something unrelated.
The thinlto failures... I'm not sure either, but again it's PPC64, so it could 
be flakiness.

It seems like it would be best to alert the owners of those bots and reland and 
get new results, assuming things pass on less exotic bot configurations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61894 tests passed, 0 failed 
and 782 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D67847#1693694 , @rnk wrote:

> In D67847#1691898 , @jyknight wrote:
>
> > 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?
>
>
> It's easy to crash LLVM even without this change, so anyone running LLVM 
> better have core dumps configured the way they like. Failed asserts raise 
> SIGABRT already, for example, and we have tons of those. The only difference 
> is that now end users, who may have never configured this stuff, may see more 
> crashes. If it's a problem, I'd consider it QoI: we should fix the 
> report_fatal_error to use proper diagnostics anyway so end users don't see 
> them as often, just as we would treat any other user-visible crash.


That may be, but right now we do report a bunch of errors via 
report_fatal_error, even if we should not. Coredumps seem unlikely to be of use 
in diagnosing the issue involved (unlike, perhaps, with a segfault), so I don't 
see why we'd _want_ to call abort vs quick_exit/_exit.

I express this as my opinion, but do not veto acceptance given by others.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

I'm still in favor of this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon times-circle color=red} Unit tests: fail. 61892 tests passed, 2 failed 
and 782 were skipped.

  failed: Clang.CXX/temp/temp_arg/temp_arg_template/p3-2a.cpp
  failed: LLVM.CodeGen/SystemZ/mverify-optypes.mir

{icon question-circle color=gray} clang-tidy: unknown.

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Reid Kleckner via Phabricator via cfe-commits]

rnk accepted this revision.
rnk added a comment.

In D67847#1691898 , @jyknight wrote:

> 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?


It's easy to crash LLVM even without this change, so anyone running LLVM better 
have core dumps configured the way they like. Failed asserts raise SIGABRT 
already, for example, and we have tons of those. The only difference is that 
now end users, who may have never configured this stuff, may see more crashes. 
If it's a problem, I'd consider it QoI: we should fix the report_fatal_error to 
use proper diagnostics anyway so end users don't see them as often, just as we 
would treat any other user-visible crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D67847#1691898 , @jyknight wrote:

> 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?


I think this patch is more for the purpose of making error handling better than 
for fixing PR35547. Fixing PR35547 is probably just a side effect. Apologies 
that I should have updated the summary to reflect that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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

[PATCH] D67847: [Support] make report_fatal_error `abort` instead of `exit`

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67847



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