Generated code has compilation errors when extracting tls_device_rx_resync_new_rec and tls_dev_event from file net/tls/tls_device.c

[marcosps]

This problem now appears from #30, which solved the problem that clang-extract was failing to rename symbols, but now the code doesn't compile:

In file included from /home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.3.18-150300.59.158/include/linux/compiler_types.h:68,
                 from <command-line>:
In function ‘arch_static_branch’,
    inlined from ‘static_key_false’ at /home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.3.18-150300.59.158/include/linux/jump_label.h:172:9,
    inlined from ‘trace_tls_device_rx_resync_nh_delay’ at /home/mpdesouza/kgr/livepatches/bsc1222402/ce/15.3u43/work_net_tls_tls_device.c/livepatch.c:852:9,
    inlined from ‘klpp_tls_device_rx_resync_new_rec’ at /home/mpdesouza/kgr/livepatches/bsc1222402/ce/15.3u43/work_net_tls_tls_device.c/livepatch.c:1249:4:
/home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.3.18-150300.59.158/include/linux/compiler-gcc.h:120:38: warning: ‘asm’ operand 0 probably does not match constraints
  120 | #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
      |                                      ^~~
/home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.3.18-150300.59.158/arch/x86/include/asm/jump_label.h:25:9: note: in expansion of macro ‘asm_volatile_goto’
   25 |         asm_volatile_goto("1:"
      |         ^~~~~~~~~~~~~~~~~
/home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.3.18-150300.59.158/include/linux/compiler-gcc.h:120:38: error: impossible constraint in ‘asm’
  120 | #define asm_volatile_goto(x...) do { asm goto(x); asm (""); } while (0)
      |                                      ^~~
/home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.3.18-150300.59.158/arch/x86/include/asm/jump_label.h:25:9: note: in expansion of macro ‘asm_volatile_goto’
   25 |         asm_volatile_goto("1:"

This was reported here, but I opened this bug to track this new issue.

[marcosps]

With the current code is returns a different error:

warning: argument unused during compilation: '--param asan-globals=1'                                                                
warning: argument unused during compilation: '--param asan-instrumentation-with-call-threshold=10000'                                
warning: argument unused during compilation: '--param asan-instrument-allocas=1'                                                     
warning: argument unused during compilation: '--param asan-stack=1'                                                                  
warning: argument unused during compilation: '--param asan-kernel-mem-intrinsic-prefix=1'                                            
warning: argument unused during compilation: '--param asan-globals=1'                                                                
warning: argument unused during compilation: '--param asan-instrumentation-with-call-threshold=10000'                                
warning: argument unused during compilation: '--param asan-instrument-allocas=1'                                                     
warning: argument unused during compilation: '--param asan-stack=1'                                                                  
warning: argument unused during compilation: '--param asan-kernel-mem-intrinsic-prefix=1'                                            
net/tls/tls_device.c:153:25: error: ignoring return value of function declared with const attribute                                  
  153 |                         __builtin_expect(!!(__ret_warn_on), 0);                                                              
      |                         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                
net/tls/tls_device.c:279:21: error: ignoring return value of function declared with const attribute                                  
  279 |                     __builtin_expect(!!(__ret_warn_on), 0);                                                                  
      |                     ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                    
net/tls/tls_device.c:281:13: error: ignoring return value of function declared with const attribute                                  
  281 |             __builtin_expect(!!(__ret_do_once), 0);                                                                          
      |             ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                            
net/tls/tls_device.c:382:25: error: ignoring return value of function declared with const attribute                                  
  382 |                         __builtin_expect(!!(__ret_warn_on), 0);                                                              
      |                         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                
net/tls/tls_device.c:508:21: error: ignoring return value of function declared with const attribute                                  
  508 |                     __builtin_expect(!!(__ret_warn_on), 0);                                                                  
      |                     ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                    
net/tls/tls_device.c:510:13: error: ignoring return value of function declared with const attribute                                  
  510 |             __builtin_expect(!!(__ret_do_once), 0);                                                                          
      |             ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                            

Error on pass: ClosurePass   

This can be reproduced by running:

$ klp-build setup --kdir --data-dir <path/to/linux> --name test_tls --module tls --conf CONFIG_TLS --file-funcs net/tls/tls_device.c tls_dev_event tls_device_rx_resync_new_rec

$ klp-build extract --name test_tls

[giulianobelinassi]

With the current code is returns a different error:

warning: argument unused during compilation: '--param asan-globals=1'                                                                
warning: argument unused during compilation: '--param asan-instrumentation-with-call-threshold=10000'                                
warning: argument unused during compilation: '--param asan-instrument-allocas=1'                                                     
warning: argument unused during compilation: '--param asan-stack=1'                                                                  
warning: argument unused during compilation: '--param asan-kernel-mem-intrinsic-prefix=1'                                            
warning: argument unused during compilation: '--param asan-globals=1'                                                                
warning: argument unused during compilation: '--param asan-instrumentation-with-call-threshold=10000'                                
warning: argument unused during compilation: '--param asan-instrument-allocas=1'                                                     
warning: argument unused during compilation: '--param asan-stack=1'                                                                  
warning: argument unused during compilation: '--param asan-kernel-mem-intrinsic-prefix=1'                                            
net/tls/tls_device.c:153:25: error: ignoring return value of function declared with const attribute                                  
  153 |                         __builtin_expect(!!(__ret_warn_on), 0);                                                              
      |                         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                
net/tls/tls_device.c:279:21: error: ignoring return value of function declared with const attribute                                  
  279 |                     __builtin_expect(!!(__ret_warn_on), 0);                                                                  
      |                     ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                    
net/tls/tls_device.c:281:13: error: ignoring return value of function declared with const attribute                                  
  281 |             __builtin_expect(!!(__ret_do_once), 0);                                                                          
      |             ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                            
net/tls/tls_device.c:382:25: error: ignoring return value of function declared with const attribute                                  
  382 |                         __builtin_expect(!!(__ret_warn_on), 0);                                                              
      |                         ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                
net/tls/tls_device.c:508:21: error: ignoring return value of function declared with const attribute                                  
  508 |                     __builtin_expect(!!(__ret_warn_on), 0);                                                                  
      |                     ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                    
net/tls/tls_device.c:510:13: error: ignoring return value of function declared with const attribute                                  
  510 |             __builtin_expect(!!(__ret_do_once), 0);                                                                          
      |             ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                            

Error on pass: ClosurePass   

This can be reproduced by running:

$ klp-build setup --kdir --data-dir <path/to/linux> --name test_tls --module tls --conf CONFIG_TLS --file-funcs net/tls/tls_device.c tls_dev_event tls_device_rx_resync_new_rec

$ klp-build extract --name test_tls

This looks like a Warning that was escalated to an Error because of Werror. What is the code output if you disable Werror?

[marcosps]

Yes, that's true. I didn't disabled that before to check with you if that's something that needed to be done on clang-extract side, or perhaps a bug. Either way, disabling -Werror did the trick, but then, another issues appeared:

net/tls/tls_device.c:435:32: error: indirection requires pointer operand ('struct tracepoint' invalid)                               
  435 |                     typeof ((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs) __UNIQUE_ID_rcu960 = ({            
      |                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                              
net/tls/tls_device.c:441:64: error: indirection requires pointer operand ('struct tracepoint' invalid)                               
  441 |                         (*(const volatile typeof (_Generic(((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs), char: (char)0, unsigned char: (unsigned char)0, signed char: (signed char)0, unsigned short: (unsigned short)0, short: (short)0, unsigned int: (unsigned int)0, int: (int)0, unsigned long: (unsigned long)0, long: (long)0, unsigned long long: (unsigned long long)0, long long: (long long)0, default: ((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs))) *)&((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs));                                                                                                           
      |                                                                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~              
net/tls/tls_device.c:441:426: error: indirection requires pointer operand ('struct tracepoint' invalid)                              
  441 |                         (*(const volatile typeof (_Generic(((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs), char: (char)0, unsigned char: (unsigned char)0, signed char: (signed char)0, unsigned short: (unsigned short)0, short: (short)0, unsigned int: (unsigned int)0, int: (int)0, unsigned long: (unsigned long)0, long: (long)0, unsigned long long: (unsigned long long)0, long long: (long long)0, default: ((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs))) *)&((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs));                                                                                                           
      |                                                                                                                                                                                                                                                                                                                                                                                                                                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                   
net/tls/tls_device.c:443:35: error: indirection requires pointer operand ('struct tracepoint' invalid)                               
  443 |                     ((typeof (*(&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs) *)(__UNIQUE_ID_rcu960));        
      |                                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                           
net/tls/tls_device.c:434:29: error: assigning to 'struct tracepoint_func *' from incompatible type 'void'                            
  434 |                 it_func_ptr = ({                                                                                             
      |                             ^ ~~                                                                                             
  435 |                     typeof ((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs) __UNIQUE_ID_rcu960 = ({            
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~            
  436 |                         do {                                                                                                 
      |                         ~~~~                                                                                                 
  437 |                             extern void __compiletime_assert_961(void);                                                      
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                      
  438 |                             if (!((sizeof ((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs) == sizeof(char) || sizeof ((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs) == sizeof(short) || sizeof ((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs) == sizeof(int) || sizeof ((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs) == sizeof(long)) || sizeof ((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs) == sizeof(long long)))                                                       
      |                             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                       
  439 |                                 __compiletime_assert_961();                                                                  
      |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                  
  440 |                         } while (0);                                                                                         
      |                         ~~~~~~~~~~~~                                                                                         
  441 |                         (*(const volatile typeof (_Generic(((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs), char: (char)0, unsigned char: (unsigned char)0, signed char: (signed char)0, unsigned short: (unsigned short)0, short: (short)0, unsigned int: (unsigned int)0, int: (int)0, unsigned long: (unsigned long)0, long: (long)0, unsigned long long: (unsigned long long)0, long long: (long long)0, default: ((&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs))) *)&((&klpe___tracepoint_tls_device_rx_resync_nh_delay)->funcs));                                                                                                           
      |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~                                                                                                           
  442 |                     });                                                                                                      
      |                     ~~~                                                                                                      
  443 |                     ((typeof (*(&(*klpe___tracepoint_tls_device_rx_resync_nh_delay))->funcs) *)(__UNIQUE_ID_rcu960));        
      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~        
  444 |                 });                                                                                                          
      |                 ~~                                                                                                           
net/tls/tls_device.c:544:21: warning: ignoring return value of function declared with const attribute                                
  544 |                     __builtin_expect(!!(__ret_warn_on), 0);                                                                  
      |                     ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                    
net/tls/tls_device.c:546:13: warning: ignoring return value of function declared with const attribute                                
  546 |             __builtin_expect(!!(__ret_do_once), 0);                                                                          
      |             ^~~~~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~~~~~                                                                            

Error on pass: FunctionExternalizerPass 

[marcosps]

Checking with the current state of clang-extract, the new error message:

net/tls/tls_device.c:392:185: error: initializer element is not a compile-time constant
  392 |                         static void *__UNIQUE_ID___addressable___SCK__tp_func_tls_device_rx_resync_nh_delay960 __attribute__((used)) __attribute__((section(".discard.addressable"))) = (void *)(uintptr_t)&(*klpe___SCK__tp_func_tls_device_rx_resync_nh_delay);

The most interesting part is that we have two different SCK (static call key) being used in this file, but only one is externalized. Both are present in the tls.ko, so both should be externalized.

[giulianobelinassi]

Checking with the current state of clang-extract, the new error message:

net/tls/tls_device.c:392:185: error: initializer element is not a compile-time constant
  392 |                         static void *__UNIQUE_ID___addressable___SCK__tp_func_tls_device_rx_resync_nh_delay960 __attribute__((used)) __attribute__((section(".discard.addressable"))) = (void *)(uintptr_t)&(*klpe___SCK__tp_func_tls_device_rx_resync_nh_delay);

The most interesting part is that we have two different SCK (static call key) being used in this file, but only one is externalized. Both are present in the tls.ko, so both should be externalized.

The problem here is that

*klpe___SCK__tp_func_tls_device_rx_resync_nh_delay

Is not known at compile time. Perhaps a weak externalization is able to fix this, but this needs testing.

[marcosps]

That's true. Now that I'm checking what we do in our own defined TRACE_EVENT, we don't use static_call. In upstream we have:

#ifdef CONFIG_HAVE_STATIC_CALL                                                                            
#define __DO_TRACE_CALL(name, args)                                     \                                 
        do {                                                            \                                 
                struct tracepoint_func *it_func_ptr;                    \                                 
                void *__data;                                           \                                 
                it_func_ptr =                                           \                                 
                        rcu_dereference_raw((&__tracepoint_##name)->funcs); \                             
                if (it_func_ptr) {                                      \                                 
                        __data = (it_func_ptr)->data;                   \                                 
                        static_call(tp_func_##name)(__data, args);      \                                 
                }                                                       \                                         } while (0)                                                                                       
#else                                                                                                     
#define __DO_TRACE_CALL(name, args)     __traceiter_##name(NULL, args)                                    
#endif /* CONFIG_HAVE_STATIC_CALL */ 

Our macros are used like CONFIG_HAVE_STATIC_CALL is not set, so we don't end up calling that static_call, which ends up on:

static_call static_call STATIC_CALL_ADDRESSABLE ADDRESSABLE(STATIC_CALLKEY(name)) ADDRESSABLE(sym, __section(".discard.addressable"))

That ends up generating:

        static void * __used __attrs \                                                                    
        __UNIQUE_ID(__PASTE(__addressable_,sym)) = (void *)(uintptr_t)&sym;

So we may need to decide what to do in this case. I think that we indeed need to externalize the tracepoint, but I need to think more about this macro madness imposed by tracepoints.

[marcosps]

With f7c935b75842ad7bf51ed860bbecdf0a1ea831d4 and ibt being used the problem is gone.