Bug 264318 - security/putty-nogtk: Crashes base clang 14.0.3 on i386
Summary: security/putty-nogtk: Crashes base clang 14.0.3 on i386
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: CURRENT
Hardware: i386 Any
: --- Affects Some People
Assignee: Dimitry Andric
URL: http://beefy17.nyi.freebsd.org/data/m...
Keywords: crash, toolchain
Depends on:
Blocks:
 
Reported: 2022-05-28 19:38 UTC by Matthias Andree
Modified: 2022-05-31 16:02 UTC (History)
2 users (show)

See Also:
koobs: mfc-stable13?
koobs: mfc-stable12?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Andree freebsd_committer freebsd_triage 2022-05-28 19:38:26 UTC
reproducer:

get ports tree onto 14-CURRENT i386

make -C /usr/ports/security/putty-nogtk depends all

clang bombs out crashing compiling the same file in two different fallout reports on i386. see 

http://beefy17.nyi.freebsd.org/data/main-i386-default/pd576f1ee73a3_sa7bb120f8b/logs/putty-0.77.log

sorry, I don't have the capacity to burn this down on 14-CURRENT on i386 with jail not matching kernel.
Comment 1 Dimitry Andric freebsd_committer freebsd_triage 2022-05-28 20:45:15 UTC
I can reproduce, and I think I have found an upstream bug for it, though that isn't resolved yet. Investigating further.
Comment 2 commit-hook freebsd_committer freebsd_triage 2022-05-28 21:35:55 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=6a5eebc190ab98de98ed7977cbdee3218758376e

