| Message ID | 0cd234096c9bfa8d29eac9906553af79d378733e.1619524463.git.costin.lupu@cs.pub.ro (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix redefinition errors for toolstack libs | expand |
Hi, At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote: > If PAGE_SIZE is already defined in the system (e.g. in > /usr/include/limits.h header) then gcc will trigger a redefinition error > because of -Werror. This commit also protects PAGE_SHIFT definitions for > keeping consistency. Thanks for looking into this! I think properly speaking we should fix this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and kdd-xen.c. If for some reason we ever ended up with a system-defined PAGE_SIZE that wasn't 4096u then we would not want to use it here because it would break our guest operations. Cheers, Tim
Hi Tim, On 4/29/21 10:58 PM, Tim Deegan wrote: > Hi, > > At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote: >> If PAGE_SIZE is already defined in the system (e.g. in >> /usr/include/limits.h header) then gcc will trigger a redefinition error >> because of -Werror. This commit also protects PAGE_SHIFT definitions for >> keeping consistency. > > Thanks for looking into this! I think properly speaking we should fix > this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using > those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and > kdd-xen.c. If for some reason we ever ended up with a system-defined > PAGE_SIZE that wasn't 4096u then we would not want to use it here > because it would break our guest operations. As discussed for another patch of the series, an agreed solution that would apply for other libs as well would be to use XC_PAGE_* macros instead of PAGE_* macros. I've just sent a v2 doing that. Please let me know if you think it would be better to introduce the KDD_PAGE_* definitions instead. Cheers, Costin
At 14:36 +0300 on 30 Apr (1619793419), Costin Lupu wrote: > Hi Tim, > > On 4/29/21 10:58 PM, Tim Deegan wrote: > > Hi, > > > > At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote: > >> If PAGE_SIZE is already defined in the system (e.g. in > >> /usr/include/limits.h header) then gcc will trigger a redefinition error > >> because of -Werror. This commit also protects PAGE_SHIFT definitions for > >> keeping consistency. > > > > Thanks for looking into this! I think properly speaking we should fix > > this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using > > those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and > > kdd-xen.c. If for some reason we ever ended up with a system-defined > > PAGE_SIZE that wasn't 4096u then we would not want to use it here > > because it would break our guest operations. > > As discussed for another patch of the series, an agreed solution that > would apply for other libs as well would be to use XC_PAGE_* macros > instead of PAGE_* macros. I've just sent a v2 doing that. Please let me > know if you think it would be better to introduce the KDD_PAGE_* > definitions instead. Sorry to be annoying, but yes, please do introduce the KDD_ versions. All the xen-specific code in KDD lives in kdd-xen.c; kdd.c shouldn't include any xen headers. Cheers, Tim.
On 4/30/21 9:45 PM, Tim Deegan wrote: > At 14:36 +0300 on 30 Apr (1619793419), Costin Lupu wrote: >> Hi Tim, >> >> On 4/29/21 10:58 PM, Tim Deegan wrote: >>> Hi, >>> >>> At 15:05 +0300 on 27 Apr (1619535916), Costin Lupu wrote: >>>> If PAGE_SIZE is already defined in the system (e.g. in >>>> /usr/include/limits.h header) then gcc will trigger a redefinition error >>>> because of -Werror. This commit also protects PAGE_SHIFT definitions for >>>> keeping consistency. >>> >>> Thanks for looking into this! I think properly speaking we should fix >>> this by defining KDD_PAGE_SHIFT and KDD_PAGE_SIZE in kdd.h and using >>> those everywhere we currently use PAGE_SIZE/PAGE_SHIFT. in kdd.c and >>> kdd-xen.c. If for some reason we ever ended up with a system-defined >>> PAGE_SIZE that wasn't 4096u then we would not want to use it here >>> because it would break our guest operations. >> >> As discussed for another patch of the series, an agreed solution that >> would apply for other libs as well would be to use XC_PAGE_* macros >> instead of PAGE_* macros. I've just sent a v2 doing that. Please let me >> know if you think it would be better to introduce the KDD_PAGE_* >> definitions instead. > > Sorry to be annoying, but yes, please do introduce the KDD_ versions. > All the xen-specific code in KDD lives in kdd-xen.c; kdd.c shouldn't > include any xen headers. No worries, will do. I imagined that might be the case for kdd.c, but I wasn't sure. Cheers, Costin
diff --git a/tools/debugger/kdd/kdd-xen.c b/tools/debugger/kdd/kdd-xen.c index f3f9529f9f..04d2361ba7 100644 --- a/tools/debugger/kdd/kdd-xen.c +++ b/tools/debugger/kdd/kdd-xen.c @@ -48,8 +48,12 @@ #define MAPSIZE 4093 /* Prime */ +#ifndef PAGE_SHIFT #define PAGE_SHIFT 12 +#endif +#ifndef PAGE_SIZE #define PAGE_SIZE (1U << PAGE_SHIFT) +#endif struct kdd_guest { struct xentoollog_logger xc_log; /* Must be first for xc log callbacks */ diff --git a/tools/debugger/kdd/kdd.c b/tools/debugger/kdd/kdd.c index 17513c2650..acd5832714 100644 --- a/tools/debugger/kdd/kdd.c +++ b/tools/debugger/kdd/kdd.c @@ -288,8 +288,12 @@ static void kdd_log_pkt(kdd_state *s, const char *name, kdd_pkt *p) * Memory access: virtual addresses and syntactic sugar. */ +#ifndef PAGE_SHIFT #define PAGE_SHIFT (12) +#endif +#ifndef PAGE_SIZE #define PAGE_SIZE (1ULL << PAGE_SHIFT) +#endif static uint32_t kdd_read_physical(kdd_state *s, uint64_t addr, uint32_t len, void *buf)
If PAGE_SIZE is already defined in the system (e.g. in /usr/include/limits.h header) then gcc will trigger a redefinition error because of -Werror. This commit also protects PAGE_SHIFT definitions for keeping consistency. Signed-off-by: Costin Lupu <costin.lupu@cs.pub.ro> --- tools/debugger/kdd/kdd-xen.c | 4 ++++ tools/debugger/kdd/kdd.c | 4 ++++ 2 files changed, 8 insertions(+)