< wumpus>
I guess, because it shuts down unexpectedly
< wumpus>
the thread doesn't exit in 5 seconds-should be plenty of time, normally, though it's a slow system
< wumpus>
re: #15141 "ValidationInvalidReason::TX_PREMATURE_SPEND is non-standard in AcceptToMemoryPoolWorker (two occurences) but invalid in Consensus::CheckTxInputs" this one is pretty weird isn't it?
< gwillen>
it's not really up to LLVM, the C integer promotion rules are fixed
< gwillen>
it should depend only on the integer types involved
< gwillen>
unfortunately they are extremely arcane
< sipa>
can we do a search replace stacktop(-i) -> stacktop(i) and then use (stack.at(stack.size()-(i)))
< gwillen>
it seems like the offending rule is "If the operand that has unsigned integer type has rank greater than or equal to the rank of the type of the other operand, the operand with signed integer type is converted to the type of the operand with unsigned integer type."
< gwillen>
so if you do math with a size_t (64 bit unsigned) and an int (32-bit signed) you get flattened
< luke-jr>
sipa: if we do that, let's rename stacktop
< gwillen>
it's a weird rule.
< gwillen>
but casting to ssize_t should solve it?
< sipa>
*(stack.rbegin() + i) also works :p
< luke-jr>
+ -i ?
< sipa>
with positive i, i mean
< sipa>
oh, no; *(stack.rbegin() + (i - 1))
< luke-jr>
positive i is a hazard
< sipa>
why?
< luke-jr>
too easy to accidentally merge or backport with an invisible error
< sipa>
that's fair
< gwillen>
this seems like something you could check statically, if it's generally a constant
< luke-jr>
gwillen: it's not always a constant
< luke-jr>
anyway, keeping the negative i should be harmless
< gwillen>
luke-jr: how did it come to pass that this is coming up -- did core change, or did LLVM change?
< luke-jr>
gwillen: -fsanitize=undefined
< luke-jr>
(apparently while we have a Travis job for it, we also have a suppressions file to hide all our existing issues!)
< gwillen>
that's odd... there shouldn't be anything undefined about that, should there? Does it cite chapter and verse?
< gwillen>
I can imagine it getting the wrong answer, but not being undefined
< gwillen>
and it sounds like it's not in fact getting the wrong answer in practice or we'd have hit this before?
< luke-jr>
gwillen: overflows are undefined behaviour
< gwillen>
not unsigned overflows
< gwillen>
only signed
< gwillen>
if it gets converted to unsigned first it should be well-defined
< luke-jr>
UBSan says unsigned are undefined too <.<
< sipa>
luke-jr: by default ubsan does not warn on unsigned overflow, but there is an optional flag to enable them
< sipa>
they're not undefined behavior, but a common red flag
< luke-jr>
gwillen: it scrolled out of my terminal - and I'm not entirely sure how I triggered it, since I wasn't running on mainnet ☺
< luke-jr>
sipa: odd, I didn't explicitly set any flag
< sipa>
the core build system enables it afaik
< luke-jr>
ah
< luke-jr>
is the cast from negative to unsigned also well-defined?
< luke-jr>
script/interpreter.cpp:922:42: runtime error: unsigned integer overflow: 2 + 18446744073709551615 cannot be represented in type 'unsigned long'
< luke-jr>
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior script/interpreter.cpp:922:42 in
< luke-jr>
gwillen: ^
< sipa>
luke-jr: cast from signed to unsigned is well defined
< sipa>
the other way isn't afaik
< * luke-jr>
typically avoids answering these questions by being explicit on anything that could reasonably be undefined :p
< gwillen>
yeah so it seems like we are in one of the corners of integer promotion that is weird but not bad
< gwillen>
wherein C promotes from signed to unsigned in a way that creates apparent integer overflow that actually gives the correct answer consistently
< gwillen>
casting to ssize_t will probably get rid of the weirdness but will cause undefined behavior (I think) if the stack has more than 2 billion elements, which I expect is not a concern
< gwillen>
actually if size_t is 64 bit it would need to be a lot more than that
< luke-jr>
gwillen: ssize_t isn't guaranteed to be large enough for all sizes?
< gmaxwell>
gwillen: we are misusing sanitization tools
< gmaxwell>
and using suppression files to cover up the misuse
< gmaxwell>
and then probably just giving up on making useful changes, to cover up the difficulty of updating the suppressions.
< gmaxwell>
there are a ton of sanitizers which are available which trigger on behavior that is sometimes (even only rarely) a mistake, and turning them on means either (1) you leave them producing false positives which you ignore, causing you to miss real reports, (2) go change the code in ways that silence them, often lowering code quality or even adding bugs, (3) suppress them, and then deal with the
< gmaxwell>
returning every time you add a new instance of the same idiom or refactor something
< gmaxwell>
In commercial tools like coverity these concerns are addressed by not emitting warnings for anything that has a high false positive rate (even if that misses some true positives), and by havinga nice database that lets you track suppressions, keep notes on them, and which manages to survive refactoring most of the time.