Time  Nick          Message
00:01 RealBadAngel  if i would make it blend with day night lights, they would become just invisible
00:04 RealBadAngel  ShadowNinja, also they do not change just overall lighting for visibility, separate color channels do cycle too
00:05 RealBadAngel  without that for example on leaves or obsidian you wouldnt notice that
00:15 RealBadAngel  but feel free to code it in a better way than i did
00:15 RealBadAngel  im going to sleep :P
00:17 VanessaE      sleep?
00:17 VanessaE      you can't do that
02:17 iio7          Hi. I am talking a quick look at the minetest code and I have three questions. Why are so much stuff commented out without any comments explaining why? Why the approx zero comments in general? And does the exist a coding standards for the project?
02:23 iio7          Found the style guide.
08:42 Megaf         Hi, I got a possible bug for you, and the bug developers like the most! Memory Leaks/Seg faults! https://github.com/minetest/minetest/issues/1619
08:51 Zeno`         Last I ran valgrind I detected no leaks
08:52 Megaf         yep, but valgring doesnt run mods...
08:52 Megaf         nor players
08:52 Zeno`         yes it does
08:52 Zeno`         I run it with mods..... probably not *your* mods though
08:53 Megaf         well, I said possible memory leaks
08:53 Megaf         that doesnt happens all the time
08:54 Megaf         I dont know what trigers that, I had yesterday close to 20 players at the same time and memory use was pretty low
08:54 Megaf         but at the moment of the crash there was less than 8 players
08:59 Zeno`         hmm
09:08 Megaf         and I rebooted the server hourse before the crash
09:21 Zeno`         Why does the debug build use -O1? That's kind of unusual isn't it?
09:53 Zeno`         Megaf, can you add what mods you use to the report?
09:53 Megaf         ep
09:53 Megaf         yep
09:56 Megaf         Zeno`: done
10:07 Zeno`         Ok, I'll try with all your mods and see if I can reproduce
10:09 Megaf         Zeno`: well, I think you will have no luck with that
10:09 Megaf         sometimes my server stays up for a week, till some mod bring it down
10:10 Megaf         and without leaks
10:34 RealBadAngel  ShadowNinja, ok, i made it adapting to light level at position
10:35 RealBadAngel  so it wont depend on day/night cycle just but a real light
10:36 Zeno`         RBA, do you know why the debug build uses -O1?
10:36 RealBadAngel  no idea
10:37 RealBadAngel  bbl, off to work
12:00 Megaf         #1620
12:01 Megaf         hm, bot wont show the issue anymore, ok. Request: Progress bar/status/info for /clearobjects. https://github.com/minetest/minetest/issues/1620
12:21 Megaf         It depends on the mod john_minetest
12:22 Megaf         I learned that NPC mod will get wiped out by builtin item mod but sheeps from Creatures wont
12:52 Zeno`         Are you here sfan5?
12:52 sfan5         yes
12:52 Zeno`         I've pushed an update that I hope your comment refers to, but I'm not sure
12:52 sfan5         what? where?
12:52 Zeno`         https://github.com/minetest/minetest/pull/1618
12:53 Zeno`         I wasn't sure if you were referring to the indentation or something else
12:53 sfan5         Zeno`: taking about the stange indent
12:53 Zeno`         yeah, thanks. I missed it
12:53 sfan5         my comment was not that clear.. soo
12:56 Zeno`         As to why I want those member functions to be const correct... it's because I'm insane
12:57 Zeno`         I'm not sure it's low priority if somebody happens to want to refactor main()
12:58 Zeno`         although *shrug*
15:52 Zeno`         ShadowNinja, are you saying my pull request doesn't compile for you?
15:53 ShadowNinja   Zeno`: Yes, I haven't pulled your patch, but making the same change locally doesn't work.
15:53 ShadowNinja   Zeno`: What compiler are you using?
15:53 Zeno`         You have to change the mutex to mutable
15:54 Zeno`         I just commented in the pull request
15:55 ShadowNinja   Zeno`: Ah, I didn't think of that keyword.
15:55 ShadowNinja   Zeno`: const references should also be used for parameters though.
15:59 Zeno`         ShadowNinja, correct
16:00 Zeno`         Did I miss something?
16:00 ShadowNinja   Zeno`: https://github.com/minetest/minetest/pull/1618#issuecomment-55285956
16:00 Zeno`         The reason for this is because I cannot refactor main to use const paramaters without this change
16:01 Zeno`         And in fact, the parameters should be const in a lot of other places as well, but this is just to "set it up" so to speak
16:02 ShadowNinja   Zeno`: Try to merge it with my commit, but it looks good.  You should also make Settings const for the operators, and lock the mutexes after the (this == &other) check.
16:03 Zeno`         Ok, I will check tomorrow
16:03 Zeno`         it's 2AM here and I'm already asleep :)
16:04 Zeno`         Yes I see what your saying now (looking at your commit)
16:04 ShadowNinja   Alright, I might just do it myself and merge it while you're gone.
16:04 Zeno`         Can you merge mine first?
16:05 Zeno`         meh, it doesn't matter so long as it's done
16:05 ShadowNinja   Zeno`: I'll merge them at the same time, but in seperate commits.
16:05 Zeno`         ok
16:05 Zeno`         msg me tomorrow if you need something (I doubt you will)
16:06 Zeno`         night
16:08 Zeno`         your changes look good but the unnecessary copying is avoided because you're passing by reference rather than them being const (I do agree 110% that the params should be const though)
16:09 Zeno`         u16 getU16Ask(const std::string &name, const std::string &question, const u16 def)      <--- def does not need to be const
16:09 Zeno`         it's not a reference nor a pointer so it's pass by value anyway
16:09 Zeno`         night
16:11 proller       remove this unused shit
16:45 hmmmm         why was m_mutex made mutable?
16:45 hmmmm         that's not necessary unless you're inheriting a class that has the member as const
17:41 ShadowNinja   hmmmm: It's necessary for const methods to be able to lock and unlock it.
17:46 ShadowNinja   hmmmm: Here's the combined patch: https://github.com/ShadowNinja/minetest/compare/minetest:master...ShadowNinja:settings-cleanup
17:47 ShadowNinja   Comments on that ^?
17:47 ShadowNinja   (anyone)
17:52 hmmmm         ShadowNinja, what's the point of this:  https://github.com/ShadowNinja/minetest/compare/minetest:master...ShadowNinja:settings-cleanup
17:52 hmmmm         you just introduced a memory leak with that btw
17:52 ShadowNinja   hmmmm: It makes less copies and lets you use getters on a const Settings.
17:53 ShadowNinja   hmmmm: Where's the meak?
17:53 ShadowNinja   leak*
17:53 hmmmm         s isn't deleted
17:53 hmmmm         &str[0] is not a copy
17:54 sapier        moving the lock in updateValue is dangerous
17:54 ShadowNinja   Ah.
17:54 sapier        same in update
17:55 sapier        those comparisons are far from atomic
17:55 hmmmm         indeed, you're comparing each and every member in Settings with the other one while one may be getting updated
17:57 ShadowNinja   hmmmm: It's a pointer comparison, not a value comparison.
17:58 hmmmm         &?
17:58 hmmmm         you sure
17:58 ShadowNinja   hmmmm: Yes, & -> Address of.
17:58 hmmmm         it's a reference
17:58 ShadowNinja   Yes.
17:58 hmmmm         that's equivalent to having
17:58 hmmmm         Settings *other
17:58 hmmmm         if *other ==
17:58 hmmmm         oh
17:59 hmmmm         I misread that
17:59 ShadowNinja   NP.
17:59 sapier        the difference is still it's not locking in between so you change concurrency behaviour ... well not sure if someone should rely on it
17:59 hmmmm         sorry, I thought that was if (other == *this) for some reason
18:00 hmmmm         sapier, it looks safe to me
18:01 ShadowNinja   sapier: You don't need to lock before the (this == &other), nothing else can change it.  And locking before that pretty much defeats the purpose of the optimization.
18:01 sapier        well it looks like something to trigger errors beeing there before ;-)
18:02 sapier        = operator relies on recursive mutex if not it's a deadlock
18:02 sapier        do we have recursive mutexes? usually they're quite slow
18:03 hmmmm         sapier, that operator would only be applied if you were comparing the structs
18:03 hmmmm         he's just comparing pointer values
18:04 sapier        I know that's what I meant with it may cause errors that have ever been in there as it's increasing chance of concurrent access to those members ... but that's not an error in this code
18:04 hmmmm         ShadowNinja, don't allocate and copy str in readFlagString, that helps nothing
18:04 ShadowNinja   sapier: If our mutexes aren't recursive then it was flawed before.
18:04 sapier        still the recursive mutex requirement is strange as this is meant to be an optimization
18:05 hmmmm         change it back to char *s = &str[0];
18:05 sapier        no
18:05 sapier        = operator didn't call update prior your optimitation
18:05 sapier        hmm + operator did
18:05 sapier        did anyone ever use it?
18:06 hmmmm         afiak nobody uses operator += and =, I am very much against operator overloading, ESPECIALLY =, so I'd say we should get rid of that entirely
18:06 sapier        I'd prefere not to have a recursive mutex requirement, as I said for what I experienced they are extremely slow
18:08 ShadowNinja   sapier: Before it depended on recursive mutexes, operator= called clear() and copy() which create their own AutoLocks.
18:08 sapier        at least jthread doesn't set the PTHREAD_MUTEX_RECURSIVE flag
18:08 ShadowNinja   So it could clear someone elses lock or unlock too soon.
18:08 sapier        that's strange I'd have a very close look at it
18:09 ShadowNinja   Actually, it still uses (or tries to use) recursive mutexes there.
18:09 sapier        possibly those are error which never have been triggered
18:09 ShadowNinja   operator+= is fixed though.
18:10 hmmmm         what if you remove the lock from update, change it into a private method, and then make a public wrapper that locks and then calls the private update
18:11 hmmmm         and you just lock in updatePublic, += and =
18:11 hmmmm         again, though, I'd prefer it if you just removed += and =
18:11 sapier        I'd suggest checking this first because current code seems to be wrong too in worst case those functions are just never used
18:12 sapier        or maybe the mutex itself isn't initialized ;-) wouldn't be first time noone checks for errors ;-)
18:13 sapier        hmm no jthread checks for return values .. at least if assert is not a nop
18:13 ShadowNinja   That could work.  This entire thing is inlined though, so I'll have to move things around.  Is there a good reason for any but the most often-called functions (get()/set()) to be inlined?
18:15 sapier        if those mutexes are recursive it's pure chance we never set it, so if default behaviour changes we'd be stuck ... unless those functions aren't ever used
18:16 sapier        I like the const changes
18:21 ShadowNinja   I fixed the locking by adding clearNoLock and updateNoLock.  Next I'll split it into a .cpp and a .h, unless there's an objection.
18:21 sapier        no I don't know how often I was looking for settings.cpp till I remembered there ain't one
18:22 ShadowNinja   sapier: Is that approval or an objection?
18:22 sapier        that's an approval for your split
18:22 ShadowNinja   Alright.
18:24 sapier        btw is there any progress in the player deletion cause?
18:24 sapier        is anyone working at it at all? ;-)
18:26 ShadowNinja   Hmmm, Settings::remove is thread-unsafe.
18:26 ShadowNinja   sapier: Nope.
18:29 sapier        I know that the other bug is hard to track down, but don't you think trying to find it would be more valuable then doing maintenance? I'm a little bit short on time so I can't do it atm
18:34 ShadowNinja   Um, there are two getS16NoEx's, and one takes an int, which is usually 32 bits.
18:34 sapier        I suggest changing to uint16_t
18:44 ShadowNinja   sapier: There's a u16 version.  If you like I could change it to use standard fixed-width types.
18:45 sapier        as I said I'm a little bit short on time atm ;-)
19:17 sapier        how may I help you john_minetest
19:23 sapier        well testing is always welcome, for what I know we've got a hard to reproduce crash right now, if someone managed to find a way to reproduce it we could fix it way more fast.
19:25 sapier        current git master
19:26 sapier        https://github.com/minetest/minetest/issues/1592
19:26 sapier        one of two bugs
19:29 sapier        a crash with even less information how to reproduce ;-)
19:30 sapier        maybe broken udp packets are involved in the crash
19:30 sapier        but that's a guess only
19:32 sapier        yes that pne seems to be a bug too ... but the information doesn't help very much ;-)
19:32 sapier        do you have a debug build?
19:34 sapier        cmake -DCMAKE_BUILD_TYPE=Debug
19:38 sapier        well it's us to thank you for testing john_minetest
19:43 sfan5         malformed udp packets are dropped at the kernel
19:43 sfan5         this doesn't affect minetest
19:43 sapier        sfan5: ppl tell different ;-)
19:44 kahrl         perhaps the client sends a malformed packet, maybe because of memory corruption
19:44 sapier        hmm thinking about it ... maybe broken udp packets and crash have a common usecase
19:44 sapier        kahrl: same thought ;-)
19:44 kahrl         or because it's a hacked client :P
19:44 sapier        yes but that wouldn't crash server
19:45 sfan5         the client can't send a malformed udp packet (udp itself, not the data)
19:45 sfan5         it would need root for that
19:45 kahrl         sfan5: malformed in the application layer sense
19:45 sapier        but broken memory on server could cause udp packet verification failure as well as random minetest crashes
19:45 sfan5         thats ofc possible
19:45 sfan5         um
19:45 sfan5         wat
19:45 sfan5         oh
19:45 sfan5         you mean broken RAM
19:45 sapier        yes
19:46 sfan5         broken RAM will likely have more issues that just minetest crashing and udp packets failing to verify
19:46 sapier        a malformed network packet no matter how malformed shall never crash minetest server, worst thing it's supposed to cause is drop of a client
19:46 sapier        sfan5: yes but lots of strange problems
19:47 kahrl         sfan5: maybe minetestserver is the only thing that eats enough RAM to exercise the broken RAM regions
19:47 sapier        today chances for memory to be used for non exact data are quite big, e.g. jpg files, video streams audio streams
19:47 sfan5         then run memtest ¯\_(ツ)_/¯
19:48 sfan5         also most memory is always used by the kernel for cache
19:48 sfan5         and video streams throw errors
19:48 sapier        no
19:48 sfan5         at least H.26[3-5] does
19:48 sapier        most video codes just ignore small errors as they contain redundancy
19:48 sfan5         it depends on the error
19:48 sapier        of course
19:49 sfan5         if metadata is corrupted there will likely be an error
19:49 sfan5         image data not so much
19:49 sapier        I just wanted to illustrate that it's quite likely for memory errors to cause that subtile issues ppl don't detect it for quite some time
19:49 sapier        of course if 1GB of your 4gb is broken you'll notice ;-)
19:50 sfan5         you'll notice anyway
19:50 sfan5         the kernel uses it for cache
19:50 sfan5         but I dunno whether the cache is checksummed
19:50 sapier        well last time I had a faulty memory module it tool me months to pinpoint the issue
19:51 sapier        some big files broken every now and then, a crash here and there
19:51 sapier        but 99% of what I did on that machine was perfectly ok
19:53 sapier        but as you said memtest is quite good at finding things like that ... once you run it ;-)
19:55 sapier        so debug build changes timing to an extent where the bug doesn't happen any longer
19:55 sfan5         then use RelWithDebugInfo
19:55 sfan5         "RelWithDebInfo"
19:56 sapier        sfan5: suggestion is worth a try, it's still slower but way more near to release
19:56 sfan5         why slower?
19:56 sapier        bigger file size
19:56 sfan5         wat
19:56 sfan5         how does that matter?
19:57 sfan5         it's loaded into RAM fully before being executed
19:57 sapier        load time as well as possible page faults
19:57 sapier        no it ain't
19:57 sfan5         and the debug regions are probably not loaded anyway
19:57 sfan5         [citation needed]
19:57 sapier        parts of code are usually loaded on first usage only
19:58 sapier        well if they're not loaded they wont harm, yet it's not really relevant it's worth a try in any case
20:00 sfan5         loading only parts of an executable doesn't improve speed at all
20:01 sfan5         the .rodata, .data, .text section will be used for sure, it's only slower not to load them at startup
21:31 ShadowNinja   A lot of files depend on settings.h to include filesys.h, util/serialize.h, log.h, strfnd.h, etc.
21:34 ShadowNinja   I now know why you should add an include for everything a file depends on, I've spent the last 10-20 minutes adding includes.
21:53 RealBadAngel  ShadowNinja, thx for yesterday ideas, i made highlighting blend with regular light
21:53 RealBadAngel  it works suprisingly good
21:54 ShadowNinja   RealBadAngel: Good.  I'll check it when I have a chance, but I'm busy now.
21:54 RealBadAngel  and doesnt depend on daytime, only the light around
21:54 RealBadAngel  i need to fine tune it, will update the pull later today
22:19 ShadowNinja   Finally done, only had to modify 23 files more than the three it should have been (source, header, make)
22:23 ShadowNinja   https://github.com/ShadowNinja/minetest/compare/settings-cleanup
22:24 ShadowNinja   I did some cleanup in addition to the split.
22:24 ShadowNinja   hmmmm: ^
22:26 VanessaE      ShadowNinja: RealBadAngel is gonna kick your ass for this... :P
22:26 ShadowNinja   VanessaE: Hmmm?  Why?
22:27 VanessaE      still reading the code but I *think* this is very much what he wanted to do.  you stole his thunder :)
22:28 VanessaE      hm, nope
22:28 VanessaE      not quite
22:28 VanessaE      you're just reworking all the underlying handlers
22:29 VanessaE      he still gets to play :)
22:32 hmmmm         ShadowNinja:  looks good, but why do you add the const qualifier to an integer being passed by value...
22:32 ShadowNinja   hmmmm: Where?  Doesn't really matter I guess.
22:33 hmmmm         all over
22:33 ShadowNinja   Just writeFlagString().
22:38 ShadowNinja   Changed.
22:48 ShadowNinja   Hmmm, I broke something somehow, the world doesn't load and the game won't shut down without tripple-Ctrl-C.
22:49 kahrl         sounds like a deadlock?
22:50 ShadowNinja   Could re-sorting the order that the sources are listed in in CMakeLists.txt break it?
22:50 kahrl         no
22:51 ShadowNinja   Otherwise yes, maybe a lock is failing somehow.  But it uses only JMutexAutoLocks, which should be foolproof.
22:51 kahrl         didn't sapier say that they are non-recursive?
22:51 ShadowNinja   Yes.
22:52 ShadowNinja   I'll check if they recurse anywhere, but I don't think that would cause this.
22:55 ShadowNinja   Doesn't look like it.
22:56 ShadowNinja   Hmmm, maybe I have a while (<always true>) {} somewhere.
22:57 * ShadowNinja looks to see if GDB can help
23:01 ShadowNinja   `thread apply all bt` shows no mention of Settings.  There are some threads waiting on locks or sellping though.
23:03 kahrl         yeah same here
23:03 kahrl         there are a few waiting on ClientInterface::m_clients_mutex
23:04 ShadowNinja   Exact same mutex here too.  It's either that, nanosleep(), select(), sem_timedwait(), or poll().
23:07 ShadowNinja   ClientInterface::sendToAll and ClientInterface::getClientIDs are fighting for m_clients_mutex on shutdown.
23:08 ShadowNinja   Both in __lll_lock_wait() (pthread_mutex_lock()).
23:09 kahrl         indeed
23:09 ShadowNinja   kahrl: Any ideas for debuging?  I'm not very familiar with GDB.
23:10 kahrl         not really
23:10 kahrl         well, you could do "thread apply all bt full" to see even more info
23:11 kahrl         http://stackoverflow.com/questions/3483094/is-it-possible-to-determine-the-thread-holding-a-mutex
23:11 kahrl         ^ might help
23:13 kahrl         another thing to try would be to revert the settings changes and try if the other changes by themselves cause the deadlock too
23:17 kahrl         ok, so ServerThread is in ClientInterface::getClientIDs trying to lock m_clients_mutex which is locked by ServerThread
23:17 ShadowNinja   kahrl: That shows that ServerAsyncRunStep -> Server::SendBlocks -> ClientInterface::getClientIDs owns the mutex.  There are really only two commits that could cause this, and one is tiny, but I'll try.
23:19 ShadowNinja   kahrl: There's only one ServerThread, main is waiting on ServerThread.
23:20 ShadowNinja   The latest, big commit is causing it.
23:20 ShadowNinja   (But how!)
23:25 ShadowNinja   Include order?
23:26 kahrl         sounds unlikely but at this point everything is possible
23:29 kahrl         why does server.cpp:3856 return without unlocking m_clients?
23:30 kahrl         (but changing return to continue there doesn't fix the deadlock)
23:31 kahrl         this whole concept of locking m_clients_mutex from outside ClientInterface is really ugly
23:40 ShadowNinja   Indeed, but how is my settings patch affecting it?
23:40 kahrl         dunno
23:40 kahrl         OK, with some printf debugging it seems that the m_clients_mutex was last locked by server.cpp:1171 in StageTwoClientInit
23:40 kahrl         (I only traced locks from server.cpp)
23:47 ShadowNinja   emergePlayer is throwing an exception.
23:49 kahrl         "invalid inventory specifier"
23:51 ShadowNinja   Settings might not be eating the EndArgs or something like that...
23:52 kahrl         or it reads too much
23:53 ShadowNinja   Maybe it's just leaving the '\n'...
23:55 kahrl         does anything initialize end_found to false??
23:55 kahrl         -?
23:58 ShadowNinja   No...
23:59 kahrl         ok, but fixing that still doesn't fix the emergePlayer exception