Implement support for PPC64 on Linux [crashpad/crashpad : master]

117 views
Skip to first unread message

Shawn Anastasio (Gerrit)

unread,
Sep 25, 2018, 7:05:56 AM9/25/18
to tpea...@raptorengineering.com, crashp...@chromium.org

This change is ready for review.

View Change

    To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

    Gerrit-Project: crashpad/crashpad
    Gerrit-Branch: master
    Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
    Gerrit-Change-Number: 1242633
    Gerrit-PatchSet: 1
    Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
    Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
    Gerrit-Comment-Date: Tue, 25 Sep 2018 06:09:20 +0000
    Gerrit-HasComments: No
    Gerrit-Has-Labels: No
    Gerrit-MessageType: comment

    Mark Mentovai (Gerrit)

    unread,
    Sep 25, 2018, 9:51:15 AM9/25/18
    to Shawn Anastasio, tpea...@raptorengineering.com, Dan Horák, crashp...@chromium.org

    I appreciate the effort you’ve put in, but I’m not too keen to carry a substantial chunk of code that most of our developers have no way to validate, debug, or work on. The bare minimum to make me comfortable with this would be to either:

     - Clearly cordon off the new contribution as “contrib” and maintain it only on a best-effort basis. This would mean putting it in a separate directory, although at that point, a distinct overlay repository might make more sense.
    - Provide sufficient test infrastructure (try- and buildbots) to provide assurance that this port functions as intended and continues to work properly.

    Note that I have the same concerns about MIPS: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1064594#message-6c0734640a0c8117a0a44302f340bd38a2d31296.

    View Change

      To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

      Gerrit-Project: crashpad/crashpad
      Gerrit-Branch: master
      Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
      Gerrit-Change-Number: 1242633
      Gerrit-PatchSet: 2
      Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
      Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
      Gerrit-CC: Dan Horák <d...@danny.cz>
      Gerrit-CC: Mark Mentovai <ma...@chromium.org>
      Gerrit-Comment-Date: Tue, 25 Sep 2018 13:51:12 +0000

      Timothy Pearson (Gerrit)

      unread,
      Sep 25, 2018, 4:13:58 PM9/25/18
      to Shawn Anastasio, tpea...@raptorengineering.com, Mark Mentovai, Dan Horák, crashp...@chromium.org

      Patch Set 2:

      I appreciate the effort you’ve put in, but I’m not too keen to carry a substantial chunk of code that most of our developers have no way to validate, debug, or work on. The bare minimum to make me comfortable with this would be to either:

       - Clearly cordon off the new contribution as “contrib” and maintain it only on a best-effort basis. This would mean putting it in a separate directory, although at that point, a distinct overlay repository might make more sense.
      - Provide sufficient test infrastructure (try- and buildbots) to provide assurance that this port functions as intended and continues to work properly.

      Note that I have the same concerns about MIPS: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1064594#message-6c0734640a0c8117a0a44302f340bd38a2d31296.

      Raptor Engineering is willing to offer dedicated build bot instances at no charge, to support this effort and its continued maintenance. These would be set up via the Integricloud platform, and as such can either be maintained by Raptor or by Google, depending on what the requirements are. They would be running on POWER9 SMT4 systems.

      View Change

        To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

        Gerrit-Project: crashpad/crashpad
        Gerrit-Branch: master
        Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
        Gerrit-Change-Number: 1242633
        Gerrit-PatchSet: 2
        Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
        Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
        Gerrit-CC: Dan Horák <d...@danny.cz>
        Gerrit-CC: Mark Mentovai <ma...@chromium.org>
        Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
        Gerrit-Comment-Date: Tue, 25 Sep 2018 20:13:20 +0000

        J Lynn (Gerrit)

        unread,
        Sep 25, 2018, 5:12:53 PM9/25/18
        to Shawn Anastasio, j...@jaesharp.com, tpea...@raptorengineering.com, Timothy Pearson, Mark Mentovai, Dan Horák, crashp...@chromium.org

        Patch Set 2:

        I appreciate the effort you’ve put in, but I’m not too keen to carry a substantial chunk of code that most of our developers have no way to validate, debug, or work on. The bare minimum to make me comfortable with this would be to either:

         - Clearly cordon off the new contribution as “contrib” and maintain it only on a best-effort basis. This would mean putting it in a separate directory, although at that point, a distinct overlay repository might make more sense.
        - Provide sufficient test infrastructure (try- and buildbots) to provide assurance that this port functions as intended and continues to work properly.

        Note that I have the same concerns about MIPS: https://chromium-review.googlesource.com/c/crashpad/crashpad/+/1064594#message-6c0734640a0c8117a0a44302f340bd38a2d31296.

        I too would like to offer build and try-bot instances on an ongoing basis for this. They'll be physically hosted on a personally owned cluster of Talos 2 POWER9 Hardware either in Virtual Machines (where they may be either maintained, preferably, by myself or, if required, Google engineers) or as tenants of a K8s cluster (where they may, again, be either maintained, preferably, by myself or, if required, Google engineers).

        View Change

          To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

          Gerrit-Project: crashpad/crashpad
          Gerrit-Branch: master
          Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
          Gerrit-Change-Number: 1242633
          Gerrit-PatchSet: 2
          Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
          Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
          Gerrit-CC: Dan Horák <d...@danny.cz>
          Gerrit-CC: J Lynn <justi...@gmail.com>
          Gerrit-CC: Mark Mentovai <ma...@chromium.org>
          Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
          Gerrit-Comment-Date: Tue, 25 Sep 2018 21:10:43 +0000

          Timothy Pearson (Gerrit)

          unread,
          Oct 17, 2018, 3:55:58 AM10/17/18
          to Shawn Anastasio, j...@jaesharp.com, tpea...@raptorengineering.com, J Lynn, Mark Mentovai, Dan Horák, crashp...@chromium.org

          What do we need to do to get this moving again? We can provide bare metal servers immediately in support of this project; I don't want to have to inject patches downstream into the distros but we're running out of options here.

          View Change

            To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

            Gerrit-Project: crashpad/crashpad
            Gerrit-Branch: master
            Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
            Gerrit-Change-Number: 1242633
            Gerrit-PatchSet: 4
            Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
            Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
            Gerrit-CC: Dan Horák <d...@danny.cz>
            Gerrit-CC: J Lynn <justi...@gmail.com>
            Gerrit-CC: Mark Mentovai <ma...@chromium.org>
            Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
            Gerrit-Comment-Date: Wed, 17 Oct 2018 07:55:55 +0000

            J Lynn (Gerrit)

            unread,
            Oct 17, 2018, 5:26:39 AM10/17/18
            to Shawn Anastasio, j...@jaesharp.com, tpea...@raptorengineering.com, Timothy Pearson, Mark Mentovai, Dan Horák, crashp...@chromium.org

            Patch Set 4:

            What do we need to do to get this moving again? We can provide bare metal servers immediately in support of this project; I don't want to have to inject patches downstream into the distros but we're running out of options here.

            Indeed, and please allow me to reiterate my own offer of compute for independent builds as well. - J

            View Change

              To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

              Gerrit-Project: crashpad/crashpad
              Gerrit-Branch: master
              Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
              Gerrit-Change-Number: 1242633
              Gerrit-PatchSet: 4
              Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
              Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
              Gerrit-CC: Dan Horák <d...@danny.cz>
              Gerrit-CC: J Lynn <justi...@gmail.com>
              Gerrit-CC: Mark Mentovai <ma...@chromium.org>
              Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
              Gerrit-Comment-Date: Wed, 17 Oct 2018 09:26:35 +0000

              Timothy Pearson (Gerrit)

              unread,
              Oct 25, 2018, 8:40:20 PM10/25/18
              to Shawn Anastasio, j...@jaesharp.com, tpea...@raptorengineering.com, J Lynn, Mark Mentovai, Dan Horák, crashp...@chromium.org

              We're now at one month since the initial request with no further communication or action taken. Any chance we can get this moving again?

              View Change

                To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-Project: crashpad/crashpad
                Gerrit-Branch: master
                Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
                Gerrit-Change-Number: 1242633
                Gerrit-PatchSet: 4
                Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-CC: Dan Horák <d...@danny.cz>
                Gerrit-CC: J Lynn <justi...@gmail.com>
                Gerrit-CC: Mark Mentovai <ma...@chromium.org>
                Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
                Gerrit-Comment-Date: Fri, 26 Oct 2018 00:40:17 +0000

                Witold Baryluk (Gerrit)

                unread,
                Nov 30, 2018, 1:32:36 PM11/30/18
                to Shawn Anastasio, j...@jaesharp.com, tpea...@raptorengineering.com, Anton Blanchard, Aljoscha Vollmerhaus, J Lynn, Timothy Pearson, Mark Mentovai, Dan Horák, crashp...@chromium.org

                Just few hopefully helpful review comments.

                View Change

                14 comments:

                To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-Project: crashpad/crashpad
                Gerrit-Branch: master
                Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
                Gerrit-Change-Number: 1242633
                Gerrit-PatchSet: 5
                Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-CC: Aljoscha Vollmerhaus <avollm...@googlemail.com>
                Gerrit-CC: Anton Blanchard <anton.b...@gmail.com>
                Gerrit-CC: Dan Horák <d...@danny.cz>
                Gerrit-CC: J Lynn <justi...@gmail.com>
                Gerrit-CC: Mark Mentovai <ma...@chromium.org>
                Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
                Gerrit-CC: Witold Baryluk <witold....@gmail.com>
                Gerrit-Comment-Date: Fri, 30 Nov 2018 18:27:29 +0000
                Gerrit-HasComments: Yes
                Gerrit-Has-Labels: No
                Gerrit-MessageType: comment

                Witold Baryluk (Gerrit)

                unread,
                Nov 30, 2018, 1:32:36 PM11/30/18
                to Shawn Anastasio, j...@jaesharp.com, tpea...@raptorengineering.com, Anton Blanchard, Aljoscha Vollmerhaus, J Lynn, Timothy Pearson, Mark Mentovai, Dan Horák, crashp...@chromium.org

                View Change

                1 comment:

                  • How about […]

                    +cost

                    static const size_t page_size = sysconf(_SC_PAGESIZE);

                    getpagesize() should also be okish, because this is linux specific source code anyway.

                To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-Project: crashpad/crashpad
                Gerrit-Branch: master
                Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
                Gerrit-Change-Number: 1242633
                Gerrit-PatchSet: 5
                Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-CC: Aljoscha Vollmerhaus <avollm...@googlemail.com>
                Gerrit-CC: Anton Blanchard <anton.b...@gmail.com>
                Gerrit-CC: Dan Horák <d...@danny.cz>
                Gerrit-CC: J Lynn <justi...@gmail.com>
                Gerrit-CC: Mark Mentovai <ma...@chromium.org>
                Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
                Gerrit-CC: Witold Baryluk <witold....@gmail.com>
                Gerrit-Comment-Date: Fri, 30 Nov 2018 18:31:05 +0000
                Gerrit-HasComments: Yes
                Gerrit-Has-Labels: No
                Comment-In-Reply-To: Witold Baryluk <witold....@gmail.com>
                Gerrit-MessageType: comment

                Shawn Anastasio (Gerrit)

                unread,
                Jan 7, 2019, 1:12:41 PM1/7/19
                to j...@jaesharp.com, tpea...@raptorengineering.com, Witold Baryluk, Anton Blanchard, Aljoscha Vollmerhaus, J Lynn, Timothy Pearson, Mark Mentovai, Dan Horák, crashp...@chromium.org

                Patch Set 5:

                (14 comments)

                Just few hopefully helpful review comments.

                Thanks for the comments. I've added some responses below.

                View Change

                13 comments:

                  • +cost […]

                    getpagesize() was chosen because it is already used throughout the code base. If the crashpad team feels that the sysconf API is more appropriate, I could go in and change all occurrences, but for now I think it's best to be consistent.

                    As for declaring it static, that's a good idea and I'll incorporate it in my next rebase.

                • File util/linux/ptracer.cc:

                  • Comment should be updated? […]

                    It was understanding that the codebase's style is in fact to derive the closing comment from the first preprocessor conditional. If this is not the case, I would be happy to go in and correct them.

                  • Wouldn't it make more sense to define this function on all archs, and create dummy versions for othe […]

                    From my point of view it could go either way, especially since this is the only invocation of the function. Does anybody have any strong feelings on this?

                • File util/linux/thread_info.h:

                  • See above response.

                  • See above response.

                  • See above response.

                  • I think it is not relevant that PPC64 is 64-bit in the context of this class. […]

                    This struct contains the floating point context for 32-bit variants of the supported architectures. The comment was meant to explain that since the port targets PPC64 only, the struct is declared as empty.

                    I'll update it to be more clear in the next rebase.

                  • See above response.

                  • See above response.

                  • #if !(defined(ARCH_CPU_ARM64) || defined(ARCH_CPU_PPC64)) […]

                    I think this is a rather subjective thing. Personally, I find the current conditional to be slightly easier to reason about.

                    That being said, if you or anybody else feels strongly about changing it, I can do so.

                  • Patch Set #5, Line 53: #endif // defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARMEL

                    ) missing at the end of the comment?

                  • Good catch. Will update in the next rebase.

                  • I think this comment (both about ARM64 and PPC64; not sure what is the behavior of PPC32) should be […]

                    My goal here again was to replicate the existing style in the code base and from what I can tell this seems to be the preferred method of inserting comments that apply to preprocessor statement blocks.

                    That being said, if you can find some examples in the code base that contradict this or a team member chimes in I can change it.

                To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.

                Gerrit-Project: crashpad/crashpad
                Gerrit-Branch: master
                Gerrit-Change-Id: I04e5d88f2619e0a13da1fb2721417b21e0be3693
                Gerrit-Change-Number: 1242633
                Gerrit-PatchSet: 5
                Gerrit-Owner: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-Reviewer: Shawn Anastasio <shawnan...@yahoo.com>
                Gerrit-CC: Aljoscha Vollmerhaus <avollm...@googlemail.com>
                Gerrit-CC: Anton Blanchard <anton.b...@gmail.com>
                Gerrit-CC: Dan Horák <d...@danny.cz>
                Gerrit-CC: J Lynn <justi...@gmail.com>
                Gerrit-CC: Mark Mentovai <ma...@chromium.org>
                Gerrit-CC: Timothy Pearson <tpearso...@gmail.com>
                Gerrit-CC: Witold Baryluk <witold....@gmail.com>
                Gerrit-Comment-Date: Mon, 07 Jan 2019 18:05:59 +0000
                Reply all
                Reply to author
                Forward
                0 new messages