| 00:01:27 | *** | Saving seen data "./dancer.seen" |
| 01:00 |
| 01:46:34 | | Join Maxdamantus [0] (~Maxdamant@user/maxdamantus) |
| 01:50:24 | Arsen | speachy: four ports on the same controller fail in different way and various other devices wrok, though, and same cable works on other places |
| 01:51:05 | Arsen | fron 3.0 ports fail after a while, resetting, 2.0 ones never figure out configuration |
| 02:00 |
| 02:01:28 | *** | No seen item changed, no save performed. |
| 02:50:26 | | Quit Maxdamantus (Ping timeout: 256 seconds) |
| 02:51:46 | desowin | Arsen: can you share pcap? do you have OpenVizsla or similar? |
| 02:52:39 | Arsen | no I'm not usually working on this part of the kernel |
| 02:53:24 | Arsen | I can share the usbmon capture thon |
| 02:53:41 | Arsen | after i sanitize it to see whether any unrelated and potentially sensitive data is there |
| 03:00 |
| 03:05:36 | desowin | you mean actual data stored on the drive or other devices? |
| 03:06:32 | desowin | if the latter, then simply filtering for device number and saving filtered packets should do |
| 03:07:25 | | Join Maxdamantus [0] (~Maxdamant@user/maxdamantus) |
| 03:08:24 | | Quit akaWolf (Ping timeout: 252 seconds) |
| 03:10:17 | | Join akaWolf [0] (~akaWolf@akawolf.org) |
| 03:33:28 | Arsen | both really |
| 04:00 |
| 04:01:32 | *** | Saving seen data "./dancer.seen" |
| 05:00 |
| 05:43:34 | Arsen | did anyone try implementing progressive jpegs or was there just no interest in that topic? I don't know why it's more complicated compared to baseline, if it even is, so I figured I should ask before looking into it |
| 05:45:26 | gevaerts | I don't know *any* details, but Unhelpful (who did the work) has said at some point that progressive is not possible in the same efficient way the rest works |
| 05:46:50 | gevaerts | Our jpeg decoder does decoding and scaling at the same time and doesn't need more RAM for bigger images (you can show 20 megapixel jpegs on your lower-end rockbox device if you really want to, although that tends to be slow), which is essential for our use |
| 05:49:01 | Arsen | afaik the point of progressive jpegs is for data to be separated out such that it slowly increases in resolution as you proceed through the file, maybe it's possible to end early? - an uneducated guess |
| 05:51:12 | gevaerts | I have no idea :) |
| 05:54:13 | gevaerts | I'll find the logs where he explained it |
| 05:56:16 | gevaerts | https://www.rockbox.org/irc/log-20090502#02:42:31 and https://www.rockbox.org/irc/log-20090514#08:46:06 |
| 05:56:54 | gevaerts | Note that I don't really understand most of that |
| 05:57:31 | gevaerts | All I can do is find links :) |
| 05:58:24 | Arsen | ah, this makes sense |
| 05:59:51 | Arsen | desowin: alright, I read the entire ptrace, it should be clean, where would you prefer me to upload it for you, if you have a preference? |
| 05:59:58 | Arsen | s/ptrace/pcap/ |
| 06:00 |
| 06:01:35 | *** | No seen item changed, no save performed. |
| 06:05:17 | Arsen | desowin: if you don't mind it being on my own webserver, here you go: https://www.aarsen.me/~arsen/failed_flac_write.pcapng |
| 06:31:21 | | Quit akaWolf (Ping timeout: 258 seconds) |
| 06:46:25 | braewoods | Arsen: you want assistance submitting a quirks patch? |
| 06:46:51 | braewoods | i've done this before but i'd need to know what the quirk is you're needing patched |
| 06:47:13 | Arsen | yeah, I don't yet, I'll investigate further |
| 06:47:23 | Arsen | if you have notes/docs about the process, I'm all ears though |
| 06:47:36 | braewoods | depends, i've mainly done audio quirks. |
| 06:47:42 | braewoods | and depends where it's required. |
| 06:48:03 | braewoods | if it requires patching the kernel, you'll need to add hardware IDs to the in kernel driver table. |
| 06:48:10 | braewoods | and maybe some other bits |
| 06:48:46 | Arsen | probably the USB driver |
| 06:48:52 | braewoods | if it's keyboard related that goes in the hardware database shipped with systemd |
| 07:00 |
| 07:15:09 | | Join akaWolf [0] (~akaWolf@akawolf.org) |
| 07:51:27 | | Quit kadoban (Quit: Bridge terminating on SIGTERM) |
| 07:51:29 | | Quit blbro[m] (Quit: Bridge terminating on SIGTERM) |
| 07:54:04 | | Join kadoban [0] (~kadoban@user/kadoban) |
| 08:00 |
| 08:00:47 | | Join blbro[m] [0] (~blbrostra@2001:470:69fc:105::8f7) |
| 08:01:36 | *** | Saving seen data "./dancer.seen" |
| 08:08:13 | | Join mixfix41 [0] (~user@user/mixfix41) |
| 08:20:16 | | Join cockroach [0] (~blattodea@user/cockroach) |
| 08:44:41 | Arsen | braewoods: no, it's most likely either usbms or xhci_hid in the kernel |
| 09:00 |
| 09:04:23 | | Quit F3l1x_10m (Read error: Connection reset by peer) |
| 09:07:22 | | Join massiveH [0] (~massiveH@ool-18e4e82f.dyn.optonline.net) |
| 09:37:46 | | Quit Maxdamantus (Ping timeout: 268 seconds) |
| 09:44:14 | braewoods | this is a warning i've never seen before |
| 09:44:17 | braewoods | warning: comparison of promoted ~unsigned with unsigned |
| 09:44:28 | desowin | Arsen: hardware level capture would tell a lot here, it didn't manage to send whole payload (240 * 512) but the length it managed to send (99 * 512) |
| 09:44:48 | desowin | do you have corresponding logf? |
| 09:46:05 | braewoods | oh well, converted it to xor equivalent |
| 09:46:07 | desowin | if you enable ramdisk you can try to determine if the iflash has any impact on it |
| 09:46:30 | Arsen | I don't think I have it, lemme look through some logs |
| 09:46:44 | desowin | as ramdisk will simply present a chunk of ram as the disk to the host |
| 09:46:53 | Arsen | and it almost certainly doesn't at this point, since a different computer works |
| 09:47:29 | Arsen | different controllers in this PC even work |
| 09:47:39 | desowin | the port reset is just a consequence of OUT URB timing out |
| 09:47:45 | Arsen | nope, lost the logs, rip |
| 09:47:47 | Arsen | yeah, thought so |
| 09:48:16 | desowin | so there's either a signal integrity issue (hardware packet level capture would tell if that's the case) or something in software |
| 09:48:29 | braewoods | a little closer to finishing inflate support |
| 09:48:37 | braewoods | next i need to add the huffman engine |
| 09:49:17 | Arsen | `logf("transfer result for ep %d/%d %X %d", ep,dir,status, length);` this log was triggered followed by a scsi write10 0, I think |
| 09:49:42 | Arsen | in usb_storage.c |
| 09:49:47 | Arsen | I'm not sure I saw the write10 at the end |
| 09:49:57 | Arsen | I'll get another debug build |
| 09:50:43 | Arsen | desowin: how do you know it didn't manage to send the full payload? |
| 09:51:02 | desowin | it didn't manage to send the full payload |
| 09:51:24 | desowin | it got 99 packets (out of 240) through |
| 09:51:27 | Arsen | yes, but what indicates that? |
| 09:51:38 | Arsen | oh, where are those numbers from? |
| 09:51:56 | desowin | URB length in the "packet" when the URB returns from host controller |
| 09:52:08 | Arsen | the ECONNRESET one? |
| 09:52:11 | desowin | yes |
| 09:52:13 | Arsen | ah |
| 09:53:55 | Arsen | what files should I put the logfs into? |
| 10:00 |
| 10:01:37 | desowin | firmware/drivers/usb-designware.c and firmware/usbstack/usb_storage.c |
| 10:01:39 | *** | Saving seen data "./dancer.seen" |
| 10:01:53 | Arsen | presumably USB serial logging won't work for this? :^) |
| 10:02:24 | desowin | you can have it compiled in, but don't enable to not distort the result |
| 10:02:30 | Arsen | yeah |
| 10:02:35 | desowin | you can simply dump log to file |
| 10:02:42 | Arsen | oh, right, that works |
| 10:02:47 | Arsen | after I fix the FS that is :P |
| 10:03:13 | desowin | FS meaning file system or full speed USB? |
| 10:03:19 | Arsen | filesystem |
| 10:06:07 | desowin | capture pcap as well to have the logf and pcap from same session |
| 10:06:22 | Arsen | sure, I'll just transfer on the debug build now |
| 10:06:56 | Arsen | sorry if I take some time, I'm dealing with some other issue as well, unrelated to CF |
| 10:20:32 | | Quit massiveH (Quit: Leaving) |
| 10:36:18 | | Quit spork (Ping timeout: 255 seconds) |
| 11:00 |
| 11:19:26 | Arsen | right, I'm back, sorry about that |
| 11:43:12 | Arsen | alright, I can see logs, lets see what happens now |
| 11:47:04 | Arsen | desowin: got the logs and stuff |
| 11:49:41 | Arsen | this buffer is tiny |
| 11:49:47 | Arsen | I'm not sure if I have the right bit there |
| 11:49:55 | Arsen | most likely not, I'll redo this |
| 11:50:09 | Arsen | issue is, I need(?) to fsck before the dump |
| 11:50:19 | Arsen | I'll try again since I can't corelate it myself either |
| 11:55:07 | Arsen | ok I can corelate it this time |
| 11:55:21 | Arsen | desowin: .tar of the two on the same place works for you? |
| 12:00 |
| 12:01:43 | *** | Saving seen data "./dancer.seen" |
| 12:10:45 | Arsen | https://aarsen.me/~arsen/20210719_1737_usbstuff.tar.zst |
| 12:32:10 | | Join spork [0] (topic@31-151-2-135.dynamic.upc.nl) |
| 12:38:51 | | Join ZincAlloy [0] (~Adium@ip5f5abcae.dynamic.kabel-deutschland.de) |
| 12:42:56 | desowin | I don't quite know this usb (device) chip, so not really much I can do here, but looking at the logs I would be suspicious about Rockbox USB driver (not the stack) |
| 12:43:11 | | Quit ZincAlloy (Ping timeout: 258 seconds) |
| 12:44:21 | desowin | depending how much you want to debug, if I was debugging this, I would add logf to usb_drv functions, especially usb_drv_recv to see if it is called before the failure occurs (I assume yes, but you never know) |
| 12:48:32 | desowin | getting 3 packets on top of the successfully received two 24 KiB chunks is suspicious |
| 12:49:17 | | Join ZincAlloy [0] (~Adium@2a02:8108:943f:d824:ca6:7e8e:19f4:c808) |
| 12:53:44 | | Quit ZincAlloy (Ping timeout: 255 seconds) |
| 13:00 |
| 13:00:43 | | Join lebellium [0] (~lebellium@2a01:cb10:2e:2000:5063:e165:3a35:2673) |
| 13:19:14 | | Join ZincAlloy [0] (~Adium@2a02:8108:943f:d824:f5db:f084:d263:e397) |
| 13:25:13 | Arsen | desowin: you saw that in the logf? |
| 13:25:22 | Arsen | I'd like to debug this honestly, I've got the time right now |
| 13:26:27 | Arsen | I don't see the three packets though |
| 13:29:20 | desowin | URB finished writing 50688 bytes, but there were two times transfer result for ep 2/0 0 24576, the difference is three packets (high speed maximum packet size is 512) |
| 13:29:50 | desowin | host can only write in multiples of maximum packet size, unless the packet ends transfer (1 URB = 1 transfer) |
| 13:31:25 | desowin | the device acknowledged these three packets, and stored them *somewhere* |
| 13:32:39 | desowin | next ones were most likely NAKed (most liekly PINGs were NAKed, not the actual OUT transactions), but to know for sure you'd need hardware level sniffer |
| 13:32:55 | Arsen | after the NAK the timeout happens? |
| 13:33:30 | desowin | there are thousands of NAKs |
| 13:33:56 | desowin | the ECONNRESET "packet" in Wireshark is 30 seconds after the URB submit |
| 13:34:05 | desowin | so host was trying to send the data for 30 seconds |
| 13:34:16 | Arsen | and getting nak'd the entire time? |
| 13:34:38 | Arsen | so, usbmon actually logs requests for something to happen on usb, not what's actually happening on usb? |
| 13:34:48 | desowin | not entire time, it got ACKed 99 times (50688 / 512) |
| 13:34:49 | Arsen | (and their results) |
| 13:35:21 | Arsen | ah |
| 13:35:48 | desowin | yes, usbmon logs requests for something to happen on usb bus |
| 13:36:00 | Arsen | right, that makes a lot of sense |
| 13:36:18 | desowin | https://www.youtube.com/watch?v=cUljKImph4s this is recording of my SharkFest '20 Virtual talk on USB |
| 13:36:46 | Arsen | how much is that sniffer you mentioned earlier? |
| 13:36:50 | Arsen | as a side note |
| 13:36:57 | Arsen | (the cost of acquiring it) |
| 13:37:55 | desowin | explains what usbmon captures and how it differs from the packets on the bus, nowadays if you get https://gitlab.com/wireshark/wireshark/-/merge_requests/2848 the usbll capture will be actually reassembled |
| 13:38:07 | desowin | for sharkfest I just correlated the packets manually |
| 13:38:16 | Arsen | I could pull in usbll if it can help here |
| 13:38:36 | Arsen | this patch* |
| 13:38:37 | Arsen | not usbll |
| 13:39:06 | desowin | the patch only affects usbll captures, so no change for usbmon capture |
| 13:39:16 | Arsen | ah, and that requires some device? |
| 13:39:33 | desowin | http://shop.sysmocom.de/products/openvizsla-v3-dot-2-usb-protocol-analyzer-pcba has assembled OpenVizsla for 154.70 EUR + shipping |
| 13:39:54 | desowin | but mind you, just getting usbll capture is only a beginning of actual troubleshooting |
| 13:40:39 | desowin | but it can for sure keep you from wandering entirely wrong direction as it actually shows what happens on the bus |
| 13:41:11 | Arsen | yeah |
| 13:45:37 | Arsen | that number is oddly specific, though |
| 13:45:43 | Arsen | 99, and I think that was happening before too |
| 13:46:00 | desowin | 99 not so much, as that's just 2 * 24 KiB buffer + 3 |
| 13:46:13 | Arsen | yeah, but it's a bit oddly specific |
| 13:46:26 | desowin | the 3 sounds like what the chipset might be able to acknowledge "on its own" before driver has to read it |
| 13:47:06 | desowin | atleast that's a guess from someone who has dealt with USB in general, but not particularly this chip |
| 13:48:39 | Arsen | I'll enable ramdisk too |
| 14:00 |
| 14:01:44 | *** | Saving seen data "./dancer.seen" |
| 14:03:49 | Arsen | desowin: I don't think I can reproduce it writing 8MiB per attempt to the ramdisk |
| 14:04:09 | Arsen | that's also significantly less than the failing writes |
| 14:04:25 | Arsen | also, if this is due to the dwc driver, why does it not happen on reads? |
| 14:05:48 | Arsen | wait wrong controller |
| 14:11:37 | Arsen | while :; do yes AAAAAAAAAAAAAA | doas dd of=/dev/sde; done I'm doing this to test |
| 14:13:48 | desowin | I wouldn't reject the possibility of it being in dwc driver just on that, but yes, it can be storage |
| 14:15:08 | desowin | if you try to find the problem not where it is, then the debugging takes really long time |
| 14:15:55 | desowin | but here, I would suspect both, atleast not until I am perfectly sure that one is perfectly fine and not affecting the other at all |
| 14:18:38 | desowin | speachy: should we register for Coverity Scan? in my experience it gives really good results |
| 14:20:36 | desowin | as far as compilation goes you just run make through cov-build and then upload compressed cov-int directory |
| 14:23:26 | Arsen | well, I have dismissed storage before because 1) it works fine on other controller (in fact, I synced my entire library onto it, but on this controller it fails with one flac) and 2) it works fine if I just copy files in rockbox' file manager |
| 14:24:24 | desowin | speachy: we might not be able to get it for all builds due to limits https://scan.coverity.com/faq#frequency but it would be really nice to get it going |
| 14:24:26 | Arsen | when I say controller, I usually mean USB host controller |
| 14:28:13 | speachy | desowin: I've not been motivated to deal with the tooling changes; IIRC they don't do so well with cross-complied crap. though of course the code that makes it into a sim build would be a pretty good subset of our codebase. |
| 14:29:48 | desowin | I got good results with cross compiled code for Nintendo switch |
| 14:29:51 | speachy | (it's not as simple as overriding CC/LD/etc. I've done a lot to detangle things (as part of the gcc494 transition) but plenty of work remains |
| 14:30:52 | Arsen | wtf, echo test | dd of=/dev/sde causes it to requery the serial code and such |
| 14:30:58 | desowin | speachy: I would say it is comparably simple to overriding CC, the problem is with the upload frequency |
| 14:30:59 | Arsen | (rerequest descriptors in general) |
| 14:31:33 | speachy | (no, I mean we can't just override CC and have it proprogate everywhere successfully yet) |
| 14:31:42 | speachy | but we're at ~2.1M LOC. |
| 14:32:01 | braewoods | it might help if we start dropping dead ports no one has touched in ages |
| 14:32:08 | speachy | not really. |
| 14:32:14 | braewoods | oh? |
| 14:32:15 | speachy | we'd be limited to 1 build per day, period |
| 14:32:27 | speachy | unless we pay extra, granted |
| 14:32:31 | braewoods | oh you mean coverity |
| 14:32:44 | desowin | speachy: if you don't mind, I can request account and upload Sansa Connect build there to check it out |
| 14:33:04 | speachy | most of the code is not in ports, but in plugins/library code. |
| 14:33:21 | desowin | that's shared between all targets, so good |
| 14:33:34 | speachy | desowin: Not that you need permission to do that, but by all means, please do |
| 14:34:08 | desowin | Arsen: does it actually trigger USB reset? why would it rerequest descriptors otherwise? |
| 14:34:41 | Arsen | no, no reset happens, and it keeps normally sending Test Unit Ready commands |
| 14:35:04 | Arsen | wait, /dev/sde isn't a device |
| 14:35:09 | desowin | basically, device can only reply to host, there's no way any code on device would result in host rereading descriptors |
| 14:35:17 | speachy | well, I suppose if you have to "officially" register it that's another matter. |
| 14:35:33 | desowin | I would only think about resets and other recovery actions performed by host |
| 14:35:55 | Arsen | /dev/sde wasn't even a node in devfs, I unplugged it and plugged it back in, it is now |
| 14:36:01 | desowin | you have to be contributor, which I fulfill |
| 14:36:35 | speachy | desowin: the reason I think we should stick to simulator builds for the first cut is that it uses stock modern toolchains |
| 14:37:04 | speachy | ie no fancy linker scripts, asm startup code, or so forth. |
| 14:37:32 | speachy | and, heh, I'm 99% sure that will end up with several hundred (or so) issues in the codecs alone. :) |
| 14:37:35 | | Join saratoga [0] (~saratoga@cpe-98-10-205-66.rochester.res.rr.com) |
| 14:38:19 | saratoga | Arsen: I never looked at it, but I think progressive jpeg could be supported by only decoding one layer of the progressive image and then rescaling that |
| 14:38:33 | desowin | yes, it will, Wireshark has a lot of outstanding issues reported by Coverity Scan but it helps detecting problems in new code pretty nicely |
| 14:38:34 | braewoods | woops. i had my shift register's bits reversed :D |
| 14:38:38 | saratoga | but i'm not sure how easy that would be to do, i didn't look at how they're packed in progressive mode |
| 14:38:38 | Arsen | saratoga: that's what I was thinking as an implementation too fwiw |
| 14:38:53 | Arsen | but according to the logs, that's also impossible due to the MCU being huge |
| 14:39:24 | Arsen | as in, you'd need the whole image decoded to start scaling it, and that would take up a lot of mem |
| 14:39:48 | Arsen | it may be possible on some more modern devices, this ipod has 64M of RAM, I have no idea whether that's enough until I try, though |
| 14:39:50 | speachy | saratoga: I think to handle progressive we'd have to not scale as we decompress because each iteration builds on the previous. I don't know if we can scale on the fly any more. |
| 14:40:03 | Arsen | ^ you couldn't |
| 14:40:21 | Arsen | which is why I was thinking of just giving up at reading further at some point |
| 14:40:59 | saratoga | i think he means decoding it at full resolution |
| 14:41:05 | Arsen | yes |
| 14:41:51 | saratoga | i think at least you could decode the lowest resolution step (1/8th) efficiently |
| 14:42:01 | saratoga | although it would only be useful for large images |
| 14:42:53 | saratoga | probably not worth it for such niche use |
| 14:44:16 | desowin | ok, registered Rockbox on Coverity Scan, will submit build soonish |
| 14:44:59 | speachy | desowin: another option is to enable support for clang's scan-analyze builds. |
| 14:46:01 | desowin | I wouldn't compare these two, Coverity Scan is just another league IMHO |
| 14:46:24 | Arsen | what does coveritry do for rockbox? |
| 14:46:27 | speachy | yes, but it has the advantage of not requiring 3rd party integration and access/service limitations. |
| 14:46:32 | braewoods | Arsen: static code analysis |
| 14:46:38 | Arsen | ah, I see |
| 14:46:38 | braewoods | it can find issues that compilers miss |
| 14:46:42 | Arsen | clang-tidy? |
| 14:46:48 | speachy | I've used clang's stuff with great effect for other projects of mine |
| 14:47:11 | desowin | I got USBPcap blue screen of death root cause pointed out by Coverity Scan |
| 14:47:13 | speachy | (and speaking of coverity, at my last $dayjob we had a _lot_ of false positives too) |
| 14:47:19 | desowin | that I wouldn't otherwise have found |
| 14:47:47 | bertrik | I use cppcheck for a quick check a lot |
| 14:48:25 | bertrik | it doesn't do a very deep analysis, but can quickly find the obvious stuff with barely any configuration |
| 14:48:59 | desowin | at work I have been involved in projects using Klocwork, it was funny that people suspected that there's something wrong with the tool as my code had just 2 issues found |
| 14:49:08 | Arsen | alright, I've got some python going to spam the ramdisk with 8MB of "B" characters |
| 14:49:10 | speachy | I've had some success with cppcheck too, but it's pretty superficial. Doesn't do much better than simply building with all warnings turned on. |
| 14:49:35 | desowin | but well, it wasn't anything big, just simple measurements and valve control |
| 14:50:16 | speachy | desowin: heh, yeah, I'm always suspicious when I see initial runs of sanity checks turn up no problems. :D |
| 14:50:50 | bertrik | code at work is sometimes very well tested (high coverage, etc) but we forget to see if it does what it's supposed to do for the customer |
| 14:51:22 | braewoods | speachy: i've introduced myself to two different constructs i've never needed before i worked on inflate... a circular output buffer and a bit reader layered on top of a byte stream |
| 14:51:23 | speachy | (BTW I still have a scan-build going on the X3 simulator. mostly dead stores, a few divide-by-zeros so far) |
| 14:51:28 | braewoods | kinda interesting |
| 14:51:46 | Arsen | desowin: yeah, spamming it with writes isn't doing anything, I think |
| 14:55:33 | speachy | lots of thjings flagged in the codecs so far, mostly due to shifts. |
| 14:58:10 | Arsen | speachy: I have figured out why there aren't more people complaining: this *only* happens with *one* USB controller, and not even all ports on it, and only while writing to the disk from the usbms driver |
| 14:58:57 | speachy | I assume the physical ports can be plugged into a different set of motherboard headers? |
| 14:59:14 | Arsen | it's not the controller and the cable to it or to the ipod, though, because the same cables work fine if any of those variables is different (using a ramdisk, or for reading) |
| 14:59:37 | desowin | well, personally I would only blame it on the controller if I actually checked the "real" USB packets |
| 14:59:57 | Arsen | yeah, if I get together enough money for that sniffer I might get it |
| 14:59:58 | speachy | I mean, it still reeks of a phyiscal hardware issue (eg bad port/cable to the motherboard) |
| 15:00 |
| 15:00:06 | desowin | there might be some subtle out-of-bounds write that triggers only in this condition |
| 15:00:32 | speachy | but without seeing true wire-level sniffs (ie showing CRCs) |
| 15:00:53 | desowin | with timing just right in case of this particular controller |
| 15:00:54 | Arsen | at this point I am more curious about what's wrong here than actually caring to fix it in order to use it, because changing any of those variables makes it "work", so I might put together some money to buy a sniffer |
| 15:01:01 | speachy | logically the packets are identical; if port A vs port B of the same controller behaves differently. |
| 15:10:23 | speachy | ok, in the X3 sim build, scan-build found 358 bugs. |
| 15:10:55 | bertrik | nice |
| 15:11:06 | speachy | 159 of those are dead stores. |
| 15:12:21 | speachy | 55 nullptr derefs, 14 div0.. and a bunch of uninitialized/undefined values used where it matters. |
| 15:14:02 | desowin | I go with Sansa Connect target build as first Coverity Scan submission due to selfish reasons - just curious if I have some strange problems in drivers |
| 15:14:47 | desowin | no idea really how many issues there will be |
| 15:16:00 | desowin | but it gets all the codecs and plugins so the results will be to great degree valid for other targets (possibly to all, not only armv4) |
| 15:16:51 | speachy | some of those are data-depenent; eg impossible to trigger unless we have no language files. |
| 15:17:06 | speachy | (...) |
| 15:17:57 | braewoods | incidently null pointer dereference isn't necessarily an error |
| 15:18:10 | speachy | (but it almost always is) |
| 15:18:23 | braewoods | indeed, one place you could ignore it if it pops up is iriver_flash |
| 15:18:30 | braewoods | it's going to be a false positive there |
| 15:18:48 | speachy | because address 0 is a valid address (which is actually a violation of the C standard) |
| 15:19:11 | braewoods | yes, but the real world doesn't always care what the C standard says. :| |
| 15:19:25 | speachy | lots of nullptr issues int he skin code |
| 15:19:33 | braewoods | technically you're not supposed to assume char is 8 bits yet people do it anyway |
| 15:20:12 | braewoods | but in practice only some esoteric legacy systems ever had a different bit value assigned to char |
| 15:20:18 | Arsen | oh, this looks interesting "[366912.197535] sd 11:0:0:0: [sdf] Synchronize Cache(10) failed: Result: hostbyte=DID_NO_CONNECT driverbyte=DRIVER_OK" |
| 15:20:22 | Arsen | when did this happen |
| 15:21:28 | Arsen | 11 minutes ago, that was after I stopped spamming it with writes, that's probably not related to this issue then |
| 15:22:51 | speachy | the '-1' in this: #define TALK_ID(n,u) (((long)(u))<<UNIT_SHIFT | ((n) & ~(-1L<<DECIMAL_SHIFT))) |
| 15:23:01 | speachy | is responsible for a substantial portion of the errors. |
| 15:23:15 | speachy | since left-shifting a negative number is undefined. |
| 15:23:48 | speachy | but I suppose casting that -1 to (unsigned int) will probably shut things up |
| 15:24:26 | braewoods | speachy: add a U? |
| 15:24:37 | braewoods | or will that even matter here |
| 15:24:39 | speachy | -1UL is a contradiction. :) |
| 15:24:46 | braewoods | even so it may still work |
| 15:24:59 | braewoods | i rarely need to use the U suffix though |
| 15:25:19 | braewoods | most of the time 'int' default integer constant is enough as it gets promoted to the proper type |
| 15:25:28 | braewoods | or demoted |
| 15:26:11 | speachy | we're relying on a specific hardware numeric representation (ie 2's complement) that C doesn't promise. |
| 15:26:18 | speachy | hence the warning |
| 15:26:19 | braewoods | huh, IS_ALIGNED is one weird macro. i assumed it would cast pointer to an integer |
| 15:26:27 | braewoods | but no such luck |
| 15:26:39 | braewoods | since the main use for it is checking pointer alignment |
| 15:29:33 | speachy | ok, fixing that knocked out 12 "errors" |
| 15:33:21 | desowin | anyone who wants to see Rockbox Coverity Scan project, please let me know their email so I can send invite |
| 15:33:30 | speachy | send one to me, please |
| 15:33:47 | desowin | the results will be available only after the application is reviewed (1-2 business days) |
| 15:35:37 | Arsen | I'd like it too |
| 15:35:44 | Arsen | out of curiosity, not necessity, if it's no issue |
| 15:36:08 | desowin | Arsen: pm me your email |
| 15:36:33 | desowin | speachy: invitation should be sent, but I can't see it in members page, so not sure |
| 15:36:48 | speachy | thank you |
| 15:38:48 | desowin | ok, you have to press space after parting in the email and click on the "add email" button |
| 15:39:00 | desowin | as otherwise it was just submitting empty list |
| 15:39:21 | desowin | now the invitations are visible as "not yet accepted" |
| 15:40:36 | braewoods | g#3590 |
| 15:40:38 | rb-bluebot | Gerrit review #3590 at https://gerrit.rockbox.org/r/c/rockbox/+/3590 : system: revamp IS_ALIGNED macro by James Buren |
| 15:40:57 | braewoods | \o/ |
| 15:41:51 | speachy | was it really only used in one file? |
| 15:42:05 | braewoods | yea, oddly enough. |
| 15:42:16 | braewoods | the only other hit is miknod files which has its own macros instead |
| 15:42:20 | braewoods | mikmod |
| 15:42:33 | | Join Maxdamantus [0] (~Maxdamant@user/maxdamantus) |
| 15:42:45 | braewoods | https://dpaste.com/A7YPBYM7W |
| 15:43:20 | braewoods | i'd have preferred an inline function but i figured i should keep the macro solution. |
| 15:44:55 | braewoods | speachy: if you're ok with that though i could replace it with an inline macro. in this case the types don't really matter |
| 15:45:08 | braewoods | err |
| 15:45:10 | braewoods | inline function |
| 15:45:12 | braewoods | doh |
| 15:45:28 | braewoods | i mainly like inline functions because they're usually easier to understand than macros |
| 15:47:14 | speachy | as a macro is fine |
| 15:47:31 | braewoods | ok |
| 15:47:49 | speachy | the overhead of calling a function will probably always be greater than just inlining it anyway. |
| 15:48:10 | braewoods | i just prefer inline functions as much as possible for simple stuff |
| 15:48:32 | braewoods | they benefit from being easier to debug and the like |
| 15:48:48 | braewoods | the only disadvantage is they can't workaround syntax limitations like macros can |
| 15:48:52 | braewoods | that i can think of |
| 15:49:07 | braewoods | sometimes i pair them together |
| 15:49:17 | braewoods | inline function for the read code, macro is a wrapper |
| 15:49:20 | braewoods | real* |
| 15:50:12 | braewoods | i've also sometimes done hacks with extern inline |
| 15:50:44 | braewoods | it's a weird duck. if you use it correctly you can have both an inline version and an external version so the compiler can choose both and you won't have a million copies of the static version |
| 15:52:32 | | Quit Maxdamantus (Ping timeout: 252 seconds) |
| 15:54:15 | braewoods | incidently we can now use _Alignof thanks to newer GCC |
| 15:57:01 | speachy | _bilgus: here's an interesting one. apps/gui/skin_engine/skin_render.c L324 |
| 15:57:45 | speachy | the call uses info->skin_vp instead of the skin_vp passed in as part of the function. |
| 15:59:02 | speachy | everyelse in the function the function-arg-supplied one is used. |
| 16:00 |
| 16:01:45 | *** | Saving seen data "./dancer.seen" |
| 16:05:48 | braewoods | speachy: do we have any 64 bit ports other than some kind of hosted one? |
| 16:05:54 | braewoods | i'm assuming no |
| 16:06:07 | braewoods | i'm guessing even the x1000 isn't 64 bit |
| 16:06:25 | speachy | most sim builds are 64-bit these days |
| 16:06:39 | braewoods | indeed but in terms of native or hosted... |
| 16:07:09 | braewoods | if we had a 64 bit native, we could look into 64 bit optimizations but for now not much point |
| 16:07:17 | speachy | braewoods: btw, #3590, the old macro allows for varying degrees of alignment checking depending on the cast used. the revamped only checks for pointer-sized alignment. |
| 16:07:47 | braewoods | speachy: that was the whole idea? what else needs to be checked for alignment? |
| 16:08:02 | speachy | it's a generic macro, so could be used for arbitrary alignment checks. |
| 16:08:22 | speachy | (eg dma that might need cacheline-aligned stuff for example) |
| 16:08:47 | braewoods | i have an idea. gimme a minute to research it. |
| 16:09:01 | speachy | (or arm processors that need the irq vector table to be 128-byte aligned or somesuch) |
| 16:09:49 | speachy | now IMO the right thing to do is make the type/size you want to align to an argument to the macro |
| 16:10:18 | speachy | explicitly instead of implicitly. IIRC Linux's alignment check macros work this way. |
| 16:16:07 | braewoods | of course _Generic doesn't work like this... |
| 16:16:24 | braewoods | i wish i could use it as a general catch all for pointers |
| 16:16:53 | braewoods | speachy: in any case, what different does it make? pointer address integers are just as valid as any other, no? |
| 16:17:19 | braewoods | though i guess in terms of semantics |
| 16:17:29 | braewoods | they are different even if they are usually the same |
| 16:17:35 | speachy | as in that new macro only works for pointers, the old one worked with whatever. |
| 16:17:59 | braewoods | it cast to uintptr_t, which should work for regular integers too |
| 16:18:14 | speachy | I'm talking about things larger than a pointer |
| 16:18:21 | speachy | or smaller |
| 16:18:30 | braewoods | larger is a problem, but smaller? |
| 16:18:43 | braewoods | smaller gets promoted and should retains its value |
| 16:19:25 | braewoods | but afaik for the most part sizeof(void*) == sizeof(largest integer type) |
| 16:19:28 | speachy | call the macro IS_ALIGNED_PTR() to reflect what it really does |
| 16:19:58 | speachy | because you're changing its semantics. |
| 16:20:40 | speachy | what I'd like to see instead is a reduction of the various alighment checks/macros all over the codebase. |
| 16:46:20 | | Quit saratoga (Quit: Connection closed) |
| 17:00 |
| 17:01:48 | | Quit lebellium (Quit: Leaving) |
| 18:00 |
| 18:01:49 | *** | Saving seen data "./dancer.seen" |
| 18:10:51 | | Quit ZincAlloy (Quit: Leaving.) |
| 18:11:56 | | Join ZincAlloy [0] (~Adium@ip5f5abcae.dynamic.kabel-deutschland.de) |
| 18:18:24 | | Quit ZincAlloy (Quit: Leaving.) |
| 20:00 |
| 20:01:50 | *** | Saving seen data "./dancer.seen" |
| 20:04:39 | _bilgus | speachy, ya got me on that no clue.. |
| 20:05:16 | speachy | I won't pretend to understand the semantics of the menu code |
| 20:05:57 | _bilgus | I'm guessing its something v. specific bc I don't recognize the flag.. |
| 20:05:59 | speachy | But g#3591 does seem to be the correct thing. |
| 20:06:01 | rb-bluebot | Gerrit review #3591 at https://gerrit.rockbox.org/r/c/rockbox/+/3591 : menu: Fix a potential nullptr deference. by Solomon Peachy |
| 20:06:15 | speachy | (a different unrelated thing) |
| 20:06:52 | _bilgus | ah lol |
| 20:09:23 | _bilgus | temp is referenced before that.. |
| 20:09:54 | _bilgus | ah but checked −− nm |
| 20:10:07 | rb-bluebot | Build Server message: New build round started. Revision df37450f91, 303 builds, 8 clients. |
| 20:12:00 | speachy | there's a second potential nullptr around L599, temp can be null before the switch and lead to much dereferencing. |
| 20:12:06 | speachy | in menu.c |
| 20:12:28 | _bilgus | 495? |
| 20:14:31 | speachy | starting at L587, if (in_stringlist) {} else if (temp) {} and then switch(type) { ... } where every case dereferences temp. |
| 20:15:06 | speachy | might only be triggerable if the menu structure is mis-defined |
| 20:15:20 | speachy | not familiar with this stuff to know its subtelties. |
| 20:18:23 | _bilgus | that one is guarded further up IIRC |
| 20:18:43 | _bilgus | I think it was an oversight originally |
| 20:19:01 | _bilgus | but L496 const struct settings_list *setting = |
| 20:19:01 | _bilgus | find_setting(temp->variable, NULL); |
| 20:19:30 | speachy | L496 doesn't kick in if type == MT_MENU |
| 20:19:32 | _bilgus | I don't see anything checking for validity but it might too be guarded] |
| 20:20:20 | speachy | L476, actually |
| 20:21:10 | _bilgus | huh how does that work?? |
| 20:21:19 | speachy | anyway, that's a different issue. what I'm referring to is L575 on |
| 20:21:44 | speachy | temp is assigned in there, only if type == MT_MENU, if it's not, then it's not guaranteed to be assigned |
| 20:22:34 | speachy | so the MT_FUNCTION_CALL case dereferences default-assigned temp, ie NULL |
| 20:22:54 | speachy | when action == ACTION_STD_OK |
| 20:23:10 | rb-bluebot | Build Server message: Build round completed after 782 seconds. |
| 20:23:13 | rb-bluebot | Build Server message: Revision df37450f91 result: All green |
| 20:23:42 | speachy | it's possible that it's semantically impossible to call STD_OK if it's not a type MT_MENU |
| 20:24:16 | _bilgus | seems unlikely |
| 20:24:35 | speachy | not something I'd care to put money on either. :) |
| 20:27:01 | speachy | I'll put up the results of this scan-build pass and you can interactively see for yourself. |
| 20:31:00 | speachy | _bilgus: https://www.shaftnet.org/~pizza/scan-build-202453/index.html |
| 20:35:18 | _bilgus | I see the one at L184 and a dead assignment |
| 20:40:56 | | Join dconrad [0] (~dconrad@208.38.228.17) |
| 20:41:32 | _bilgus | I guess this encourages you to add pointer null checks but most of these are likely benign |
| 20:47:51 | speachy | yeah. but there's a good chance there's some real bugs in there amongst the noise. |
| 20:50:20 | _bilgus | indeed |
| 20:51:09 | _bilgus | this one likely is https://www.shaftnet.org/~pizza/scan-build-202453/report-88af48.html#EndPath |
| 20:52:06 | braewoods | oh man i hate parsers sometimes. |
| 20:52:12 | braewoods | they seem like the ones most prone to bugs |
| 20:52:28 | braewoods | sometimes i wonder if a parser generator would be a better idea |
| 20:53:08 | speachy | braewoods: they usually are. |
| 20:54:46 | _bilgus | https://www.shaftnet.org/~pizza/scan-build-202453/report-f9a6b6.html#EndPath |
| 20:54:58 | _bilgus | that one is real |
| 20:54:58 | braewoods | would it even be worth replacing the theme/skin parser? |
| 20:55:00 | braewoods | at this point |
| 20:55:13 | braewoods | i once dabbled in parsers |
| 20:55:44 | braewoods | i'll look at the parser sometime and see what its hard requirements are |
| 20:55:56 | braewoods | the grammar determines what kind of generators are practical |
| 20:56:01 | speachy | _bilgus: yes but only if the language file is toast. |
| 20:56:39 | speachy | ie num_clips == 0 |
| 20:57:54 | _bilgus | if (!size) continue; |
| 20:58:22 | speachy | I suppose the correct thing to do is, after the loop is finished, bail immediately if real_clips is 0. |
| 20:58:30 | braewoods | depending on requirements, i've usually used lemon for my grammar side. |
| 20:58:46 | braewoods | re2c is also a decent choice |
| 20:58:50 | braewoods | for lexer |
| 20:59:58 | braewoods | one reason not to use bison... it's non-reentrant last i heard |
| 21:00 |
| 21:00:15 | braewoods | a reentrant parser combo is probably a good idea these days |
| 21:00:54 | speachy | braewoods: first you need a formal grammar, which IIRC doesn't actually exist. |
| 21:16:09 | braewoods | yea, |
| 21:16:13 | braewoods | i'd have to look |
| 21:26:25 | | Join Maxdamantus [0] (~Maxdamant@user/maxdamantus) |
| 21:30:48 | _bilgus | !g3593 |
| 21:31:18 | _bilgus | g#3593 |
| 21:31:20 | rb-bluebot | Gerrit review #3593 at https://gerrit.rockbox.org/r/c/rockbox/+/3593 : talk.c check for 0 talk clips & announce_status fix typo by William Wilgus |
| 21:33:17 | speachy | yay, first actual bug squashed. |
| 21:33:41 | speachy | (required malformed input files, but hey, it's at least possible..) |
| 21:34:40 | _bilgus | too bad I neglected a parens and introduced a far worse bug |
| 21:34:44 | _bilgus | :p |
| 21:35:54 | braewoods | _bilgus: it attained sentience? |
| 21:36:49 | _bilgus | if it were that easy.. |
| 21:36:58 | speachy | is it really a new bug if it wouldn't even compile? |
| 21:42:47 | _bilgus | https://www.shaftnet.org/~pizza/scan-build-202453/report-2f91fc.html#EndPath |
| 21:43:11 | _bilgus | that one is a bug but I can't tell if its a real bug |
| 21:47:43 | speachy | I think it doesn't matter what exp is initialized to... though 1 seems the logical default exponent |
| 22:00 |
| 22:01:51 | *** | No seen item changed, no save performed. |
| 22:08:17 | | Quit dconrad (Read error: Connection reset by peer) |
| 22:08:49 | | Join dconrad [0] (~dconrad@208.38.228.17) |
| 22:22:42 | | Quit mixfix41 (Ping timeout: 240 seconds) |
| 22:31:51 | | Quit cockroach (Quit: leaving) |
| 22:50:38 | munkis | huh that's a strange construction |
| 22:56:00 | munkis | The only reason I can think of to do that is to avoid compiler warnings while intentionally not initializing |
| 23:00 |
| 23:23:28 | | Quit dconrad () |
| 23:25:15 | _bilgus | whats that save 4 bytes? |
| 23:49:45 | _bilgus | is this really a bug? https://www.shaftnet.org/~pizza/scan-build-202453/report-77fc0f.html#EndPath |
| 23:50:44 | _bilgus | thoughts opinions diatribes? |
icon identifies that the person is a core developer (has commit access).