Implement support for PPC64 on Linux [crashpad/crashpad : master]
Shawn Anastasio (Gerrit)
This change is ready for review.
To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.
Mark Mentovai (Gerrit)
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.
Timothy Pearson (Gerrit)
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.
J Lynn (Gerrit)
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).
Timothy Pearson (Gerrit)
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.
J Lynn (Gerrit)
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
Timothy Pearson (Gerrit)
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?
Witold Baryluk (Gerrit)
Just few hopefully helpful review comments.
14 comments:
File util/linux/ptrace_broker.cc:
Patch Set #5, Line 96:
size_t page_size = getpagesize();How about
static size_t page_size = sysconf(_SC_PAGESIZE);
instead?
computed once, and not using legacy API.
Patch Set #5, Line 457:
#endif // ARCH_CPU_X86_FAMILYComment should be updated?
i.e.
#elif defined(ARCH_CPU_PPC64_FAMILY) // end of ARCH_CPU_MIPS_FAMILY
...
...
#else // end of ARCH_CPU_PPC64_FAMILY
#error Port.
#endif
yes, end of MIPS, not X86, if you look at the code blocks above.Unless the code style in chromium is to add the comment that is derived from the opening of the first #if (which is indeed ARCH_CPU_X86_FAMILY still).
Patch Set #5, Line 548:
GetVectorRegisters64(tid, &info->vector_context, can_log_) &&Wouldn't it make more sense to define this function on all archs, and create dummy versions for other archs?
File util/linux/thread_info.h:
Patch Set #5, Line 90:
#endif // ARCH_CPU_X86_FAMILYDitto. Outdated comment.
Patch Set #5, Line 158:
#endif // ARCH_CPU_X86_FAMILYditto
Patch Set #5, Line 171:
#endif // ARCH_CPU_X86_FAMILY || ARCH_CPU_ARM64ditto
Patch Set #5, Line 245:
// PPC64 is 64-bitI think it is not relevant that PPC64 is 64-bit in the context of this class.
A better comment maybe?
Patch Set #5, Line 248:
#endif // ARCH_CPU_X86_FAMILYditto
Patch Set #5, Line 286:
#endif // ARCH_CPU_X86_FAMILYditto
Patch Set #5, Line 316:
#endif // ARCH_CPU_X86ditto
Patch Set #5, Line 325:
__attribute__((__aligned__(16))) // Vector context must be doubleword aligned.Would it hurt (question to chromium team), just to leave it aligned 16 on all archs by default?
File util/posix/signals_test.cc:
Patch Set #5, Line 48:
#if !defined(ARCH_CPU_ARM64) && !defined(ARCH_CPU_PPC64)#if !(defined(ARCH_CPU_ARM64) || defined(ARCH_CPU_PPC64))
?
It just feels easier to think about.
similarly below.
Patch Set #5, Line 53:
#endif // defined(ARCH_CPU_X86_FAMILY) || defined(ARCH_CPU_ARMEL) missing at the end of the comment?
Patch Set #5, Line 123:
// PPC64 fixed-point division by zero also doesn't produce a SIGFPE.I think this comment (both about ARM64 and PPC64; not sure what is the behavior of PPC32) should be BEFORE the #if block.
To view, visit change 1242633. To unsubscribe, or for help writing mail filters, visit settings.
Witold Baryluk (Gerrit)
1 comment:
Patch Set #5, Line 96:
size_t page_size = getpagesize();
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.
Shawn Anastasio (Gerrit)
Patch Set 5:
(14 comments)
Just few hopefully helpful review comments.
Thanks for the comments. I've added some responses below.
13 comments:
Patch Set #5, Line 96:
size_t page_size = getpagesize();
+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.
Patch Set #5, Line 457:
#endif // ARCH_CPU_X86_FAMILY
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.
Patch Set #5, Line 548:
GetVectorRegisters64(tid, &info->vector_context, can_log_) &&
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:
Patch Set #5, Line 90:
#endif // ARCH_CPU_X86_FAMILYDitto. Outdated comment.
See above response.
Patch Set #5, Line 158:
#endif // ARCH_CPU_X86_FAMILYditto
See above response.
Patch Set #5, Line 171:
#endif // ARCH_CPU_X86_FAMILY || ARCH_CPU_ARM64ditto
See above response.
Patch Set #5, Line 245:
// PPC64 is 64-bit
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.
Patch Set #5, Line 248:
#endif // ARCH_CPU_X86_FAMILYditto
See above response.
Patch Set #5, Line 286:
#endif // ARCH_CPU_X86_FAMILYditto
See above response.
Patch Set #5, Line 316:
#endif // ARCH_CPU_X86ditto
See above response.
File util/posix/signals_test.cc:
Patch Set #5, Line 48:
#if !defined(ARCH_CPU_ARM64) && !defined(ARCH_CPU_PPC64)
#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.
Patch Set #5, Line 123:
// PPC64 fixed-point division by zero also doesn't produce a SIGFPE.
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.