zos: add support for z/OS [v8/v8 : main]
Michael Lippautz (Gerrit)
Michael Lippautz added 7 comments![]( "Open in Gerrit")
Thanks, did a first pass over `src/base` mostly which looks okay. `libsampler` can also be reworked to require less zOS specifics.
bool startThread(void* (*threadEntry)(void*)) {nit: CamelCase
const size_t default_stack_size = 4 * 1024 * 1024;constexpr size_t kDefaultStackSize
const pthread_t kNoThread = static_cast<pthread_t>(0);Why is the initializer not working?
// TODO(gabylb): zos - implement when a debugger is supported.Let's use t he tracking bug here.
It's concerning that this is the only platform that doesn't support debug break.
// TODO(gabylb): zos - rusage struct doesn't yet include ru_maxrssSame here: Let's use the tracking bug.
std::unordered_map<pthread_t, SamplerList> sampler_map_;I think if `Sampler::PlatformData` would not store a `pthread_t` that is initialized from `pthread_self()` but rather the result of `OS::GetCurrentThreadId()` (as `int`) then this would not require any `ThreadKey()` magic in the .cc file.
Related details
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 7 comments![]( "Open in Gerrit")
Thanks Michael for your feedback. Please see updates in the patchset 2.
bool startThread(void* (*threadEntry)(void*)) {Gaby Baghdadinit: CamelCase
Done
const size_t default_stack_size = 4 * 1024 * 1024;Gaby Baghdadiconstexpr size_t kDefaultStackSize
Done
const pthread_t kNoThread = static_cast<pthread_t>(0);Why is the initializer not working?
pthread_t on z/OS is a struct (one unsigned long long member). I've added a comment in Patchset 2.
// TODO(gabylb): zos - implement when a debugger is supported.Let's use t he tracking bug here.
It's concerning that this is the only platform that doesn't support debug break.
Done
// TODO(gabylb): zos - rusage struct doesn't yet include ru_maxrssSame here: Let's use the tracking bug.
Done
std::unordered_map<pthread_t, SamplerList> sampler_map_;I think if `Sampler::PlatformData` would not store a `pthread_t` that is initialized from `pthread_self()` but rather the result of `OS::GetCurrentThreadId()` (as `int`) then this would not require any `ThreadKey()` magic in the .cc file.
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 5 comments![]( "Open in Gerrit")
#ifndef __MVS__`V8_OS_ZOS`
What is is the include for?
LESavStackAsync::getInstance().save(new_sp); // from zoslibWhat is this needed for? As you can see, this part of the code is generally platform agnostic.
// The binary mode results in an unreadable mixed mode embedded.S.Why? What would need to be fixed?
void DeclareLabelProlog(FILE* fp, const char* name);How about adding an empty virtual `DeclareLabelProlog()` and epilogue to the file write baser and only overriding that for zos?
Related details
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
- Gaby Baghdadi
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 11 comments![]( "Open in Gerrit")
Hi Gabby,
This CL is also failing a lot of tests on the simulator which need to be resolved.
Milad, can you review the *s390* changes?
Thanks for your help Michael.
// r2, r9: number of arguments including receiver (C callee-saved)nit, extra space in comment.
#define ABI_USES_FUNCTION_DESCRIPTORS 0curious on why this block had to be moved to constants-s390.h?
LoadMultipleP(r5, r6, MemOperand(ip, 0));MemOperand(ip), `0` can be removed.
LoadMultipleP(r5, r6, MemOperand(function, 0));`0` can be removed.
LoadU64(r5, MemOperand(scratch, 0));`0` can be removed.
// they are passed in registers. To ensure stack slots are available tonit, comment has extra spaces, i.e ` To ensure...`.
__ StoreMultipleP(r4, sp, MemOperand(r4, 0));`0` can be removed.
__ LoadMultipleP(r4, sp, MemOperand(sp, 0));`0` can be removed.
__ LoadMultipleP(r5, r6, MemOperand(ip, 0));`0` can be removed.
Related details
- Gaby Baghdadi
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 16 comments![]( "Open in Gerrit")
Hi Gabby,
This CL is also failing a lot of tests on the simulator which need to be resolved.
I
Thanks Michael and Milad for your feedback. I had to revert the libsampler change (to use int vm_tid_), as the pthread_kill() in Sampler::DoSample() requires pthread_self() rather then GetCurrentThreadId(), and that regressed many tests in at least linux-s390x. Hopefully the original patch that uses new ThreadKey() will be accepted.
// r2, r9: number of arguments including receiver (C callee-saved)nit, extra space in comment.
Acknowledged
#define ABI_USES_FUNCTION_DESCRIPTORS 0curious on why this block had to be moved to constants-s390.h?
I have a comment about that in the CL's description (under 5a):
- Move define of ABI_USES_FUNCTION_DESCRIPTORS (1 for z/OS, 0 LoZ) and
related macros from assembler-s390.h to constants-s390.h, so that
for z/OS they're defined when checked, as in simulator.h
(which includes simulator-s390.h that includes constants-s390.h).
MemOperand(ip), `0` can be removed.
Acknowledged
LoadMultipleP(r5, r6, MemOperand(function, 0));Gaby Baghdadi`0` can be removed.
Acknowledged
LoadU64(r5, MemOperand(scratch, 0));Gaby Baghdadi`0` can be removed.
Acknowledged
// they are passed in registers. To ensure stack slots are available tonit, comment has extra spaces, i.e ` To ensure...`.
`V8_OS_ZOS`
What is is the include for?
netinet/ip.h doesn't exist on z/OS, hence the added #ifndef.
LESavStackAsync::getInstance().save(new_sp); // from zoslibWhat is this needed for? As you can see, this part of the code is generally platform agnostic.
Please see https://github.com/ibmruntimes/zoslib/blob/main/include/zos-savstack.h#L21-L37. I'll remove it, and can revisit it if/when needed.
__ StoreMultipleP(r4, sp, MemOperand(r4, 0));Gaby Baghdadi`0` can be removed.
Acknowledged
__ LoadMultipleP(r4, sp, MemOperand(sp, 0));Gaby Baghdadi`0` can be removed.
Acknowledged
__ LoadMultipleP(r5, r6, MemOperand(ip, 0));Gaby Baghdadi`0` can be removed.
Acknowledged
// The binary mode results in an unreadable mixed mode embedded.S.Why? What would need to be fixed?
The "b" mode creates the file (embedded.s) as 'mixed' type, as opposed to binary or text, and the clang compiler fails to read it. Does the file created here need to be binary?
How about adding an empty virtual `DeclareLabelProlog()` and epilogue to the file write baser and only overriding that for zos?
Acknowledged
#ifdef __MVS__Gaby BaghdadiHere and below: `V8_OS_ZOS`
Acknowledged
Related details
- Michael Lippautz
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
#ifndef __MVS__Gaby Baghdadi`V8_OS_ZOS`
What is is the include for?
netinet/ip.h doesn't exist on z/OS, hence the added #ifndef.
In that case it should be `ifndef V8_OS_ZOS`?
`__MVS__` is used for toggling `ZOS`. We should do that centrally and then use the more declarative name.
LESavStackAsync::getInstance().save(new_sp); // from zoslibGaby BaghdadiWhat is this needed for? As you can see, this part of the code is generally platform agnostic.
Please see https://github.com/ibmruntimes/zoslib/blob/main/include/zos-savstack.h#L21-L37. I'll remove it, and can revisit it if/when needed.
I am not familiar with ZOS. The comment only describes what it does not not why. We should be able to provide proper reasoning why this is needed if we add it.
// The binary mode results in an unreadable mixed mode embedded.S.Gaby BaghdadiWhy? What would need to be fixed?
The "b" mode creates the file (embedded.s) as 'mixed' type, as opposed to binary or text, and the clang compiler fails to read it. Does the file created here need to be binary?
Adding @les...@chromium.org as an OWNER to help judge this. I am not familiar with the code.
Related details
- Gaby Baghdadi
- Leszek Swirski
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
return static_cast<int>(thread_id.__ & 0x7fffffff);This will be hard to maintain which is why we have our base abstraction.
Why do we need to mask away the top bits? Are they non-deterministic, i.e., will subsequent calls to pthread_self() yield in different MSBs?
Related details
- Gaby Baghdadi
- Leszek Swirski
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 3 comments![]( "Open in Gerrit")
#ifndef __MVS__Gaby Baghdadi`V8_OS_ZOS`
What is is the include for?
Michael Lippautznetinet/ip.h doesn't exist on z/OS, hence the added #ifndef.
In that case it should be `ifndef V8_OS_ZOS`?
`__MVS__` is used for toggling `ZOS`. We should do that centrally and then use the more declarative name.
Ok, I moved the include of d8.h to top of the include list (in absence of d8-posix.h) to use V8_OS_ZOS instead. Thanks.
return static_cast<int>(thread_id.__ & 0x7fffffff);This will be hard to maintain which is why we have our base abstraction.
Why do we need to mask away the top bits? Are they non-deterministic, i.e., will subsequent calls to pthread_self() yield in different MSBs?
I removed ThreadKey(), and changed `pthread_t vm_tid()` to return int from `base::OS::GetCurrentThreadId()` as you suggested earlier, and also added a new `pthread_t vm_tself()` to use in the pthread_kill command.
For a given thread, those masked bits will always be the same. I'll call the `int gettid()` (from zoslib) instead. pthread_t.__ is `unsigned long long` for alignment.
Related details
- Leszek Swirski
- Michael Lippautz
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
#define ABI_USES_FUNCTION_DESCRIPTORS 0Gaby Baghdadicurious on why this block had to be moved to constants-s390.h?
I have a comment about that in the CL's description (under 5a):
- Move define of ABI_USES_FUNCTION_DESCRIPTORS (1 for z/OS, 0 LoZ) and
related macros from assembler-s390.h to constants-s390.h, so that
for z/OS they're defined when checked, as in simulator.h
(which includes simulator-s390.h that includes constants-s390.h).
- Gaby Baghdadi
- Leszek Swirski
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
// TODO(gabylb): zos - implement when a debugger is supported.Gaby BaghdadiLet's use t he tracking bug here.
It's concerning that this is the only platform that doesn't support debug break.
Done
Patchset 11 implements this for z/OS (with `asm(" dc x'0001'");`).
Related details
- Leszek Swirski
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 4 comments![]( "Open in Gerrit")
Hello. I've resolved the merge conflict and addressed the 3 remaining comments. Please review.
#ifndef __MVS__Gaby Baghdadi`V8_OS_ZOS`
What is is the include for?
Michael Lippautznetinet/ip.h doesn't exist on z/OS, hence the added #ifndef.
Gaby BaghdadiIn that case it should be `ifndef V8_OS_ZOS`?
`__MVS__` is used for toggling `ZOS`. We should do that centrally and then use the more declarative name.
Ok, I moved the include of d8.h to top of the include list (in absence of d8-posix.h) to use V8_OS_ZOS instead. Thanks.
Done
return static_cast<int>(thread_id.__ & 0x7fffffff);Gaby BaghdadiThis will be hard to maintain which is why we have our base abstraction.
Why do we need to mask away the top bits? Are they non-deterministic, i.e., will subsequent calls to pthread_self() yield in different MSBs?
I removed ThreadKey(), and changed `pthread_t vm_tid()` to return int from `base::OS::GetCurrentThreadId()` as you suggested earlier, and also added a new `pthread_t vm_tself()` to use in the pthread_kill command.
For a given thread, those masked bits will always be the same. I'll call the `int gettid()` (from zoslib) instead. pthread_t.__ is `unsigned long long` for alignment.
Done
// The binary mode results in an unreadable mixed mode embedded.S.Gaby BaghdadiWhy? What would need to be fixed?
Michael LippautzThe "b" mode creates the file (embedded.s) as 'mixed' type, as opposed to binary or text, and the clang compiler fails to read it. Does the file created here need to be binary?
Adding @les...@chromium.org as an OWNER to help judge this. I am not familiar with the code.
I've now reverted this change, so it doesn't block this CL. After it's approved and merged, I'll file an issue to propose replacing the "wb" with "w".
Related details
- Leszek Swirski
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski added 1 comment![]( "Open in Gerrit")
// The binary mode results in an unreadable mixed mode embedded.S.Gaby BaghdadiWhy? What would need to be fixed?
Michael LippautzThe "b" mode creates the file (embedded.s) as 'mixed' type, as opposed to binary or text, and the clang compiler fails to read it. Does the file created here need to be binary?
Adding @les...@chromium.org as an OWNER to help judge this. I am not familiar with the code.
(forgot to send this Draft)
`b` is ignored on POSIX (https://linux.die.net/man/3/fopen#:~:text=The%20mode%20string,non%2DUNIX%20environments), but I think we might want non-binary on all platforms since `.S` files are text files with assembly, not machine code. I did some code archaeology here and "wb" here goes back to the initial export (https://chromium.googlesource.com/v8/v8/+/43d26ecc3563a46f62a0224030667c8f8f3f6ceb/src/mksnapshot.cc#125) so there's no known issue with using "w" instead.
Related details
- Gaby Baghdadi
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
// The binary mode results in an unreadable mixed mode embedded.S.Gaby BaghdadiWhy? What would need to be fixed?
Michael LippautzThe "b" mode creates the file (embedded.s) as 'mixed' type, as opposed to binary or text, and the clang compiler fails to read it. Does the file created here need to be binary?
Leszek SwirskiAdding @les...@chromium.org as an OWNER to help judge this. I am not familiar with the code.
(forgot to send this Draft)
`b` is ignored on POSIX (https://linux.die.net/man/3/fopen#:~:text=The%20mode%20string,non%2DUNIX%20environments), but I think we might want non-binary on all platforms since `.S` files are text files with assembly, not machine code. I did some code archaeology here and "wb" here goes back to the initial export (https://chromium.googlesource.com/v8/v8/+/43d26ecc3563a46f62a0224030667c8f8f3f6ceb/src/mksnapshot.cc#125) so there's no known issue with using "w" instead.
Thanks Leszek. I've now removed the `b` for all platforms in Patchset 16.
Related details
- Leszek Swirski
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 2 comments![]( "Open in Gerrit")
// The binary mode results in an unreadable mixed mode embedded.S.Gaby BaghdadiWhy? What would need to be fixed?
Michael LippautzThe "b" mode creates the file (embedded.s) as 'mixed' type, as opposed to binary or text, and the clang compiler fails to read it. Does the file created here need to be binary?
Leszek SwirskiAdding @les...@chromium.org as an OWNER to help judge this. I am not familiar with the code.
Gaby Baghdadi(forgot to send this Draft)
`b` is ignored on POSIX (https://linux.die.net/man/3/fopen#:~:text=The%20mode%20string,non%2DUNIX%20environments), but I think we might want non-binary on all platforms since `.S` files are text files with assembly, not machine code. I did some code archaeology here and "wb" here goes back to the initial export (https://chromium.googlesource.com/v8/v8/+/43d26ecc3563a46f62a0224030667c8f8f3f6ceb/src/mksnapshot.cc#125) so there's no known issue with using "w" instead.
Thanks Leszek. I've now removed the `b` for all platforms in Patchset 16.
Done
Related details
- Leszek Swirski
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
Hello Michael. Are other reviewers required for this CL? I tried adding, but found that your name was included as owner in every updated file in this CL. Please let me what's needed in order to move this CL forward.
Related details
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz added 3 comments![]( "Open in Gerrit")
The CL overall lgtm.
I have two remaining questions on the snapshot stuff. Leszek there is the better owner though, so if he is fine, we can move ahead and land this.
WriteDirectiveOrSeparator(w, 0, kByte);Shouldn't this prevent us from putting `WriteDirectiveOrSeparator()` behind !zos?
void EmbeddedFileWriter::WriteBinaryContentsAsInlineAssembly(I wonder if we should make this a virtual somehwere to allow specialization?
Related details
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 2 comments![]( "Open in Gerrit")
Shouldn't this prevent us from putting `WriteDirectiveOrSeparator()` behind !zos?
This call is not used on z/OS (it's guarded above by `#if V8_TARGET_ARCH_IA32 || V8_TARGET_ARCH_X64`). However, I've now removed the guard for WriteDirectiveOrSeparator() and WriteLineEndIfNeeded().
void EmbeddedFileWriter::WriteBinaryContentsAsInlineAssembly(I wonder if we should make this a virtual somehwere to allow specialization?
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand voted Code-Review+1![]( "Open in Gerrit")
- Gaby Baghdadi
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
Hi Leszek, would you please take a look at this CL once more when you have a chance, thank you.
Related details
- Gaby Baghdadi
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Leszek Swirski (Gerrit)
Leszek Swirski voted and added 1 comment![]( "Open in Gerrit")
Votes added by Leszek Swirski
| Code-Review | +1 |
1 comment
void EmbeddedFileWriter::WriteBinaryContentsAsInlineAssembly(Gaby BaghdadiI wonder if we should make this a virtual somehwere to allow specialization?
@les...@chromium.org please advise.
I think this is ok as-is, since it's a bit of a one-off. A cleaner version would move the whole implementation into `PlatformEmbeddedFileWriter`, since that's where the platform specific stuff lives, but I won't block on it.
Related details
- Gaby Baghdadi
- Michael Lippautz
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
1 comment
lgtm overall
Related details
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
void EmbeddedFileWriter::WriteBinaryContentsAsInlineAssembly(Gaby BaghdadiI wonder if we should make this a virtual somehwere to allow specialization?
Leszek Swirski@les...@chromium.org please advise.
I think this is ok as-is, since it's a bit of a one-off. A cleaner version would move the whole implementation into `PlatformEmbeddedFileWriter`, since that's where the platform specific stuff lives, but I won't block on it.
Thanks Leszek. With Michael's code review approval, I'll go ahead and resolve this item.
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand voted Commit-Queue+2![]( "Open in Gerrit")
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
Re failure with Tryjob v8/try/v8_linux_noi18n_compile_dbg, changing the format to the suggested `%.16llx` results in a warning when compiled with clang on z/OS, as well as clang on x86:
```
clang version 13.0.0 (https://github.com/llvm/llvm-project/ 24c8eaec9467b2aaf70b0db33a4e4dd415139a50)
Target: x86_64-unknown-linux-gnu
Thread model: posix
../../src/snapshot/embedded/platform-embedded-file-writer-zos.cc:128:34: warning: format specifies type 'unsigned long long' but the argument has type 'uint64_t' (aka 'unsigned long') [-Wformat]
return fprintf(fp_, "%.16llx", value);
~~~~~~~ ^~~~~
%.16lx
```
fyi @mlip...@chromium.org
Related details
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
Re failure with Tryjob v8/try/v8_linux_noi18n_compile_dbg, changing the format to the suggested `%.16llx` results in a warning when compiled with clang on z/OS, as well as clang on x86:
```
clang version 13.0.0 (https://github.com/llvm/llvm-project/ 24c8eaec9467b2aaf70b0db33a4e4dd415139a50)
Target: x86_64-unknown-linux-gnu
Thread model: posix../../src/snapshot/embedded/platform-embedded-file-writer-zos.cc:128:34: warning: format specifies type 'unsigned long long' but the argument has type 'uint64_t' (aka 'unsigned long') [-Wformat]
return fprintf(fp_, "%.16llx", value);
~~~~~~~ ^~~~~
%.16lx
```
fyi @mlip...@chromium.org
Issue resolved in patch set 19. Please review.
Related details
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
Re the 2 try jobs that failed:
- v8_linux64_bazel failed with `'src/snapshot/embedded/platform-embedded-file-writer-zos.h' file not found` - even though headers in the `#include`s before it are from the same directory, and all other platforms built successfully.
- node_ci_linux64_rel `wasi/test-wasi-not-started` seems to be a known failure, as reported in https://issues.chromium.org/issues/349860793.
@mlip...@chromium.org can you please help resolve.
Related details
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 2 comments![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
2 comments
Re the 2 try jobs that failed:
- v8_linux64_bazel failed with `'src/snapshot/embedded/platform-embedded-file-writer-zos.h' file not found` - even though headers in the `#include`s before it are from the same directory, and all other platforms built successfully.
- node_ci_linux64_rel `wasi/test-wasi-not-started` seems to be a known failure, as reported in https://issues.chromium.org/issues/349860793.
@mlip...@chromium.org can you please help resolve.
Re the 2 try jobs that failed:
- v8_linux64_bazel failed with `'src/snapshot/embedded/platform-embedded-file-writer-zos.h' file not found` - even though headers in the `#include`s before it are from the same directory, and all other platforms built successfully.
Can you add the file to `BUILD.bazel` similar to how the `aix` file is in there? I don't think this would build but it's enough to get through the build system.
- node_ci_linux64_rel `wasi/test-wasi-not-started` seems to be a known failure, as reported in https://issues.chromium.org/issues/349860793.
This one will likely resolve itself as it's unrelated.
Changes lgtm but there's also a bazel buildfile that requires adjusting. Not sure
Related details
- Gaby Baghdadi
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
Changes lgtm but there's also a bazel buildfile that requires adjusting. Not sure
- Leszek Swirski
- Michael Lippautz
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Lippautz (Gerrit)
Michael Lippautz voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Lippautz
| Code-Review | +1 |
- Gaby Baghdadi
- Leszek Swirski
- Milad Farazmand
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Gaby Baghdadi
- Leszek Swirski
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Gaby Baghdadi
- Leszek Swirski
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
zos: add support for z/OS
The following changes are required to build V8 on z/OS s390x platform.
These changes are added on top of the currently supported s390
port of V8.
All changes are guarded by V8_OS_ZOS or __MVS__, except in sampler.cc,
embedded-file-writer.h, and the moving of macros from assembler-s390.h
to constants-s390.h, as described below.
Changes include:
1) Download dependency and build:
a) DEPS
- Download dependency https://github.com/ibmruntimes/zoslib into
third_party/zoslib.
- Skip fetching certain packages (ninja, luci-go, etc.) as there's no
z/OS binary for them upstream.
b) BUILD.gn
- Build zoslib, exclude NA source and compiler options, and add
z/OS-specific source files to the build.
2) Define macros, constants and includes:
a) include/v8config.h
- Include zos-base.h and define V8_OS_ZOS, V8_OS_STRING, and
V8_OS_POSIX; zos-base.h is from third_party/zoslib/.
b) src/d8/d8-posix.cc
- Exclude NA header.
3) Platform:
a) src/base/platform/platform-posix.cc
- Implement GetCurrentThreadId().
- Set stack size before starting thread.
- Change for pthread_t being a struct, not int.
- Exclude NA static functions and headers.
b) src/base/platform/platform-zos.cc
- New file, implements PosixMemoryMappedFile class, OS::Allocate(),
OS::Free(), etc.
c) src/heap/base/asm/zos/push_registers_asm.cc
- New file, implements PushAllRegistersAndIterateStack() for z/OS.
d) src/base/debug/stack_trace_zos.cc:
- New file, implements StackTrace class methods and stack dumping
signals thread and handler for z/OS.
e) src/base/platform/time.cc
- Use clock_gettime() in ClockNow(), and ClockNow() in
ThreadTicks::Now().
- Return true in ThreadTicks::IsSupported().
f) src/base/sys-info.cc
- Implement NumberOfProcessors() and AmountOfPhysicalMemory() using
API from zoslib.
4) Snapshot:
a) src/snapshot/embedded/embedded-file-writer.cc
src/snapshot/embedded/embedded-file-writer.h
- Update WriteBinaryContentsAsInlineAssembly() so a line doesn't exceed
the max length of 72, a z/OS requirement for HLASM, and define
DeclareLabelProlog() and DeclareLabelEpilogue() for the embedded blob
code and data symbols.
b) src/snapshot/embedded/platform-embedded-file-writer-zos.cc
src/snapshot/embedded/platform-embedded-file-writer-zos.h
- New files, implements embedded file writer for z/OS.
c) src/snapshot/embedded/platform-embedded-file-writer-base.h
src/snapshot/embedded/platform-embedded-file-writer-base.cc
- Include src/snapshot/embedded/platform-embedded-file-writer-zos.h,
define EmbeddedTargetOs::kZOS and update
NewPlatformEmbeddedFileWriter() if embedded target is z/OS.
d) src/snapshot/embedded/embedded-file-writer.h
- Replace "wb" by "w" in open of embedded.S
Ref: https://chromium-review.googlesource.com/c/v8/v8/+/5558534/comment/ab5f65b7_ede704b3/
5) Codegen:
a) src/codegen/s390/constants-s390.h
src/codegen/s390/macro-assembler-s390.cc
src/codegen/s390/macro-assembler-s390.h
src/codegen/s390/register-s390.h
src/codegen/s390/assembler-s390.cc
src/codegen/s390/assembler-s390.h
src/compiler/backend/s390/code-generator-s390.cc
- Call JS code using a function descriptor as required on z/OS.
- Support XPLINK ABI in builtins for z/OS; shuffle XPLINK arguments
coming from C to JS arguments. Add support for calling z/OS XPLINK
C/C++ from JS codegen.
- Enable support for native regexp codegen for z/OS. Arguments are
shuffled from z/OS XPLINK ABI to LoZ ABI on function entry and vice
versa on return.
- Implement supportsCPUFeature(), supportsSTFLE() and ProbeImpl().
- Handle nop pseudo instructions for basr, bras, brasl call types.
- Move define of ABI_USES_FUNCTION_DESCRIPTORS (1 for z/OS, 0 LoZ) and
related macros from assembler-s390.h to constants-s390.h, so that
for z/OS they're defined when checked, as in simulator.h
(which includes simulator-s390.h that includes constants-s390.h).
b) src/diagnostics/objects-debug.cc
- Don't check for instruction start alignment, as no code alignment is
required on z/OS. The start of each function and labels are aligned
on a half-word binary.
6) Execution:
a) src/execution/execution.cc
- Save LE stack pointer to restore on signal while in JS.
b) src/execution/isolate.cc
- Allow more overflow stack space as z/OS XPLINK uses larger frames.
- Ensure thread has enough stack guard.
c) src/execution/simulator.h
- Create a pseudo function descriptor to ensure correct dispatch to
generated code.
7) Regexp:
a) src/regexp/s390/regexp-macro-assembler-s390.cc
- Enable regexp support:
- Add support for native regexp codegen.
- Shuffle arguments from z/OS XPLINK ABI to LoZ ABI on function
entry and vice versa on return.
8) Builtins:
a) src/builtins/s390/builtins-s390.cc
- Add support for XPLINK ABI in builtins:
- Add support for shuffling XPLINK arguments coming from C to JS.
- Add support for calling z/OS XPLINK C/C++ from JS codegen.
9) Adjust for pthread_t being a struct (not int):
a) src/base/platform/platform.h
src/libsampler/sampler.h
10) libsampler:
a) src/libsampler/sampler.cc
- Implement FillRegisterState() to set the state's registers (pc, sp,
fp, lr) with the given context in the signal handler.
- Adjust for pthread_t being a struct (not int);
new vm_tself() returns pthread_self() for use in pthread_kill(); also
change vm_tid() to return int using base::OS::GetCurrentThreadId().
11) tools:
a) tools/testrunner/local/statusfile.py
- Add "zos" to VARIABLES.
b) tools/testrunner/local/utils.py
- Return "zos" in GuessOS() if platform.system() is "OS/390"
- Uupdate IsS390SimdSupported() to return true on z13 or higher.
12) test:
a) test/cctest/cctest-utils.h
test/unittests/test-utils.h
- Define GET_STACK_POINTER_TO(sp_addr) macro for z/OS.
Co-Authored-By: CW Cheung <ccw.2...@ca.ibm.com>
Co-Authored-By: Igor Todorovski <itod...@ca.ibm.com>
Co-Authored-By: Wayne Zhang <Shuowan...@ibm.com>
- M BUILD.bazel
- M BUILD.gn
- M DEPS
- M include/v8config.h
- A src/base/debug/stack_trace_zos.cc
- M src/base/immediate-crash.h
- M src/base/platform/memory.h
- M src/base/platform/platform-posix.cc
- A src/base/platform/platform-zos.cc
- M src/base/platform/platform.h
- M src/base/platform/semaphore.h
- M src/base/platform/time.cc
- M src/base/sys-info.cc
- M src/builtins/s390/builtins-s390.cc
- M src/codegen/s390/assembler-s390.cc
- M src/codegen/s390/assembler-s390.h
- M src/codegen/s390/constants-s390.h
- M src/codegen/s390/macro-assembler-s390.cc
- M src/codegen/s390/macro-assembler-s390.h
- M src/codegen/s390/register-s390.h
- M src/compiler/backend/s390/code-generator-s390.cc
- M src/d8/d8-posix.cc
- M src/diagnostics/objects-debug.cc
- M src/execution/simulator.h
- A src/heap/base/asm/zos/push_registers_asm.cc
- M src/libsampler/sampler.cc
- M src/libsampler/sampler.h
- M src/regexp/s390/regexp-macro-assembler-s390.cc
- M src/snapshot/embedded/embedded-file-writer.cc
- M src/snapshot/embedded/embedded-file-writer.h
- M src/snapshot/embedded/platform-embedded-file-writer-base.cc
- M src/snapshot/embedded/platform-embedded-file-writer-base.h
- A src/snapshot/embedded/platform-embedded-file-writer-zos.cc
- A src/snapshot/embedded/platform-embedded-file-writer-zos.h
- M test/cctest/BUILD.gn
- M test/cctest/cctest-utils.h
- M test/cctest/cctest.status
- M test/inspector/inspector.status
- M test/mjsunit/mjsunit.status
- M test/unittests/test-utils.h
- M tools/testrunner/local/statusfile.py
- M tools/testrunner/local/utils.py
Code-Review: +1 by Michael Lippautz, +1 by Milad Farazmand
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Michael Achenbach added 1 comment![]( "Open in Gerrit")
'url': Var('chromium_url') + '/external/github.com/ibmruntimes/zoslib.git' + '@' + '3accd08b136344128beb3f594e8ab3eb30accc56',This does not exist yet, does it? This currently breaks our auto roller which verifies all entries.
Related details
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Michael Achenbach has created a revert of this change![]( "Open in Gerrit")
Related details
- Gaby Baghdadi
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 2 comments![]( "Open in Gerrit")
'url': Var('chromium_url') + '/external/github.com/ibmruntimes/zoslib.git' + '@' + '3accd08b136344128beb3f594e8ab3eb30accc56',This does not exist yet, does it? This currently breaks our auto roller which verifies all entries.
The github.com/ibmruntimes/zoslib repo exists for the specified commit: https://github.com/ibmruntimes/zoslib/commit/3accd08b136344128beb3f594e8ab3eb30accc56
Is the format here incorrect?
'url': Var('chromium_url') + '/external/github.com/ibmruntimes/zoslib.git' + '@' + '3accd08b136344128beb3f594e8ab3eb30accc56',This does not exist yet, does it? This currently breaks our auto roller which verifies all entries.
It seems it got squashed out during the PR merge. The correct commit is https://github.com/ibmruntimes/zoslib/commit/1e68de6e37efced3738a88536fccb6bbfe2d70b2.
Should I upload a fix here (in a merged CL)? Or open another CL to correct the commit in DEPS?
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
'url': Var('chromium_url') + '/external/github.com/ibmruntimes/zoslib.git' + '@' + '3accd08b136344128beb3f594e8ab3eb30accc56',Gaby BaghdadiThis does not exist yet, does it? This currently breaks our auto roller which verifies all entries.
The github.com/ibmruntimes/zoslib repo exists for the specified commit: https://github.com/ibmruntimes/zoslib/commit/3accd08b136344128beb3f594e8ab3eb30accc56
Is the format here incorrect?
Please ignore the above comment.
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaby Baghdadi (Gerrit)
Gaby Baghdadi added 1 comment![]( "Open in Gerrit")
'url': Var('chromium_url') + '/external/github.com/ibmruntimes/zoslib.git' + '@' + '3accd08b136344128beb3f594e8ab3eb30accc56',Gaby BaghdadiThis does not exist yet, does it? This currently breaks our auto roller which verifies all entries.
Gaby BaghdadiThe github.com/ibmruntimes/zoslib repo exists for the specified commit: https://github.com/ibmruntimes/zoslib/commit/3accd08b136344128beb3f594e8ab3eb30accc56
Is the format here incorrect?
Please ignore the above comment.
Hi @mlip...@chromium.org, I've uploaded https://chromium-review.googlesource.com/c/v8/v8/+/5668493 that updates the commit hash to a valid one.
Related details
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |