On 21 May 2023, at 17:57, Hans Petter Selasky <hsela...@freebsd.org> wrote: > > On 5/21/23 18:33, Jessica Clarke wrote: >> On 21 May 2023, at 17:21, Hans Petter Selasky <hsela...@freebsd.org> wrote: >>> >>> The branch main has been updated by hselasky: >>> >>> URL: >>> https://cgit.FreeBSD.org/src/commit/?id=805d759338a2be939fffc8bf3f25cfaab981a9be >>> >>> commit 805d759338a2be939fffc8bf3f25cfaab981a9be >>> Author: Hans Petter Selasky <hsela...@freebsd.org> >>> AuthorDate: 2023-05-21 11:25:28 +0000 >>> Commit: Hans Petter Selasky <hsela...@freebsd.org> >>> CommitDate: 2023-05-21 16:20:16 +0000 >>> >>> mlx4: Move DEFINE_MUTEX() outside function body. >>> >>> Move static mutex declaration outside function body, to avoid global >>> variables being declared on the stack, when using SYSINITs. >> What? This is nonsense. It’s not on the stack either way round. >> Please revert this. >> Jess > > Hi Jess, > > I think this is a false positive of yours. You need to look through all the > macros used there.
No it’s not. All the definitions are static. > Basically DEFINE_MUTEX() expands to a bunch of structures, which are not in > any block. They are all static. > The "static" you see in patch just covers the first mutex structure. Because only the first one is ever not static. The rest always are. > SYSINITs use "static" in front of all structure definitions. Exactly. > If you want to change from static structures to global symbols, then my > change is correct. Which will bloat symbol tables excessively. But you didn’t state this as your goal, you stated it as a behavioural change *today*, which it’s not (other than changing the scope). Your commit message was entirely nonsense, and so I told you that. If your goal is instead to allow for future changes, put that in your commit message. I am not a mind reader, nor is anyone else. It is extremely unhelpful to have commits that say one thing but intend to achieve a different thing. > Before: > > static DEFINE_MUTEX(xxx); > > Expands to something like: > > static struct yyy xxx; static struct sysinit zzz; .... > > > > If you want to change from "static struct sysinit zzz;" to "extern struct > sysinit zzz" and also initialize the structure there, then that won't work, > based on what I currently know about C-programming. I tried, but clang gave > me a warning about it. Clearly trying to define a variable with global scope at function scope isn’t going to work, yes. > You can't declare global variables inside a function or it is not good style. > > > > From what I can see, this location is the only place I've come accross in the > FreeBSD kernel, where a SYSINIT() is used inside a function, and I thought I > would just move that outside the function instead. > > This change also allows for: > > https://reviews.freebsd.org/D40193 Which looks like a mess to me. Jess > --HPS
