Bug 237515 - textproc/htmldoc hangs at runtime when compiled with clang 8
Summary: textproc/htmldoc hangs at runtime when compiled with clang 8
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-toolchain mailing list
URL:
Keywords: needs-qa, regression
Depends on:
Blocks:
 
Reported: 2019-04-24 06:50 UTC by Tobias Kortkamp
Modified: 2019-07-12 19:48 UTC (History)
2 users (show)

See Also:
bugzilla: maintainer-feedback? (jose)


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Kortkamp freebsd_committer 2019-04-24 06:50:14 UTC
This might be an optimizer regression in clang 8 since it builds and runs fine
when compiled with clang 7.  Please see the upstream issue [1] and recent build
logs [2] for a tiny bit more details.

I don't know how to really debug this, so help would be appreciated.
I've applied a workaround for this in ports r499817, so to test use
a revision before that or directly build from the upstream repository.

[1] https://github.com/michaelrsweet/htmldoc/issues/349
[2] http://beefy12.nyi.freebsd.org/data/head-amd64-default/p499421_s346424/logs/errors/htmldoc-1.9.3.log
Comment 1 Dimitry Andric freebsd_committer 2019-04-24 17:41:29 UTC
I can reproduce the issue, and it seems to be caused by a loop optimization after https://reviews.llvm.org/rL346397.  There is an upstream PR, https://bugs.llvm.org/show_bug.cgi?id=39673, which is maybe related.
Comment 2 Dimitry Andric freebsd_committer 2019-05-23 19:58:45 UTC
I managed to reduce the issue to a fairly small test case, and submitted a new upstream bug: https://bugs.llvm.org/show_bug.cgi?id=41998
Comment 3 commit-hook freebsd_committer 2019-07-01 21:06:44 UTC
A commit references this bug:

Author: dim
Date: Mon Jul  1 21:06:11 UTC 2019
New revision: 349583
URL: https://svnweb.freebsd.org/changeset/base/349583

