Re: [lmi] Unit tests hygiene

[Vadim Zeitlin]

On Mon, 13 Jun 2022 20:33:53 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:

GC> On 6/9/22 23:22, Vadim Zeitlin wrote:
GC> > On Thu, 9 Jun 2022 21:37:00 +0000 Greg Chicares <gchicares@sbcglobal.net> 
wrote:
GC> > 
GC> > GC> Here's an example that I think is UB, though UBSan doesn't flag it:
GC> > GC> 
GC> > GC>     // UBSAN doesn't complain about this; shouldn't it?
GC> > GC>     auto const b {db.query<oenum_alb_or_anb>(DB_ChildRiderMinAmt)};
GC> > GC>     LMI_TEST_EQUAL(25000, b);
GC> > GC> 
GC> > GC> Am I missing something, or is this truly a false negative?
GC> > 
GC> >  Sorry, I know I'm tired today (because I've already made a couple of bad
GC> > mistakes in the last hour), but what exactly is the problem here, i.e. why
GC> > should it be UB? AFAIK comparing an enum with an int is not UB, due to the
GC> > promotion rules. Am I missing some other reason here?
GC> 
GC> Consider the throwaway patch below, which I believe demonstrates that
GC> this isn't mere comparison of an int to an enum, but actually entails
GC> the assignment of an out-of-enumerated-range 'int' value into an enum.

 Oh, sorry, I was tired because I had somehow focused only on the
comparison without even realizing that if the test passed, it had to be
true, meaning that the enum had an invalid value.

GC> In more detail, I manually "inlined" the implementation of
GC>   template<typename T> T product_database::query(e_database_key k) const
GC> from 'database.hpp' as 'c' below. I believe that
GC>   - the assignment into 'c' is just like the assignment into 'a'; and
GC>   - the assignment into 'c' is just like the assignment into 'b';
GC> and so by transitivity all three should be treated the same by UBSan.
GC> But UBSan flags only 'a', not 'b' or 'c', whereas I believe that all
GC> three constitute UB.

 You're correct and gcc UBSAN implementation is wrong. FWIW clang UBSAN
agrees with you and outputs

input_test.cpp:177:5: runtime error: load of value 25000, which is not a valid 
value for type 'oenum_alb_or_anb const'

with the unmodified test and 2 other identical warnings on lines 187 and
192 with the patch applied.

 Unfortunately even gcc 12 doesn't catch any of these problems and so there
is not much I can recommend here. I guess the main conclusion is that I'd
better use sanitizers in the clang CI build rather than the gcc one, as
they seem to work much better with their "native" compiler.

 Sorry for completely missing the point the first time,
VZ

pgpabNlDXbDfW.pgp
Description: PGP signature