Previous day | Jump to hour: 01 02 03 04 05 06 07 08 09 10 11 12 13 14 15 16 17 18 19 20 21 22 23 | Next day

Seconds: Show Hide | Joins: Show Hide | View raw
Font: Serif Sans-Serif Monospace | Size: Small Medium Large

Click in the nick column to highlight everything a person has said.
The Logo icon identifies that the person is a core developer (has commit access).

#rockbox log for 2021-07-19

00:01:27***Saving seen data "./dancer.seen"
01:46:34 Join Maxdamantus [0] (~Maxdamant@user/maxdamantus)
01:50:24Arsenspeachy: 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:05Arsenfron 3.0 ports fail after a while, resetting, 2.0 ones never figure out configuration
02:01:28***No seen item changed, no save performed.
02:50:26 Quit Maxdamantus (Ping timeout: 256 seconds)
02:51:46desowinArsen: can you share pcap? do you have OpenVizsla or similar?
02:52:39Arsenno I'm not usually working on this part of the kernel
02:53:24ArsenI can share the usbmon capture thon
02:53:41Arsenafter i sanitize it to see whether any unrelated and potentially sensitive data is there
03:05:36desowinyou mean actual data stored on the drive or other devices?
03:06:32desowinif 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] (
03:33:28Arsenboth really
04:01:32***Saving seen data "./dancer.seen"
05:43:34Arsendid 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:26gevaertsI 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:50gevaertsOur 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:01Arsenafaik 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:12gevaertsI have no idea :)
05:54:13gevaertsI'll find the logs where he explained it
05:56:16gevaerts and
05:56:54gevaertsNote that I don't really understand most of that
05:57:31gevaertsAll I can do is find links :)
05:58:24Arsenah, this makes sense
05:59:51Arsendesowin: 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?
06:01:35***No seen item changed, no save performed.
06:05:17Arsendesowin: if you don't mind it being on my own webserver, here you go:
06:31:21 Quit akaWolf (Ping timeout: 258 seconds)
06:46:25braewoodsArsen: you want assistance submitting a quirks patch?
06:46:51braewoodsi've done this before but i'd need to know what the quirk is you're needing patched
06:47:13Arsenyeah, I don't yet, I'll investigate further
06:47:23Arsenif you have notes/docs about the process, I'm all ears though
06:47:36braewoodsdepends, i've mainly done audio quirks.
06:47:42braewoodsand depends where it's required.
06:48:03braewoodsif it requires patching the kernel, you'll need to add hardware IDs to the in kernel driver table.
06:48:10braewoodsand maybe some other bits
06:48:46Arsenprobably the USB driver
06:48:52braewoodsif it's keyboard related that goes in the hardware database shipped with systemd
07:15:09 Join akaWolf [0] (
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: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:41Arsenbraewoods: no, it's most likely either usbms or xhci_hid in the kernel
09:04:23 Quit F3l1x_10m (Read error: Connection reset by peer)
09:07:22 Join massiveH [0] (
09:37:46 Quit Maxdamantus (Ping timeout: 268 seconds)
09:44:14braewoodsthis is a warning i've never seen before
09:44:17braewoodswarning: comparison of promoted ~unsigned with unsigned
09:44:28desowinArsen: 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:48desowindo you have corresponding logf?
09:46:05braewoodsoh well, converted it to xor equivalent
09:46:07desowinif you enable ramdisk you can try to determine if the iflash has any impact on it
09:46:30ArsenI don't think I have it, lemme look through some logs
09:46:44desowinas ramdisk will simply present a chunk of ram as the disk to the host
09:46:53Arsenand it almost certainly doesn't at this point, since a different computer works
09:47:29Arsendifferent controllers in this PC even work
09:47:39desowinthe port reset is just a consequence of OUT URB timing out
09:47:45Arsennope, lost the logs, rip
09:47:47Arsenyeah, thought so
09:48:16desowinso there's either a signal integrity issue (hardware packet level capture would tell if that's the case) or something in software
09:48:29braewoodsa little closer to finishing inflate support
09:48:37braewoodsnext i need to add the huffman engine
09:49:17Arsen`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:42Arsenin usb_storage.c
09:49:47ArsenI'm not sure I saw the write10 at the end
09:49:57ArsenI'll get another debug build
09:50:43Arsendesowin: how do you know it didn't manage to send the full payload?
09:51:02desowinit didn't manage to send the full payload
09:51:24desowinit got 99 packets (out of 240) through
09:51:27Arsenyes, but what indicates that?
09:51:38Arsenoh, where are those numbers from?
09:51:56desowinURB length in the "packet" when the URB returns from host controller
09:52:08Arsenthe ECONNRESET one?
09:53:55Arsenwhat files should I put the logfs into?
10:01:37desowinfirmware/drivers/usb-designware.c and firmware/usbstack/usb_storage.c
10:01:39***Saving seen data "./dancer.seen"
10:01:53Arsenpresumably USB serial logging won't work for this? :^)
10:02:24desowinyou can have it compiled in, but don't enable to not distort the result
10:02:35desowinyou can simply dump log to file
10:02:42Arsenoh, right, that works
10:02:47Arsenafter I fix the FS that is :P
10:03:13desowinFS meaning file system or full speed USB?
10:06:07desowincapture pcap as well to have the logf and pcap from same session
10:06:22Arsensure, I'll just transfer on the debug build now
10:06:56Arsensorry 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:19:26Arsenright, I'm back, sorry about that
11:43:12Arsenalright, I can see logs, lets see what happens now
11:47:04Arsendesowin: got the logs and stuff
11:49:41Arsenthis buffer is tiny
11:49:47ArsenI'm not sure if I have the right bit there
11:49:55Arsenmost likely not, I'll redo this
11:50:09Arsenissue is, I need(?) to fsck before the dump
11:50:19ArsenI'll try again since I can't corelate it myself either
11:55:07Arsenok I can corelate it this time
11:55:21Arsendesowin: .tar of the two on the same place works for you?
12:01:43***Saving seen data "./dancer.seen"
12:32:10 Join spork [0] (
12:38:51 Join ZincAlloy [0] (
12:42:56desowinI 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:21desowindepending 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:32desowingetting 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: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:13Arsendesowin: you saw that in the logf?
13:25:22ArsenI'd like to debug this honestly, I've got the time right now
13:26:27ArsenI don't see the three packets though
13:29:20desowinURB 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:50desowinhost can only write in multiples of maximum packet size, unless the packet ends transfer (1 URB = 1 transfer)
13:31:25desowinthe device acknowledged these three packets, and stored them *somewhere*
13:32:39desowinnext 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:55Arsenafter the NAK the timeout happens?
13:33:30desowinthere are thousands of NAKs
13:33:56desowinthe ECONNRESET "packet" in Wireshark is 30 seconds after the URB submit
13:34:05desowinso host was trying to send the data for 30 seconds
13:34:16Arsenand getting nak'd the entire time?
13:34:38Arsenso, usbmon actually logs requests for something to happen on usb, not what's actually happening on usb?
13:34:48desowinnot entire time, it got ACKed 99 times (50688 / 512)
13:34:49Arsen(and their results)
13:35:48desowinyes, usbmon logs requests for something to happen on usb bus
13:36:00Arsenright, that makes a lot of sense
13:36:18desowin this is recording of my SharkFest '20 Virtual talk on USB
13:36:46Arsenhow much is that sniffer you mentioned earlier?
13:36:50Arsenas a side note
13:36:57Arsen(the cost of acquiring it)
13:37:55desowinexplains what usbmon captures and how it differs from the packets on the bus, nowadays if you get the usbll capture will be actually reassembled
13:38:07desowinfor sharkfest I just correlated the packets manually
13:38:16ArsenI could pull in usbll if it can help here
13:38:36Arsenthis patch*
13:38:37Arsennot usbll
13:39:06desowinthe patch only affects usbll captures, so no change for usbmon capture
13:39:16Arsenah, and that requires some device?
13:39:33desowin has assembled OpenVizsla for 154.70 EUR + shipping
13:39:54desowinbut mind you, just getting usbll capture is only a beginning of actual troubleshooting
13:40:39desowinbut it can for sure keep you from wandering entirely wrong direction as it actually shows what happens on the bus
13:45:37Arsenthat number is oddly specific, though
13:45:43Arsen99, and I think that was happening before too
13:46:00desowin99 not so much, as that's just 2 * 24 KiB buffer + 3
13:46:13Arsenyeah, but it's a bit oddly specific
13:46:26desowinthe 3 sounds like what the chipset might be able to acknowledge "on its own" before driver has to read it
13:47:06desowinatleast that's a guess from someone who has dealt with USB in general, but not particularly this chip
13:48:39ArsenI'll enable ramdisk too
14:01:44***Saving seen data "./dancer.seen"
14:03:49Arsendesowin: I don't think I can reproduce it writing 8MiB per attempt to the ramdisk
14:04:09Arsenthat's also significantly less than the failing writes
14:04:25Arsenalso, if this is due to the dwc driver, why does it not happen on reads?
14:05:48Arsenwait wrong controller
14:11:37Arsenwhile :; do yes AAAAAAAAAAAAAA | doas dd of=/dev/sde; done I'm doing this to test
14:13:48desowinI wouldn't reject the possibility of it being in dwc driver just on that, but yes, it can be storage
14:15:08desowinif you try to find the problem not where it is, then the debugging takes really long time
14:15:55desowinbut 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:38desowinspeachy: should we register for Coverity Scan? in my experience it gives really good results
14:20:36desowinas far as compilation goes you just run make through cov-build and then upload compressed cov-int directory
14:23:26Arsenwell, 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:24desowinspeachy: we might not be able to get it for all builds due to limits but it would be really nice to get it going
14:24:26Arsenwhen I say controller, I usually mean USB host controller
14:28:13speachydesowin: 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:48desowinI got good results with cross compiled code for Nintendo switch
14:29:51speachy(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:52Arsenwtf, echo test | dd of=/dev/sde causes it to requery the serial code and such
14:30:58desowinspeachy: I would say it is comparably simple to overriding CC, the problem is with the upload frequency
14:30:59Arsen(rerequest descriptors in general)
14:31:33speachy(no, I mean we can't just override CC and have it proprogate everywhere successfully yet)
14:31:42speachybut we're at ~2.1M LOC.
14:32:01braewoodsit might help if we start dropping dead ports no one has touched in ages
14:32:08speachynot really.
14:32:15speachywe'd be limited to 1 build per day, period
14:32:27speachyunless we pay extra, granted
14:32:31braewoodsoh you mean coverity
14:32:44desowinspeachy: if you don't mind, I can request account and upload Sansa Connect build there to check it out
14:33:04speachymost of the code is not in ports, but in plugins/library code.
14:33:21desowinthat's shared between all targets, so good
14:33:34speachydesowin: Not that you need permission to do that, but by all means, please do
14:34:08desowinArsen: does it actually trigger USB reset? why would it rerequest descriptors otherwise?
14:34:41Arsenno, no reset happens, and it keeps normally sending Test Unit Ready commands
14:35:04Arsenwait, /dev/sde isn't a device
14:35:09desowinbasically, device can only reply to host, there's no way any code on device would result in host rereading descriptors
14:35:17speachywell, I suppose if you have to "officially" register it that's another matter.
14:35:33desowinI would only think about resets and other recovery actions performed by host
14:35:55Arsen/dev/sde wasn't even a node in devfs, I unplugged it and plugged it back in, it is now
14:36:01desowinyou have to be contributor, which I fulfill
14:36:35speachydesowin: the reason I think we should stick to simulator builds for the first cut is that it uses stock modern toolchains
14:37:04speachyie no fancy linker scripts, asm startup code, or so forth.
14:37:32speachyand, 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] (
14:38:19saratogaArsen: 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:33desowinyes, 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:34braewoodswoops. i had my shift register's bits reversed :D
14:38:38saratogabut 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:38Arsensaratoga: that's what I was thinking as an implementation too fwiw
14:38:53Arsenbut according to the logs, that's also impossible due to the MCU being huge
14:39:24Arsenas in, you'd need the whole image decoded to start scaling it, and that would take up a lot of mem
14:39:48Arsenit 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:50speachysaratoga: 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:03Arsen^ you couldn't
14:40:21Arsenwhich is why I was thinking of just giving up at reading further at some point
14:40:59saratogai think he means decoding it at full resolution
14:41:51saratogai think at least you could decode the lowest resolution step (1/8th) efficiently
14:42:01saratogaalthough it would only be useful for large images
14:42:53saratogaprobably not worth it for such niche use
14:44:16desowinok, registered Rockbox on Coverity Scan, will submit build soonish
14:44:59speachydesowin: another option is to enable support for clang's scan-analyze builds.
14:46:01desowinI wouldn't compare these two, Coverity Scan is just another league IMHO
14:46:24Arsenwhat does coveritry do for rockbox?
14:46:27speachyyes, but it has the advantage of not requiring 3rd party integration and access/service limitations.
14:46:32braewoodsArsen: static code analysis
14:46:38Arsenah, I see
14:46:38braewoodsit can find issues that compilers miss
14:46:48speachyI've used clang's stuff with great effect for other projects of mine
14:47:11desowinI got USBPcap blue screen of death root cause pointed out by Coverity Scan
14:47:13speachy(and speaking of coverity, at my last $dayjob we had a _lot_ of false positives too)
14:47:19desowinthat I wouldn't otherwise have found
14:47:47bertrikI use cppcheck for a quick check a lot
14:48:25bertrikit doesn't do a very deep analysis, but can quickly find the obvious stuff with barely any configuration
14:48:59desowinat 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:08Arsenalright, I've got some python going to spam the ramdisk with 8MB of "B" characters
14:49:10speachyI'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:35desowinbut well, it wasn't anything big, just simple measurements and valve control
14:50:16speachydesowin: heh, yeah, I'm always suspicious when I see initial runs of sanity checks turn up no problems. :D
14:50:50bertrikcode 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:22braewoodsspeachy: 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:23speachy(BTW I still have a scan-build going on the X3 simulator. mostly dead stores, a few divide-by-zeros so far)
14:51:28braewoodskinda interesting
14:51:46Arsendesowin: yeah, spamming it with writes isn't doing anything, I think
14:55:33speachylots of thjings flagged in the codecs so far, mostly due to shifts.
14:58:10Arsenspeachy: 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:57speachyI assume the physical ports can be plugged into a different set of motherboard headers?
14:59:14Arsenit'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:37desowinwell, personally I would only blame it on the controller if I actually checked the "real" USB packets
14:59:57Arsenyeah, if I get together enough money for that sniffer I might get it
14:59:58speachyI mean, it still reeks of a phyiscal hardware issue (eg bad port/cable to the motherboard)
15:00:06desowinthere might be some subtle out-of-bounds write that triggers only in this condition
15:00:32speachybut without seeing true wire-level sniffs (ie showing CRCs)
15:00:53desowinwith timing just right in case of this particular controller
15:00:54Arsenat 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:01speachylogically the packets are identical; if port A vs port B of the same controller behaves differently.
15:10:23speachyok, in the X3 sim build, scan-build found 358 bugs.
15:11:06speachy159 of those are dead stores.
15:12:21speachy55 nullptr derefs, 14 div0.. and a bunch of uninitialized/undefined values used where it matters.
15:14:02desowinI 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:47desowinno idea really how many issues there will be
15:16:00desowinbut 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:51speachysome of those are data-depenent; eg impossible to trigger unless we have no language files.
15:17:57braewoodsincidently null pointer dereference isn't necessarily an error
15:18:10speachy(but it almost always is)
15:18:23braewoodsindeed, one place you could ignore it if it pops up is iriver_flash
15:18:30braewoodsit's going to be a false positive there
15:18:48speachybecause address 0 is a valid address (which is actually a violation of the C standard)
15:19:11braewoodsyes, but the real world doesn't always care what the C standard says. :|
15:19:25speachylots of nullptr issues int he skin code
15:19:33braewoodstechnically you're not supposed to assume char is 8 bits yet people do it anyway
15:20:12braewoodsbut in practice only some esoteric legacy systems ever had a different bit value assigned to char
15:20:18Arsenoh, 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:22Arsenwhen did this happen
15:21:28Arsen11 minutes ago, that was after I stopped spamming it with writes, that's probably not related to this issue then
15:22:51speachythe '-1' in this: #define TALK_ID(n,u) (((long)(u))<<UNIT_SHIFT | ((n) & ~(-1L<<DECIMAL_SHIFT)))
15:23:01speachyis responsible for a substantial portion of the errors.
15:23:15speachysince left-shifting a negative number is undefined.
15:23:48speachybut I suppose casting that -1 to (unsigned int) will probably shut things up
15:24:26braewoodsspeachy: add a U?
15:24:37braewoodsor will that even matter here
15:24:39speachy-1UL is a contradiction. :)
15:24:46braewoodseven so it may still work
15:24:59braewoodsi rarely need to use the U suffix though
15:25:19braewoodsmost of the time 'int' default integer constant is enough as it gets promoted to the proper type
15:25:28braewoodsor demoted
15:26:11speachywe're relying on a specific hardware numeric representation (ie 2's complement) that C doesn't promise.
15:26:18speachyhence the warning
15:26:19braewoodshuh, IS_ALIGNED is one weird macro. i assumed it would cast pointer to an integer
15:26:27braewoodsbut no such luck
15:26:39braewoodssince the main use for it is checking pointer alignment
15:29:33speachyok, fixing that knocked out 12 "errors"
15:33:21desowinanyone who wants to see Rockbox Coverity Scan project, please let me know their email so I can send invite
15:33:30speachysend one to me, please
15:33:47desowinthe results will be available only after the application is reviewed (1-2 business days)
15:35:37ArsenI'd like it too
15:35:44Arsenout of curiosity, not necessity, if it's no issue
15:36:08desowinArsen: pm me your email
15:36:33desowinspeachy: invitation should be sent, but I can't see it in members page, so not sure
15:36:48speachythank you
15:38:48desowinok, you have to press space after parting in the email and click on the "add email" button
15:39:00desowinas otherwise it was just submitting empty list
15:39:21desowinnow the invitations are visible as "not yet accepted"
15:40:36braewoods g#3590
15:40:38rb-bluebotGerrit review #3590 at : system: revamp IS_ALIGNED macro by James Buren
15:41:51speachywas it really only used in one file?
15:42:05braewoodsyea, oddly enough.
15:42:16braewoodsthe only other hit is miknod files which has its own macros instead
15:42:33 Join Maxdamantus [0] (~Maxdamant@user/maxdamantus)
15:43:20braewoodsi'd have preferred an inline function but i figured i should keep the macro solution.
15:44:55braewoodsspeachy: 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:10braewoodsinline function
15:45:28braewoodsi mainly like inline functions because they're usually easier to understand than macros
15:47:14speachyas a macro is fine
15:47:49speachythe overhead of calling a function will probably always be greater than just inlining it anyway.
15:48:10braewoodsi just prefer inline functions as much as possible for simple stuff
15:48:32braewoodsthey benefit from being easier to debug and the like
15:48:48braewoodsthe only disadvantage is they can't workaround syntax limitations like macros can
15:48:52braewoodsthat i can think of
15:49:07braewoodssometimes i pair them together
15:49:17braewoodsinline function for the read code, macro is a wrapper
15:50:12braewoodsi've also sometimes done hacks with extern inline
15:50:44braewoodsit'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:15braewoodsincidently we can now use _Alignof thanks to newer GCC
15:57:01speachy_bilgus: here's an interesting one. apps/gui/skin_engine/skin_render.c L324
15:57:45speachythe call uses info->skin_vp instead of the skin_vp passed in as part of the function.
15:59:02speachyeveryelse in the function the function-arg-supplied one is used.
16:01:45***Saving seen data "./dancer.seen"
16:05:48braewoodsspeachy: do we have any 64 bit ports other than some kind of hosted one?
16:05:54braewoodsi'm assuming no
16:06:07braewoodsi'm guessing even the x1000 isn't 64 bit
16:06:25speachymost sim builds are 64-bit these days
16:06:39braewoodsindeed but in terms of native or hosted...
16:07:09braewoodsif we had a 64 bit native, we could look into 64 bit optimizations but for now not much point
16:07:17speachybraewoods: 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:47braewoodsspeachy: that was the whole idea? what else needs to be checked for alignment?
16:08:02speachyit's a generic macro, so could be used for arbitrary alignment checks.
16:08:22speachy(eg dma that might need cacheline-aligned stuff for example)
16:08:47braewoodsi have an idea. gimme a minute to research it.
16:09:01speachy(or arm processors that need the irq vector table to be 128-byte aligned or somesuch)
16:09:49speachynow IMO the right thing to do is make the type/size you want to align to an argument to the macro
16:10:18speachyexplicitly instead of implicitly. IIRC Linux's alignment check macros work this way.
16:16:07braewoodsof course _Generic doesn't work like this...
16:16:24braewoodsi wish i could use it as a general catch all for pointers
16:16:53braewoodsspeachy: in any case, what different does it make? pointer address integers are just as valid as any other, no?
16:17:19braewoodsthough i guess in terms of semantics
16:17:29braewoodsthey are different even if they are usually the same
16:17:35speachyas in that new macro only works for pointers, the old one worked with whatever.
16:17:59braewoodsit cast to uintptr_t, which should work for regular integers too
16:18:14speachyI'm talking about things larger than a pointer
16:18:21speachyor smaller
16:18:30braewoodslarger is a problem, but smaller?
16:18:43braewoodssmaller gets promoted and should retains its value
16:19:25braewoodsbut afaik for the most part sizeof(void*) == sizeof(largest integer type)
16:19:28speachycall the macro IS_ALIGNED_PTR() to reflect what it really does
16:19:58speachybecause you're changing its semantics.
16:20:40speachywhat 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:01:48 Quit lebellium (Quit: Leaving)
18:01:49***Saving seen data "./dancer.seen"
18:10:51 Quit ZincAlloy (Quit: Leaving.)
18:11:56 Join ZincAlloy [0] (
18:18:24 Quit ZincAlloy (Quit: Leaving.)
20:01:50***Saving seen data "./dancer.seen"
20:04:39_bilgusspeachy, ya got me on that no clue..
20:05:16speachyI won't pretend to understand the semantics of the menu code
20:05:57_bilgusI'm guessing its something v. specific bc I don't recognize the flag..
20:05:59speachyBut g#3591 does seem to be the correct thing.
20:06:01rb-bluebotGerrit review #3591 at : menu: Fix a potential nullptr deference. by Solomon Peachy
20:06:15speachy(a different unrelated thing)
20:06:52_bilgusah lol
20:09:23_bilgustemp is referenced before that..
20:09:54_bilgusah but checked −− nm
20:10:07rb-bluebotBuild Server message: New build round started. Revision df37450f91, 303 builds, 8 clients.
20:12:00speachythere's a second potential nullptr around L599, temp can be null before the switch and lead to much dereferencing.
20:12:06speachyin menu.c
20:14:31speachystarting at L587, if (in_stringlist) {} else if (temp) {} and then switch(type) { ... } where every case dereferences temp.
20:15:06speachymight only be triggerable if the menu structure is mis-defined
20:15:20speachynot familiar with this stuff to know its subtelties.
20:18:23_bilgusthat one is guarded further up IIRC
20:18:43_bilgusI think it was an oversight originally
20:19:01_bilgusbut L496 const struct settings_list *setting =
20:19:01_bilgus find_setting(temp->variable, NULL);
20:19:30speachyL496 doesn't kick in if type == MT_MENU
20:19:32_bilgusI don't see anything checking for validity but it might too be guarded]
20:20:20speachyL476, actually
20:21:10_bilgushuh how does that work??
20:21:19speachyanyway, that's a different issue. what I'm referring to is L575 on
20:21:44speachytemp is assigned in there, only if type == MT_MENU, if it's not, then it's not guaranteed to be assigned
20:22:34speachyso the MT_FUNCTION_CALL case dereferences default-assigned temp, ie NULL
20:22:54speachywhen action == ACTION_STD_OK
20:23:10rb-bluebotBuild Server message: Build round completed after 782 seconds.
20:23:13rb-bluebotBuild Server message: Revision df37450f91 result: All green
20:23:42speachyit's possible that it's semantically impossible to call STD_OK if it's not a type MT_MENU
20:24:16_bilgusseems unlikely
20:24:35speachynot something I'd care to put money on either. :)
20:27:01speachyI'll put up the results of this scan-build pass and you can interactively see for yourself.
20:35:18_bilgusI see the one at L184 and a dead assignment
20:40:56 Join dconrad [0] (~dconrad@
20:41:32_bilgusI guess this encourages you to add pointer null checks but most of these are likely benign
20:47:51speachyyeah. but there's a good chance there's some real bugs in there amongst the noise.
20:51:09_bilgusthis one likely is
20:52:06braewoodsoh man i hate parsers sometimes.
20:52:12braewoodsthey seem like the ones most prone to bugs
20:52:28braewoodssometimes i wonder if a parser generator would be a better idea
20:53:08speachybraewoods: they usually are.
20:54:58_bilgusthat one is real
20:54:58braewoodswould it even be worth replacing the theme/skin parser?
20:55:00braewoodsat this point
20:55:13braewoodsi once dabbled in parsers
20:55:44braewoodsi'll look at the parser sometime and see what its hard requirements are
20:55:56braewoodsthe grammar determines what kind of generators are practical
20:56:01speachy_bilgus: yes but only if the language file is toast.
20:56:39speachyie num_clips == 0
20:57:54_bilgusif (!size) continue;
20:58:22speachyI suppose the correct thing to do is, after the loop is finished, bail immediately if real_clips is 0.
20:58:30braewoodsdepending on requirements, i've usually used lemon for my grammar side.
20:58:46braewoodsre2c is also a decent choice
20:58:50braewoodsfor lexer
20:59:58braewoodsone reason not to use bison... it's non-reentrant last i heard
21:00:15braewoodsa reentrant parser combo is probably a good idea these days
21:00:54speachybraewoods: first you need a formal grammar, which IIRC doesn't actually exist.
21:16:13braewoodsi'd have to look
21:26:25 Join Maxdamantus [0] (~Maxdamant@user/maxdamantus)
21:31:18_bilgus g#3593
21:31:20rb-bluebotGerrit review #3593 at : talk.c check for 0 talk clips & announce_status fix typo by William Wilgus
21:33:17speachyyay, first actual bug squashed.
21:33:41speachy(required malformed input files, but hey, it's at least possible..)
21:34:40_bilgustoo bad I neglected a parens and introduced a far worse bug
21:35:54braewoods_bilgus: it attained sentience?
21:36:49_bilgusif it were that easy..
21:36:58speachyis it really a new bug if it wouldn't even compile?
21:43:11_bilgusthat one is a bug but I can't tell if its a real bug
21:47:43speachyI think it doesn't matter what exp is initialized to... though 1 seems the logical default exponent
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@
22:22:42 Quit mixfix41 (Ping timeout: 240 seconds)
22:31:51 Quit cockroach (Quit: leaving)
22:50:38munkishuh that's a strange construction
22:56:00munkisThe only reason I can think of to do that is to avoid compiler warnings while intentionally not initializing
23:23:28 Quit dconrad ()
23:25:15_bilguswhats that save 4 bytes?
23:49:45_bilgusis this really a bug?
23:50:44_bilgusthoughts opinions diatribes?

Previous day | Next day