< wumpus>
the path length is not actually an issue, it's just a general warning that it always throws
< wumpus>
it's kind of annoying but only in the way that it's overly apparent in the log so it's always a false lead when something goes wrong in appveyor
< fanquake>
wumpus ah ok. Having trouble keeping up with Appveyor changes.
< wumpus>
the build is based on some package system (vcproj) which has its own breaking changes all the time, I don't know if it offers something like version pinning (it would avoid things randomly breaking on one hand, on the other it'd be one more set of dependency versions to keep up to date...)
< luke-jr>
why can't Appveyor just use autotools?
< wumpus>
anyhow I don't know if the new error is caused by that or some change of our own this time: "C:\projects\bitcoin\src\sync.h(139,23): error C2220: the following warning is treated as an error [C:\projects\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]"
< wumpus>
because eeeeveryone likes to do things their own way
< wumpus>
the last change in sync.h was in september
< wumpus>
"Tries to lock the mutex. Returns immediately. On successful lock acquisition returns true, otherwise returns false. "
< wumpus>
I wonder why we're ignoring that return value and using Base::owns_lock instead
< bitcoin-git>
[bitcoin] laanwj opened pull request #17707: util: Use try_lock return value in UniqueLock::TryEnter (master...2019_12_try_lock_returnval) https://github.com/bitcoin/bitcoin/pull/17707
< wumpus>
^^ the other option would be to disable the warning, but I think avoiding unnecessary calls to owns_lock is better
< wumpus>
I have very little interest in language lawyering but AFAIK misaligned integers/pointers can sometimes lead to real issues
< sipa>
wumpus: i suspect little impact right now in production
< sipa>
becaus epretty much all modern platforms don't care about alignment (at worst it's a performance issue), and for cases where alignment is actually necessary, it'd be an immediate trap rather than misbehavior
< sipa>
but i think there is a concern that compilers may misoptimize in the futurr
< sipa>
and of course it interferes with sanitizers that correctly detect misaligned access...
< wumpus>
ok but an immediate trap is also bad
< wumpus>
we don't want the software to crash right?
< wumpus>
do we, in general, try to avoid unalgned accesses in the code?
< wumpus>
(this is the only use of pragma(pack) FWIW)
< wumpus>
I think it's safer to remove it
< sipa>
i'm very surprised that pragma pack actually makes the compiler generate code it considers itself incorrect
< wumpus>
FWIW it's the same in rust, pragma(pack) is a cesspool of UB
< sipa>
looking at the actual chabge
< sipa>
change
< wumpus>
it's for when a data structure needs to be tightly packed in memory, but those are really exceptional, generally mis-informed protocol parsing and such
< wumpus>
I don't think that's the case here, none of that code relies on the struct's binary layout
< sipa>
yeah, my only question is if this significantly changes the size of CScript
< sipa>
as doing so may affect the performance of CCoinsCache
< sipa>
if it doesn't affect it significantly we should just remove the pragma
< wumpus>
but using UB to get a speedup is a bit questionable
< sipa>
oh absolutely!
< wumpus>
I think that's over the line that is reasonable for a probject like this
< sipa>
we need to get rid of the pack regardless, if it causes invalid code
< sipa>
but if just removing it causes a cache impact, we should perhaps look for another solution to get better memory usage
< wumpus>
sure, that's always a good thing
< aj>
CScriptBase goes from 32 to 40 bytes for me on clang, after dropping the pragma :(
< wumpus>
same in gcc
< wumpus>
I do not really understand why, though
< wumpus>
let's try shuffling the fields inside prevector
< aj>
i think having the pointer in the union is forcing it to be aligned and/or padded, but shuffling isn't fixing it for me (yet at least)
< wumpus>
I've moved the char* pointer first, then the capacity, then the size
< wumpus>
(but it has to recompile almost everything...)
< aj>
wumpus: yeah, me too. i'm getting the union as 32 bytes despite the array being 28 bytes and the struct having 8+4 bytes
< wumpus>
oh
< aj>
or is sizeof automatically a multiple of alignof? yeah that's it isn't it?
< wumpus>
yes, I think so
< wumpus>
I wonder if there's a way to express not wanting to waste unnecessary bytes in/after the union but still putting everything at the required alignment
< wumpus>
the final word only needs 4-byte alignment not 8-byte
< wumpus>
(... putting the size *inside* the union might solve it, but would be even scarier because it'd rely on the two union members overlapping in a certain way)
< aj>
wumpus: could move the pragma to only be around the union declaration, i think that works and would fix the observed bugs, but wouldn't quite guarantee that it was aligned at 8 bytes
< wumpus>
it wouldn't fully solve the problem either
< wumpus>
exactly, the outer structure would then get an alignment of 4 instead of 8, I suppose
< aj>
putting size inside the union looks like it'd just invoke other sets of undefined behaviour
< wumpus>
yes
< wumpus>
it's aliasing
< sipa>
hmm, maybe the better question is the sizeof of CCoin
< sipa>
there is a lot more in there than just a CScript
< sipa>
#17060 contains a commit that changes the prevector layout as well, and iirc also removes the pragma pack
< gribble>
https://github.com/bitcoin/bitcoin/issues/17060 | Cache 26% more coins: Reduce CCoinsMap::value_type from 96 to 76 bytes by martinus . Pull Request #17060 . bitcoin/bitcoin . GitHub
< wumpus>
it doesn't seem to remove the pragma pack
< sipa>
oh, then i misremember
< wumpus>
it does put a size inside the union, in addition to the size outside it (which is a smaller type)
< aj>
#pragma pack union d_o_i { }; #pragma pack(pop) alignas(alignof(char*)) d_o_i _union = {}; size_type _size = 0; with char*indirect before size_type capacity; seems to work
< sipa>
will that not generate unaligned access toi?
< sipa>
too?
< wumpus>
if you can reliably control the alignment of the outer structure, you can pack the fields so that no unaligned access happens
< wumpus>
(I think that's already OK with current prevector, if it happens to end up on a 8-aligned address)
< aj>
sipa: if you used direct_or_indirect elsewhere you could, but alignas() forces _union to be aligned right, and that forces CScritBase to also require 8 byte alignment everywhere it's used, which should fix things i think
< wumpus>
yes, forcing prevctor at align(sizeof(char*)) should do it
< sipa>
ah
< wumpus>
e.g. the current inside of a prevector is, in the indirect case: uint32_t uint32_t char* ... which is ok, at least for architetectures with 32 and 64 bit pointers, to be even more sure move the char* to the beginning and the size to the end
< aj>
wumpus: posted a diff in the PR that seems like it works right for me
< wumpus>
that's quite a bigger change than I expected
< aj>
it's mostly comments and static_asserts
< wumpus>
shouldn't you alignas the *outer* structure?
< wumpus>
I'm giving up here, I admit I don't know enough about C++ compilers to do this safely
< aj>
i think you could `class alignas(alignof(char*)) prevector { .. }`
< aj>
could probably *just* do that and not change the pack pragma or reorder anything even
< sipa>
aj: but what is the resulting impact on CCoinCacheEntry or whatever it is called?
< wumpus>
aj: ok, looking at your diff more I think putting the #pragma pack(push,1) inside the structure is a good idea, it makes sure that the size_type size is still aligned correctly no matter what sizeof(T)*N is
< wumpus>
aj: can't find anything wrong with it, I think it would even work out for 128 bit pointers
< wumpus>
e.g. the beginning of _union is aligned to alignas(alignof(char*)), which is where the pointer is
< aj>
(that's `class alignas(char*) prevector { .. }`, so it seems to work right)
< aj>
yeah, sizeof,alignof
< wumpus>
LGTM then, that should solve the alignment issue without increasing the size of any of the structures
< sipa>
nice!
< bitcoin-git>
[bitcoin] ajtowns opened pull request #17708: prevector: avoid misaligned member accesses (master...201912-prevec-pack) https://github.com/bitcoin/bitcoin/pull/17708
< vasild>
Ah, I am late to the alignment party, but let me throw my $0.02: the program would indeed crash on nonaligned access at least on some platforms (Solaris/sparc64). E.g. if you have a uint64_t pointer that points to a location that is not aligned on a 8 byte boundary. x86 is more forgiving - no crash, but however the read or write is split in two words so it is not atomic anymore which could cause
< vasild>
difficult to debug bugs if some code assumes that read/writes to 64 integers are atomic on 64 bit CPUs (which they are if aligned, and yes, better use std::atomic).
< wumpus>
we definitely don't assume writes to CScript are atomic :)
< sipsorcery>
re. the latest appveyor job failures I'm not seeing any problem with local msvc builds.
< sipsorcery>
I'll do some checks by switching one of my appveyor jobs to the previous VS2019 image.
< wumpus>
they should really stop changing those kind of things behind our back
< bitcoin-git>
[bitcoin] sipsorcery opened pull request #17709: msvc: Add an ignore for warning C4834 related to nodiscard attribute. (master...msvc_ignore_c4834) https://github.com/bitcoin/bitcoin/pull/17709
< bitcoin-git>
[bitcoin] laanwj closed pull request #17707: util: Use try_lock return value in UniqueLock::TryEnter (master...2019_12_try_lock_returnval) https://github.com/bitcoin/bitcoin/pull/17707
< bitcoin-git>
[bitcoin] sipsorcery opened pull request #17710: msvc: Change appveyor build image to previous visual studio 2019. (master...appveyor_prev2019) https://github.com/bitcoin/bitcoin/pull/17710
< aj>
hmm, with taproot, should we consider bumping CScript from 28 bytes (which fits p2pkh(25B), p2sh(23B) and p2wpkh(22B)) to 36 bytes (which would also fit p2wsh(34B) and p2tr(34B))?
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #17704: depends: Set default depends fallback url to drahtbot.space (master...1912-dependsDrahtBot) https://github.com/bitcoin/bitcoin/pull/17704
< bitcoin-git>
[bitcoin] instagibbs opened pull request #17712: sendmany/bumpfee: Avoid address reuse and partial spends as per walle... (master...avoid_reuse_sm_bf) https://github.com/bitcoin/bitcoin/pull/17712
< bitcoin-git>
bitcoin/master 1bb5d51 Sebastian Falbesoner: test: add unit test for non-standard bare multisig txs
< bitcoin-git>
bitcoin/master ea756bc MarcoFalke: Merge #17502: test: add unit test for non-standard bare multisig txs
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #17502: test: add unit test for non-standard bare multisig txs (master...20191118-test_check-for-non-standard-txs-bare-multisig) https://github.com/bitcoin/bitcoin/pull/17502