Re: inline asm semantics: output constraint width smaller than input

["Török Edwin" <edwintorok@gmail.com>]

On 2009-01-24 19:27, Ingo Molnar wrote:
> * Török Edwin <edwintorok@gmail.com> wrote:
>   
>>  #define put_user(x, ptr)                    \
>>  ({                                \
>> -    int __ret_pu;                        \
>> +    __typeof__(*(ptr)) __ret_pu;                \
>>     
>
> This does not look right. We can sometimes have put_user() of non-integer 
> types (say structures). 

I didn't encounter it with my .config, but it is certainly possible.
I think using __builtin_choose_expr would be better than the switch

See a new patch at the end of this mail, using __builtin_choose_expr.
[vmlinux size still same]
It also includes some 32-bit stuff, but that is not complete yet.

> How does the (int)__ret_pu cast work in that case? 
>   

It would fail at compile time, with an error message that you can't cast
aggregates to ints, so my patch is not good.

> We'll fall into this branch in that case:
>
>         default:                                                \
>                 __put_user_x(X, __pu_val, ptr, __ret_pu);       \
>                 break;                                          \
>
> and __ret_pu has a nonsensical type in that case.
>   

That branch is a call to a non-existent function __put_user_X, and
should give error at link time, right?
In the new patch below I used (void)-EFAULT so that you would get an
error at compile time (as suggested
in __builtin_choose_expr in gcc's manual), if that branch would ever get
expanded. Does that sound right?

>   
>>      __typeof__(*(ptr)) __pu_val;                \
>>      __chk_user_ptr(ptr);                    \
>>      might_fault();                        \
>>      __pu_val = x;                        \
>> +       /* return value is 0 or -EFAULT, both fit in 1 byte, and \
>> +    * are sign-extendable to int */                \
>>      switch (sizeof(*(ptr))) {                \
>>      case 1:                            \
>>          __put_user_x(1, __pu_val, ptr, __ret_pu);    \
>> @@ -261,7 +263,7 @@ extern void __put_user_8(void);
>>          __put_user_x(X, __pu_val, ptr, __ret_pu);    \
>>          break;                        \
>>      }                            \
>> -    __ret_pu;                        \
>> +    (int)__ret_pu;                        \
>>  })
>>  
>>  #define __put_user_size(x, ptr, size, retval, errret)            \
>> diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
>> index f456860..12d27f8 100644
>> --- a/arch/x86/lib/delay.c
>> +++ b/arch/x86/lib/delay.c
>> @@ -112,7 +112,7 @@ EXPORT_SYMBOL(__delay);
>>  
>>  inline void __const_udelay(unsigned long xloops)
>>  {
>> -    int d0;
>> +    unsigned long d0;
>>  
>>      xloops *= 4;
>>      asm("mull %%edx"
>>     
>
> Is this all that you need (plus the 16-bit setup code tweaks)

The 16-bit setup code is compiled, but obviously doesn't work.
I think the best approach would be for LLVM to give a warning/error, when
-fno-unit-at-a-time is used, since it doesn't support that.

>  to get LLVM 
> to successfully build a 64-bit kernel image?
>   

With the .config I sent previously, yes. With some other .config most
likely more changes are needed,
for example the SMP code, KVM code, but as I said in my previous email I
don't know how to "fix" the inline asm in that case.

There's something wrong with building some modules also, I keep getting
an "idr_init" undefined error, but the symbol is present in my vmlinux.
If I turn make those modules built into the kernel it works then (sctp,
w1, thermalsysfs).

It looks like I'll also have to submit some patches for ARCH=um, because
I get undefined references to __bad_size, __guard, and

__stack_smash_handler. __bad_size is probably because LLVM didn't inline expand + DCE something GCC did.


With an unpatched LLVM I would also need weak attributes to be on the
function type, instead of the return type,
but I think thats an LLVM bug, and a one-line patch corrects it.

> If yes then this doesnt look all that bad or invasive at first sight (if 
> the put_user() workaround can be expressed in a cleaner way), but in any 
> case it would be nice to hear an LLVM person's opinion about roughly when 
> this is going to be solved in LLVM itself.
>   

Yes, that is why LLVMDev is on Cc:, somebody will eventually reply ;)


Not-Signed-off-by: Török Edwin <edwintorok@gmail.com>
---

 arch/x86/include/asm/uaccess.h |   41
+++++++++++++++++++--------------------
 arch/x86/lib/delay.c           |    2 +-
 arch/x86/pci/pcbios.c          |    4 ++-
 3 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 69d2757..46d00a8 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -154,7 +154,7 @@ extern int __get_user_bad(void);
 
 #define get_user(x, ptr)                        \
 ({                                    \
-    int __ret_gu;                            \
+    unsigned long __ret_gu;                        \
     unsigned long __val_gu;                        \
     __chk_user_ptr(ptr);                        \
     might_fault();                            \
@@ -176,7 +176,7 @@ extern int __get_user_bad(void);
         break;                            \
     }                                \
     (x) = (__typeof__(*(ptr)))__val_gu;                \
-    __ret_gu;                            \
+    (int)__ret_gu;                            \
 })
 
 #define __put_user_x(size, x, ptr, __ret_pu)            \
@@ -200,12 +200,15 @@ extern int __get_user_bad(void);
              : "A" (x), "r" (addr), "i" (-EFAULT), "0" (err))
 
 #define __put_user_x8(x, ptr, __ret_pu)                \