Log:
  Pull in r360968 from upstream llvm trunk (by Philip Reames):

    Clarify comments on helpers used by LFTR [NFC]

    I'm slowly wrapping my head around this code, and am making comment
    improvements where I can.

  Pull in r360972 from upstream llvm trunk (by Philip Reames):

    [LFTR] Factor out a helper function for readability purpose [NFC]

  Pull in r360976 from upstream llvm trunk (by Philip Reames):

    [IndVars] Don't reimplement Loop::isLoopInvariant [NFC]

    Using dominance vs a set membership check is indistinguishable from a
    compile time perspective, and the two queries return equivelent
    results.  Simplify code by using the existing function.

  Pull in r360978 from upstream llvm trunk (by Philip Reames):

    [LFTR] Strengthen assertions in genLoopLimit [NFCI]

  Pull in r362292 from upstream llvm trunk (by Nikita Popov):

    [IndVarSimplify] Fixup nowrap flags during LFTR (PR31181)

    Fix for https://bugs.llvm.org/show_bug.cgi?id=31181 and partial fix
    for LFTR poison handling issues in general.

    When LFTR moves a condition from pre-inc to post-inc, it may now
    depend on value that is poison due to nowrap flags. To avoid this, we
    clear any nowrap flag that SCEV cannot prove for the post-inc addrec.

    Additionally, LFTR may switch to a different IV that is dynamically
    dead and as such may be arbitrarily poison. This patch will correct
    nowrap flags in some but not all cases where this happens. This is
    related to the adoption of IR nowrap flags for the pre-inc addrec.
    (See some of the switch_to_different_iv tests, where flags are not
    dropped or insufficiently dropped.)

    Finally, there are likely similar issues with the handling of GEP
    inbounds, but we don't have a test case for this yet.

    Differential Revision: https://reviews.llvm.org/D60935

  Pull in r362971 from upstream llvm trunk (by Philip Reames):

    Prepare for multi-exit LFTR [NFC]

    This change does the plumbing to wire an ExitingBB parameter through
    the LFTR implementation, and reorganizes the code to work in terms of
    a set of individual loop exits. Most of it is fairly obvious, but
    there's one key complexity which makes it worthy of consideration.
    The actual multi-exit LFTR patch is in D62625 for context.

    Specifically, it turns out the existing code uses the backedge taken
    count from before a IV is widened. Oddly, we can end up with a
    different (more expensive, but semantically equivelent) BE count for
    the loop when requerying after widening.  For the nestedIV example
    from elim-extend, we end up with the following BE counts:
    BEFORE: (-2 + (-1 * %innercount) + %limit)
    AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>)

    This is the only test in tree which seems sensitive to this
    difference. The actual result of using the wider BETC on this example
    is that we actually produce slightly better code. :)

    In review, we decided to accept that test change.  This patch is
    structured to preserve the old behavior, but a separate change will
    immediate follow with the behavior change.  (I wanted it separate for
    problem attribution purposes.)

    Differential Revision: https://reviews.llvm.org/D62880

  Pull in r362975 from upstream llvm trunk (by Philip Reames):

    [LFTR] Use recomputed BE count

    This was discussed as part of D62880.  The basic thought is that
    computing BE taken count after widening should produce (on average)
    an equally good backedge taken count as the one before widening.
    Since there's only one test in the suite which is impacted by this
    change, and it's essentially equivelent codegen, that seems to be a
    reasonable assertion.  This change was separated from r362971 so that
    if this turns out to be problematic, the triggering piece is obvious
    and easily revertable.

    For the nestedIV example from elim-extend.ll, we end up with the
    following BE counts:
    BEFORE: (-2 + (-1 * %innercount) + %limit)
    AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>)

    Note that before is an i32 type, and the after is an i64.  Truncating
    the i64 produces the i32.

  Pull in r362980 from upstream llvm trunk (by Philip Reames):

    Factor out a helper function for readability and reuse in a future
    patch [NFC]

  Pull in r363613 from upstream llvm trunk (by Philip Reames):

    Fix a bug w/inbounds invalidation in LFTR (recommit)

    Recommit r363289 with a bug fix for crash identified in pr42279.
    Issue was that a loop exit test does not have to be an icmp, leading
    to a null dereference crash when new logic was exercised for that
    case.  Test case previously committed in r363601.

    Original commit comment follows:

    This contains fixes for two cases where we might invalidate inbounds
    and leave it stale in the IR (a miscompile). Case 1 is when switching
    to an IV with no dynamically live uses, and case 2 is when doing
    pre-to-post conversion on the same pointer type IV.

    The basic scheme used is to prove that using the given IV (pre or
    post increment forms) would have to already trigger UB on the path to
    the test we're modifying. As such, our potential UB triggering use
    does not change the semantics of the original program.

    As was pointed out in the review thread by Nikita, this is defending
    against a separate issue from the hasConcreteDef case. This is about
    poison, that's about undef. Unfortunately, the two are different, see
    Nikita's comment for a fuller explanation, he explains it well.

    (Note: I'm going to address Nikita's last style comment in a separate
    commit just to minimize chance of subtle bugs being introduced due to
    typos.)

    Differential Revision: https://reviews.llvm.org/D62939

  Pull in r363875 from upstream llvm trunk (by Philip Reames):

    [LFTR] Rename variable to minimize confusion [NFC]

    (Recommit of r363293 which was reverted when a dependent patch was.)

    As pointed out by Nikita in D62625, BackedgeTakenCount is generally
    used to refer to the backedge taken count of the loop. A conditional
    backedge taken count - one which only applies if a particular exit is
    taken - is called a ExitCount in SCEV code, so be consistent here.

  Pull in r363877 from upstream llvm trunk (by Philip Reames):

    [LFTR] Stylistic cleanup as suggested in last review comment of
    D62939 [NFC]

    (Resumbit of r363292 which was reverted along w/an earlier patch)

  Pull in r364346 from upstream llvm trunk (by Philip Reames):

    [LFTR] Adjust debug output to include extensions (if any)

  Pull in r364693 from upstream llvm trunk (by Philip Reames):

    [IndVars] Remove a bit of manual constant folding [NFC]

    SCEV is more than capable of folding (add x, trunc(0)) to x.

  Pull in r364709 from upstream llvm trunk (by Nikita Popov):

    [LFTR] Fix post-inc pointer IV with truncated exit count (PR41998)

    Fixes https://bugs.llvm.org/show_bug.cgi?id=41998. Usually when we
    have a truncated exit count we'll truncate the IV when comparing
    against the limit, in which case exit count overflow in post-inc form
    doesn't matter. However, for pointer IVs we don't do that, so we have
    to be careful about incrementing the IV in the wide type.

    I'm fixing this by removing the IVCount variable (which was ExitCount
    or ExitCount+1) and replacing it with a UsePostInc flag, and then
    moving the actual limit adjustment to the individual cases (which
    are: pointer IV where we add to the wide type, integer IV where we
    add to the narrow type, and constant integer IV where we add to the
    wide type).

    Differential Revision: https://reviews.llvm.org/D63686

  Together, these should fix a hang when building the textproc/htmldoc
  port, due to an incorrect loop optimization.

  PR:		237515
  MFC after:	1 week

