Re: r247835: drm2 code breaks buildkernel

On Tue, Mar 05, 2013 at 03:13:12PM +0100, Jean-S?bastien P?dron wrote:
> On 05.03.2013 13:30, Glen Barber wrote:
> > dev/drm2/ttm/ttm_lock.h:208: warning: redundant redeclaration of 'ttm_write_unlock' [-Wredundant-decls]
> > dev/drm2/ttm/ttm_lock.h:134: warning: previous declaration of 'ttm_write_unlock' was here
> > dev/drm2/ttm/ttm_lock.h:220: warning: redundant redeclaration of 'ttm_write_lock' [-Wredundant-decls]
> > dev/drm2/ttm/ttm_lock.h:146: warning: previous declaration of 'ttm_write_lock' was here
> 
> Those redundant declarations weren't spotted by clang.
> 
> Konstantin, would you like me to commit the fix for this? And we need to
> upstream it too.
I am somewhat sceptical about upstreaming. I reported a real bug in the
ttm_lock with the trivial fix when I read the code for porting. Bug was
confirmed by Linux people, but no action was taken.

> 
> > dev/drm2/ttm/ttm_page_alloc.c:122: warning: declaration does not declare anything
> > dev/drm2/ttm/ttm_page_alloc.c:123: warning: declaration does not declare anything
> 
> These errors and the following are caused by unnamed structs and unions
> inside another struct:
> 
> struct ttm_pool_manager {
>         ...
> 
>         union {
>                 struct ttm_page_pool    pools[NUM_POOLS];
>                 struct {
>                         ...
>                 } ;
>         };
> };
> 
> With default options, clang accepts this but apparently, not gcc.
> 
> I would like an opinion from the toolchain gurus, because I don't know
> what's the proper way to fix this one.
> 
> J.R. Oldroyd CC'd, because he started to work on radeonkms backport to 9
> and faced exactly those issues.

The patch below is supposed to fix double declaration (it is pathetic that
clang silently accepts this, while issuing countless useless warnings).
Also there is a usual workaround for the anonimous union/struct issue.

Glen neglected to even mention that he used gcc (is it true ?), as well
as to show the command line invocation of the compiler. Hopefully the
patch helps.

diff --git a/sys/dev/drm2/ttm/ttm_lock.h b/sys/dev/drm2/ttm/ttm_lock.h
index ac8159e..6d45457 100644
--- a/sys/dev/drm2/ttm/ttm_lock.h
+++ b/sys/dev/drm2/ttm/ttm_lock.h
_at__at_ -125,27 +125,6 _at__at_ extern int ttm_read_lock(struct ttm_lock *lock, bool interruptible);
 extern int ttm_read_trylock(struct ttm_lock *lock, bool interruptible);
 
 /**
- * ttm_write_unlock
- *
- * _at_lock: Pointer to a struct ttm_lock
- *
- * Releases a write lock.
- */
-extern void ttm_write_unlock(struct ttm_lock *lock);
-
-/**
- * ttm_write_lock
- *
- * _at_lock: Pointer to a struct ttm_lock
- * _at_interruptible: Interruptible sleeping while waiting for a lock.
- *
- * Takes the lock in write mode.
- * Returns:
- * -ERESTARTSYS If interrupted by a signal and interruptible is true.
- */
-extern int ttm_write_lock(struct ttm_lock *lock, bool interruptible);
-
-/**
  * ttm_lock_downgrade
  *
  * _at_lock: Pointer to a struct ttm_lock
diff --git a/sys/dev/drm2/ttm/ttm_page_alloc.c b/sys/dev/drm2/ttm/ttm_page_alloc.c
index ffc8483..9a30a46 100644
--- a/sys/dev/drm2/ttm/ttm_page_alloc.c
+++ b/sys/dev/drm2/ttm/ttm_page_alloc.c
_at__at_ -113,16 +113,22 _at__at_ struct ttm_pool_manager {
 	struct ttm_pool_opts	options;
 
 	union {
-		struct ttm_page_pool	pools[NUM_POOLS];
-		struct {
-			struct ttm_page_pool	wc_pool;
-			struct ttm_page_pool	uc_pool;
-			struct ttm_page_pool	wc_pool_dma32;
-			struct ttm_page_pool	uc_pool_dma32;
-		} ;
-	};
+		struct ttm_page_pool	u_pools[NUM_POOLS];
+		struct _utag {
+			struct ttm_page_pool	u_wc_pool;
+			struct ttm_page_pool	u_uc_pool;
+			struct ttm_page_pool	u_wc_pool_dma32;
+			struct ttm_page_pool	u_uc_pool_dma32;
+		} _ut;
+	} _u;
 };
 
+#define	pools _u.u_pools
+#define	wc_pool _u._ut.u_wc_pool
+#define	uc_pool _u._ut.u_uc_pool
+#define	wc_pool_dma32 _u._ut.u_wc_pool_dma32
+#define	uc_pool_dma32 _u._ut.u_uc_pool_dma32
+
 MALLOC_DEFINE(M_TTM_POOLMGR, "ttm_poolmgr", "TTM Pool Manager");
 
 static void