-    asm volatile("call __put_user_8" : "=a" (__ret_pu)    \
-             : "A" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
+    ({ u32 __ret_pu;\
+     asm volatile("call __put_user_8" : "=a" (__ret_pu)    \
+             : "A" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx");\
+     (int)__ret_pu;})
 #else
 #define __put_user_asm_u64(x, ptr, retval) \
     __put_user_asm(x, ptr, retval, "q", "", "Zr", -EFAULT)
-#define __put_user_x8(x, ptr, __ret_pu) __put_user_x(8, x, ptr, __ret_pu)
+#define __put_user_x8(x, ptr, __ret_pu) \
+    ({ u64 __ret_pu; __put_user_x(8, x, ptr, __ret_pu); (int)__ret_pu; })
 #endif
 
 extern void __put_user_bad(void);
@@ -239,29 +242,25 @@ extern void __put_user_8(void);
  */
 #define put_user(x, ptr)                    \
 ({                                \
-    int __ret_pu;                        \
     __typeof__(*(ptr)) __pu_val;                \
     __chk_user_ptr(ptr);                    \
     might_fault();                        \
     __pu_val = x;                        \
-    switch (sizeof(*(ptr))) {                \
-    case 1:                            \
+    __builtin_choose_expr(sizeof(*(ptr)) == 1,        \
+        ({  u8 __ret_pu;                    \
         __put_user_x(1, __pu_val, ptr, __ret_pu);    \
-        break;                        \
-    case 2:                            \
+            (int)__ret_pu;}),                \
+    __builtin_choose_expr(sizeof(*(ptr)) == 2,        \
+            ({  u16 __ret_pu;                    \
         __put_user_x(2, __pu_val, ptr, __ret_pu);    \
-        break;                        \
-    case 4:                            \
+            (int)__ret_pu;}),                \
+    __builtin_choose_expr(sizeof(*(ptr)) == 4,        \
+        ({  u32 __ret_pu;                    \
         __put_user_x(4, __pu_val, ptr, __ret_pu);    \
-        break;                        \
-    case 8:                            \
-        __put_user_x8(__pu_val, ptr, __ret_pu);        \
-        break;                        \
-    default:                        \
-        __put_user_x(X, __pu_val, ptr, __ret_pu);    \
-        break;                        \
-    }                            \
-    __ret_pu;                        \
+        (int)__ret_pu;}),                \
+    __builtin_choose_expr(sizeof(*(ptr)) == 8,        \
+        __put_user_x8(__pu_val, ptr, __ret_pu),        \
+            (void)-EFAULT))));                \
 })
 
 #define __put_user_size(x, ptr, size, retval, errret)            \
diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
index f456860..12d27f8 100644
--- a/arch/x86/lib/delay.c
+++ b/arch/x86/lib/delay.c
@@ -112,7 +112,7 @@ EXPORT_SYMBOL(__delay);
 
 inline void __const_udelay(unsigned long xloops)
 {
-    int d0;
+    unsigned long d0;
 
     xloops *= 4;
     asm("mull %%edx"
diff --git a/arch/x86/pci/pcbios.c b/arch/x86/pci/pcbios.c
index b82cae9..dfff175 100644
--- a/arch/x86/pci/pcbios.c
+++ b/arch/x86/pci/pcbios.c
@@ -65,6 +65,7 @@ static struct {
 static unsigned long bios32_service(unsigned long service)
 {
     unsigned char return_code;    /* %al */
+    unsigned long return_code_compat; /* %eax */
     unsigned long address;        /* %ebx */
     unsigned long length;        /* %ecx */
     unsigned long entry;        /* %edx */
@@ -72,13 +73,14 @@ static unsigned long bios32_service(unsigned long
service)
 
     local_irq_save(flags);
     __asm__("lcall *(%%edi); cld"
-        : "=a" (return_code),
+        : "=a" (return_code_compat),
           "=b" (address),
           "=c" (length),
           "=d" (entry)
         : "0" (service),
           "1" (0),
           "D" (&bios32_indirect));
+    return_code = return_code_compat;
     local_irq_restore(flags);
 
     switch (return_code) {
-- 
1.5.6.5