> On Aug 19, 2022, at 01:52, Claudio Jeker <[email protected]> wrote: > > On Thu, Aug 18, 2022 at 10:32:36PM -0500, Scott Cheloha wrote: >> Hi, >> >> clockframe is sometimes defined in cpu.h, sometimes in frame.h, and >> sometimes defined once each in both header files. >> >> Can we put the clockframe definitions in frame.h? Always? It is, at >> least ostensibly, a "frame". >> >> I do not want to consolidate the clockframe definitions in cpu.h >> because this is creating a circular dependency problem for my clock >> interrupt patch. >> >> In particular, cpu.h needs a data structure defined in a new header >> file to add it to struct cpu_info on all architectures, like this: >> >> /* cpu.h */ >> >> #include <sys/clockintr.h> >> >> struct cpu_info { >> /* ... */ >> struct clockintr_state; >> }; >> >> ... but the header clockintr.h needs the clockframe definition so it >> can prototype functions accepting a clockframe pointer, like this: >> >> /* clockintr.h */ >> >> #include <machine/frame.h> /* this works fine */ >> >> #ifdef this_does_not_work >> #include <machine/cpu.h> >> #endif >> >> int clockintr_foo(struct clockframe *, int, short); >> int clockintr_bar(struct clockframe *, char *, long); > > You can also just do: > > struct clockframe; > int clockintr_foo(struct clockframe *, int, short); > int clockintr_bar(struct clockframe *, char *, long); > > There is no need to have the full struct definition for a pointer. > With that there is no need to include machine/frame.h or machine/cpu.h > In my opinion this may be the preferred way of handling this but unifying > the definitions into one place still makes sense to me.
That was the first thing I tried. It doesn't work if clockframe is not a real struct. On some platforms it's just a preprocessor macro. So you end up with a redefinition and compilation fails, hence this patch. That particular landmine is already in the tree, in systm.h. It seems to have laid dormant due to #include ordering and luck. I think there is also a discussion to be had about whether we could just throw away the "clockframe" abstraction and put a trapframe/intrframe/whatever pointer into cpu_info and rewrite the CLKF macros to use a cpu_info pointer... ... but that seemed like more work than just making the place-of-definition consistent. Does that sound more appealing than "fixing" clockframe?