Changes:
  head/contrib/llvm/include/llvm/Analysis/ValueTracking.h
  head/contrib/llvm/lib/Analysis/ValueTracking.cpp
  head/contrib/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
Comment 4 commit-hook freebsd_committer 2019-07-12 19:44:49 UTC
A commit references this bug:

Author: dim
Date: Fri Jul 12 19:44:00 UTC 2019
New revision: 349954
URL: https://svnweb.freebsd.org/changeset/base/349954

Log:
  MFC r349583:

  Pull in r360968 from upstream llvm trunk (by Philip Reames):

    Clarify comments on helpers used by LFTR [NFC]

    I'm slowly wrapping my head around this code, and am making comment
    improvements where I can.

  Pull in r360972 from upstream llvm trunk (by Philip Reames):

    [LFTR] Factor out a helper function for readability purpose [NFC]

  Pull in r360976 from upstream llvm trunk (by Philip Reames):

    [IndVars] Don't reimplement Loop::isLoopInvariant [NFC]

    Using dominance vs a set membership check is indistinguishable from a
    compile time perspective, and the two queries return equivelent
    results.  Simplify code by using the existing function.

  Pull in r360978 from upstream llvm trunk (by Philip Reames):

    [LFTR] Strengthen assertions in genLoopLimit [NFCI]

  Pull in r362292 from upstream llvm trunk (by Nikita Popov):

    [IndVarSimplify] Fixup nowrap flags during LFTR (PR31181)

    Fix for https://bugs.llvm.org/show_bug.cgi?id=31181 and partial fix
    for LFTR poison handling issues in general.

    When LFTR moves a condition from pre-inc to post-inc, it may now
    depend on value that is poison due to nowrap flags. To avoid this, we
    clear any nowrap flag that SCEV cannot prove for the post-inc addrec.

    Additionally, LFTR may switch to a different IV that is dynamically
    dead and as such may be arbitrarily poison. This patch will correct
    nowrap flags in some but not all cases where this happens. This is
    related to the adoption of IR nowrap flags for the pre-inc addrec.
    (See some of the switch_to_different_iv tests, where flags are not
    dropped or insufficiently dropped.)

    Finally, there are likely similar issues with the handling of GEP
    inbounds, but we don't have a test case for this yet.

    Differential Revision: https://reviews.llvm.org/D60935

  Pull in r362971 from upstream llvm trunk (by Philip Reames):

    Prepare for multi-exit LFTR [NFC]

    This change does the plumbing to wire an ExitingBB parameter through
    the LFTR implementation, and reorganizes the code to work in terms of
    a set of individual loop exits. Most of it is fairly obvious, but
    there's one key complexity which makes it worthy of consideration.
    The actual multi-exit LFTR patch is in D62625 for context.

    Specifically, it turns out the existing code uses the backedge taken
    count from before a IV is widened. Oddly, we can end up with a
    different (more expensive, but semantically equivelent) BE count for
    the loop when requerying after widening.  For the nestedIV example
    from elim-extend, we end up with the following BE counts:
    BEFORE: (-2 + (-1 * %innercount) + %limit)
    AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>)

    This is the only test in tree which seems sensitive to this
    difference. The actual result of using the wider BETC on this example
    is that we actually produce slightly better code. :)

    In review, we decided to accept that test change.  This patch is
    structured to preserve the old behavior, but a separate change will
    immediate follow with the behavior change.  (I wanted it separate for
    problem attribution purposes.)

    Differential Revision: https://reviews.llvm.org/D62880

  Pull in r362975 from upstream llvm trunk (by Philip Reames):

    [LFTR] Use recomputed BE count

    This was discussed as part of D62880.  The basic thought is that
    computing BE taken count after widening should produce (on average)
    an equally good backedge taken count as the one before widening.
    Since there's only one test in the suite which is impacted by this
    change, and it's essentially equivelent codegen, that seems to be a
    reasonable assertion.  This change was separated from r362971 so that
    if this turns out to be problematic, the triggering piece is obvious
    and easily revertable.

    For the nestedIV example from elim-extend.ll, we end up with the
    following BE counts:
    BEFORE: (-2 + (-1 * %innercount) + %limit)
    AFTER: (-1 + (sext i32 (-1 + %limit) to i64) + (-1 * (sext i32 %innercount to i64))<nsw>)

    Note that before is an i32 type, and the after is an i64.  Truncating
    the i64 produces the i32.

  Pull in r362980 from upstream llvm trunk (by Philip Reames):

    Factor out a helper function for readability and reuse in a future
    patch [NFC]

  Pull in r363613 from upstream llvm trunk (by Philip Reames):

    Fix a bug w/inbounds invalidation in LFTR (recommit)

    Recommit r363289 with a bug fix for crash identified in pr42279.
    Issue was that a loop exit test does not have to be an icmp, leading
    to a null dereference crash when new logic was exercised for that
    case.  Test case previously committed in r363601.

    Original commit comment follows:

    This contains fixes for two cases where we might invalidate inbounds
    and leave it stale in the IR (a miscompile). Case 1 is when switching
    to an IV with no dynamically live uses, and case 2 is when doing
    pre-to-post conversion on the same pointer type IV.

    The basic scheme used is to prove that using the given IV (pre or
    post increment forms) would have to already trigger UB on the path to
    the test we're modifying. As such, our potential UB triggering use
    does not change the semantics of the original program.

    As was pointed out in the review thread by Nikita, this is defending
    against a separate issue from the hasConcreteDef case. This is about
    poison, that's about undef. Unfortunately, the two are different, see
    Nikita's comment for a fuller explanation, he explains it well.

    (Note: I'm going to address Nikita's last style comment in a separate
    commit just to minimize chance of subtle bugs being introduced due to
    typos.)

    Differential Revision: https://reviews.llvm.org/D62939

  Pull in r363875 from upstream llvm trunk (by Philip Reames):

    [LFTR] Rename variable to minimize confusion [NFC]

    (Recommit of r363293 which was reverted when a dependent patch was.)

    As pointed out by Nikita in D62625, BackedgeTakenCount is generally
    used to refer to the backedge taken count of the loop. A conditional
    backedge taken count - one which only applies if a particular exit is
    taken - is called a ExitCount in SCEV code, so be consistent here.

  Pull in r363877 from upstream llvm trunk (by Philip Reames):

    [LFTR] Stylistic cleanup as suggested in last review comment of
    D62939 [NFC]

    (Resumbit of r363292 which was reverted along w/an earlier patch)

  Pull in r364346 from upstream llvm trunk (by Philip Reames):

    [LFTR] Adjust debug output to include extensions (if any)

  Pull in r364693 from upstream llvm trunk (by Philip Reames):

    [IndVars] Remove a bit of manual constant folding [NFC]

    SCEV is more than capable of folding (add x, trunc(0)) to x.

  Pull in r364709 from upstream llvm trunk (by Nikita Popov):

    [LFTR] Fix post-inc pointer IV with truncated exit count (PR41998)

    Fixes https://bugs.llvm.org/show_bug.cgi?id=41998. Usually when we
    have a truncated exit count we'll truncate the IV when comparing
    against the limit, in which case exit count overflow in post-inc form
    doesn't matter. However, for pointer IVs we don't do that, so we have
    to be careful about incrementing the IV in the wide type.

    I'm fixing this by removing the IVCount variable (which was ExitCount
    or ExitCount+1) and replacing it with a UsePostInc flag, and then
    moving the actual limit adjustment to the individual cases (which
    are: pointer IV where we add to the wide type, integer IV where we
    add to the narrow type, and constant integer IV where we add to the
    wide type).

    Differential Revision: https://reviews.llvm.org/D63686

  Together, these should fix a hang when building the textproc/htmldoc
  port, due to an incorrect loop optimization.

  PR:		237515

Changes:
_U  stable/11/
  stable/11/contrib/llvm/include/llvm/Analysis/ValueTracking.h
  stable/11/contrib/llvm/lib/Analysis/ValueTracking.cpp
  stable/11/contrib/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
_U  stable/12/
  stable/12/contrib/llvm/include/llvm/Analysis/ValueTracking.h
  stable/12/contrib/llvm/lib/Analysis/ValueTracking.cpp
  stable/12/contrib/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp