----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/67614/#review204876 -----------------------------------------------------------
Ship it! Thanks for this cleanup James! Do you know whether we trigger just implementation-defined or really undefined behavior here? In the later case we might want to backport the change as this has been part of all releases since 0.28.0. The change that broke `memcpy`'s preconditions was `4a01850c55` which changed `Tree::Memory::set` from `bool` to a `std::atomic_bool`. Let's call out that commit in the commit message here. I left notes unrelated to your fix below; feel free to ignore them. 3rdparty/stout/include/stout/os/posix/fork.hpp Line 338 (original), 338 (patched) <https://reviews.apache.org/r/67614/#comment287667> This function looks like it does not modify `tree`, but in fact does modify `tree.process` through a pointer. I feel something like taking a `Tree* tree` would be more honest. 3rdparty/stout/include/stout/os/posix/fork.hpp Line 350 (original), 350 (patched) <https://reviews.apache.org/r/67614/#comment287664> There seems to be some non-async signal safe stuff in this `fork`/`exec*` bracket (e.g., `set.store`, construction of `std::set<pid_t> pids`, `pids.insert`). We should probably create a ticket for that. - Benjamin Bannier On June 15, 2018, 8:36 p.m., James Peach wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/67614/ > ----------------------------------------------------------- > > (Updated June 15, 2018, 8:36 p.m.) > > > Review request for mesos, Benjamin Bannier and Benjamin Hindman. > > > Bugs: MESOS-9002 > https://issues.apache.org/jira/browse/MESOS-9002 > > > Repository: mesos > > > Description > ------- > > GCC 8.1 warns about using `memcpy` to copy a `os::Fork::Tree::Memory` > struct because it doesn't have a trivial copy operator. We can replace > the `memcpy` with direct access to the structure fields, which has the > same effect. > > > Diffs > ----- > > 3rdparty/stout/include/stout/os/posix/fork.hpp > 098224ee4abe73b61f180af3dac5989141e5908a > > > Diff: https://reviews.apache.org/r/67614/diff/1/ > > > Testing > ------- > > make check (Fedora 28) > > > Thanks, > > James Peach > >