commit 6a5eebc190ab98de98ed7977cbdee3218758376e
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2022-05-28 21:26:37 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2022-05-28 21:26:37 +0000

    Apply clang fix for assertion failure building putty 0.77 on i386

    Merge commit 45084eab5e63 from llvm git (by Arthur Eubanks):

      [clang] Fix some clang->llvm type cache invalidation issues

      Take the following as an example

        struct z {
          z (*p)();
        };

        z f();

      When we attempt to get the LLVM type of f, we recurse into z. z itself
      has a function pointer with the same type as f. Given the recursion,
      Clang simply treats z::p as a pointer to an empty struct `{}*`. The
      LLVM type of f is as expected. So we have two different potential
      LLVM types for a given Clang type. If we store one of those into the
      cache, when we access the cache with a different context (e.g. we
      are/aren't recursing on z) we may get an incorrect result. There is some
      attempt to clear the cache in these cases, but it doesn't seem to handle
      all cases.

      This change makes it so we only use the cache when we are not in any
      sort of function context, i.e. `noRecordsBeingLaidOut() &&
      FunctionsBeingProcessed.empty()`, which are the cases where we may
      decide to choose a different LLVM type for a given Clang type. LLVM
      types for builtin types are never recursive so they're always ok.

      This allows us to clear the type cache less often (as seen with the
      removal of one of the calls to `TypeCache.clear()`). We
      still need to clear it when we use a placeholder type then replace it
      later with the final type and other dependent types need to be
      recalculated.

      I've added a check that the cached type matches what we compute. It
      triggered in this test case without the fix. It's currently not
      check-clang clean so it's not on by default for something like expensive
      checks builds.

      This change uncovered another issue where the LLVM types for an argument
      and its local temporary don't match. For example in type-cache-3, when
      expanding z::dc's argument into a temporary alloca, we ConvertType() the
      type of z::p which is `void ({}*)*`, which doesn't match the alloca GEP
      type of `{}*`.

      No noticeable compile time changes:
      https://llvm-compile-time-tracker.com/compare.php?from=3918dd6b8acf8c5886b9921138312d1c638b2937&to=50bdec9836ed40e38ece0657f3058e730adffc4c&stat=instructions

      Fixes #53465.

      Reviewed By: rnk

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

    PR:             264318
    Reported by:    mandree
    MFC after:      3 days

 contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h |  5 ++-
 contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp  | 18 ++++++--
 .../clang/lib/CodeGen/CodeGenTypes.cpp             | 52 ++++++++++++++++++----
 3 files changed, 60 insertions(+), 15 deletions(-)
Comment 3 commit-hook freebsd_committer freebsd_triage 2022-05-30 18:31:16 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=2390e2073f12af55d083d98fc124fa8638524e62

commit 2390e2073f12af55d083d98fc124fa8638524e62
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2022-05-28 21:26:37 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2022-05-30 18:28:39 +0000

    Apply clang fix for assertion failure building putty 0.77 on i386

    Merge commit 45084eab5e63 from llvm git (by Arthur Eubanks):

      [clang] Fix some clang->llvm type cache invalidation issues

      Take the following as an example

        struct z {
          z (*p)();
        };

        z f();

      When we attempt to get the LLVM type of f, we recurse into z. z itself
      has a function pointer with the same type as f. Given the recursion,
      Clang simply treats z::p as a pointer to an empty struct `{}*`. The
      LLVM type of f is as expected. So we have two different potential
      LLVM types for a given Clang type. If we store one of those into the
      cache, when we access the cache with a different context (e.g. we
      are/aren't recursing on z) we may get an incorrect result. There is some
      attempt to clear the cache in these cases, but it doesn't seem to handle
      all cases.

      This change makes it so we only use the cache when we are not in any
      sort of function context, i.e. `noRecordsBeingLaidOut() &&
      FunctionsBeingProcessed.empty()`, which are the cases where we may
      decide to choose a different LLVM type for a given Clang type. LLVM
      types for builtin types are never recursive so they're always ok.

      This allows us to clear the type cache less often (as seen with the
      removal of one of the calls to `TypeCache.clear()`). We
      still need to clear it when we use a placeholder type then replace it
      later with the final type and other dependent types need to be
      recalculated.

      I've added a check that the cached type matches what we compute. It
      triggered in this test case without the fix. It's currently not
      check-clang clean so it's not on by default for something like expensive
      checks builds.

      This change uncovered another issue where the LLVM types for an argument
      and its local temporary don't match. For example in type-cache-3, when
      expanding z::dc's argument into a temporary alloca, we ConvertType() the
      type of z::p which is `void ({}*)*`, which doesn't match the alloca GEP
      type of `{}*`.

      No noticeable compile time changes:
      https://llvm-compile-time-tracker.com/compare.php?from=3918dd6b8acf8c5886b9921138312d1c638b2937&to=50bdec9836ed40e38ece0657f3058e730adffc4c&stat=instructions

      Fixes #53465.

      Reviewed By: rnk

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

    PR:             264318
    Reported by:    mandree
    MFC after:      3 days

    (cherry picked from commit 6a5eebc190ab98de98ed7977cbdee3218758376e)

 contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h |  5 ++-
 contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp  | 18 ++++++--
 .../clang/lib/CodeGen/CodeGenTypes.cpp             | 52 ++++++++++++++++++----
 3 files changed, 60 insertions(+), 15 deletions(-)
Comment 4 commit-hook freebsd_committer freebsd_triage 2022-05-31 15:56:45 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=9ac7261611ad130fcfae0da803e2e85c1b9c73f1

commit 9ac7261611ad130fcfae0da803e2e85c1b9c73f1
Author:     Dimitry Andric <dim@FreeBSD.org>
AuthorDate: 2022-05-28 21:26:37 +0000
Commit:     Dimitry Andric <dim@FreeBSD.org>
CommitDate: 2022-05-31 13:17:17 +0000

    Apply clang fix for assertion failure building putty 0.77 on i386

    Merge commit 45084eab5e63 from llvm git (by Arthur Eubanks):

      [clang] Fix some clang->llvm type cache invalidation issues

      Take the following as an example

        struct z {
          z (*p)();
        };

        z f();

      When we attempt to get the LLVM type of f, we recurse into z. z itself
      has a function pointer with the same type as f. Given the recursion,
      Clang simply treats z::p as a pointer to an empty struct `{}*`. The
      LLVM type of f is as expected. So we have two different potential
      LLVM types for a given Clang type. If we store one of those into the
      cache, when we access the cache with a different context (e.g. we
      are/aren't recursing on z) we may get an incorrect result. There is some
      attempt to clear the cache in these cases, but it doesn't seem to handle
      all cases.

      This change makes it so we only use the cache when we are not in any
      sort of function context, i.e. `noRecordsBeingLaidOut() &&
      FunctionsBeingProcessed.empty()`, which are the cases where we may
      decide to choose a different LLVM type for a given Clang type. LLVM
      types for builtin types are never recursive so they're always ok.

      This allows us to clear the type cache less often (as seen with the
      removal of one of the calls to `TypeCache.clear()`). We
      still need to clear it when we use a placeholder type then replace it
      later with the final type and other dependent types need to be
      recalculated.

      I've added a check that the cached type matches what we compute. It
      triggered in this test case without the fix. It's currently not
      check-clang clean so it's not on by default for something like expensive
      checks builds.

      This change uncovered another issue where the LLVM types for an argument
      and its local temporary don't match. For example in type-cache-3, when
      expanding z::dc's argument into a temporary alloca, we ConvertType() the
      type of z::p which is `void ({}*)*`, which doesn't match the alloca GEP
      type of `{}*`.

      No noticeable compile time changes:
      https://llvm-compile-time-tracker.com/compare.php?from=3918dd6b8acf8c5886b9921138312d1c638b2937&to=50bdec9836ed40e38ece0657f3058e730adffc4c&stat=instructions

      Fixes #53465.

      Reviewed By: rnk

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

    PR:             264318
    Reported by:    mandree
    MFC after:      3 days

    (cherry picked from commit 6a5eebc190ab98de98ed7977cbdee3218758376e)

 contrib/llvm-project/clang/lib/CodeGen/CGBuilder.h |  5 ++-
 contrib/llvm-project/clang/lib/CodeGen/CGCall.cpp  | 18 ++++++--
 .../clang/lib/CodeGen/CodeGenTypes.cpp             | 52 ++++++++++++++++++----
 3 files changed, 60 insertions(+), 15 deletions(-)