(sorry for top posting)
Dear Jan and Steve,
Problems solved.
After a few trials and errors, I figured there are TWO similar
constructs that, depending on the order of parameter evaluation, may
not pass what is desired due to unexpected modification during
parameter evaluation.
I changed my local patches as follows and now the errors during test
run on the tryserver (using clang) match the ones I see locally (using
GCC).
Now I can home in the few remaining errors possibly introduced my
patches. (They may be timing-dependent issues uncovered by my patch,
but who knows.)
Thank you again!
Chiaki
PS: I have found that gcc-8 has a more feature-rich format string
checking: it warns if a complete output would be longer than the
buffer's length passed to snprintf(). Amazing. However, since I turn
some warnings including this one into compilation errors (to avoid the
unexpected failure on the tryserver while I can build successfully), I had
to tweak the webrtc code in a few places.
Modified parts:
comm/mailnews/local/src/nsPop3Sink.cpp
//
// clang and GCC has a different order of argument evaluation.
// We have to save m_outFileSream because
// geter_AddRefs(m_outFileStream) may trash it depending on the
// order of evaluation of passed arguments!
//
nsCOMPtr<nsIOutputStream> tmp_p = m_outFileStream;
rv1 = NS_NewBufferedOutputStream(getter_AddRefs(m_outFileStream),
tmp_p.forget(),
64 * 1024 );
A bit overkill.:
/comm/mailnews/local/src/nsLocalMailFolder.cpp:
nsCOMPtr<nsIOutputStream> tmp_in = mCopyState->m_fileStream;
nsCOMPtr<nsIOutputStream> tmp_out;
nsresult rv2
= NS_NewBufferedOutputStream(getter_AddRefs(tmp_out),
tmp_in.forget(),
64 * 1024 );
mCopyState->m_fileStream = tmp_out;
On 2019/07/31 5:45, ISHIKAWA,chiaki wrote:
Dear Steve,
Thank you for your analysis and suggestion for moving to gcc-8.
I would change my local GCC compiler to gcc-8, and g++-8.
Also, I would modify the offending code so that
the evaluation order of the arguments does not get in the way.
I still yet to do a few more tasks, but I think the mystery of the
difference between the clang version and GCC version, which got me
wondering for the last several days,
has been solved by your and Jan's analysis.
I *think* I was assuming left-to-right argument ordering somehow.
I probably should use clang occasionally to put my implicit
assumptions gleaned from years of GCC usage to torture test, so to speak.
Thank you very much !!!
Chiaki
PS: Well, for whatever the reason, I also found a problem with cflow-8
(maybe my particular setting of C compilation time macros, etc.).
That would be an unexpected bonus. :-)
On 2019/07/31 0:15, Steve Fink wrote:
On 7/30/19 6:20 AM, Jan de Mooij wrote:
On Tue, Jul 30, 2019 at 1:51 PM ISHIKAWA,chiaki <ishik...@yk.rim.or.jp>
wrote:
nsresult rv2
=
NS_NewBufferedOutputStream(getter_AddRefs(mCopyState->m_fileStream),
mCopyState->m_fileStream.forget(), <=== It seems this can be
nullptr in
clang-8 version???
64 * 1024 );
This looks like it could be caused by Clang evaluating your
arguments in a
different order from GCC (see also [0]). If Clang evaluates
left-to-right,
that getter_AddRefs might null out your m_fileStream before we
evaluate the
m_fileStream.forget().
Note that, if I'm reading the spec correctly, it's not just the order
of evaluation between arguments. Before c++17, the evaluation of
different arguments could be interleaved arbitrarily. It looks like
c++17 changes argument evaluation to be "indeterminately sequenced"
instead of the previous "unsequenced". Indeterminately sequenced
means they can happen in any order, but *cannot* be interleaved as
they are now. I don't think that comes up in your specific example,
but it's closely related.
https://en.cppreference.com/w/cpp/language/eval_order
I know the interleaving happens in practice because the GC rooting
hazard analysis ran into this: a Rooted value passed to a function
was extracted into a temporary, another argument's evaluation
triggered a GC that moved the value, and then the now-invalid
temporary was passed into the function. It looks like c++17 will
prevent this from happening.
I guess that gives me motivation to finish updating the rooting
analysis to gcc8, which has c++17 support. Which is relevant to you,
since you're compiling with gcc: the minimum gcc version is going to
be bumped to gcc7 soon, and it looks like we'll only be testing with
gcc8 in the continuous integration system, so it would be safest to
use a minimum of gcc 8. (gcc 7.4.0 has a bug that breaks the rooting
analysis, so it will go straight to gcc 8.)
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
