search

SUSE / clang-extract

Other
7 stars 3 forks source link

FunctionDepsFinder: Fix closure computation not analyzing functions which have a prototype declared after its body #28

Closed giulianobelinassi closed 1 month ago

giulianobelinassi commented 1 month ago

clang has a mechanism (DeclContext::lookup) which is SUPPOSED TO return the list of all Decls that matches the lookup name. However, this method doesn't work as intented. In kernel (see github issue #20) it results in the lookup_result set not having the Decl that has the body of the function, which is the most important!

Therefore, we fallback to the older but slower method: sweep through the AST top level looking for the Decls we want to extract.

If this issue in the symbol name hash comes from a bug in LLVM, this still needs investigation.

marcosps commented 1 month ago

Thanks, it fixes the issue! Some interesting notes here:

Like stated on #5, some macros can be redeclared:

make -C /home/mpdesouza/kgr/data/x86_64/lib/modules/5.14.21-150500.55.59-default/build modules M=/home/mpdesouza/kgr/livepatches/bsc_giuliano/ce/15.5u12/work_drivers_net_wireless_intel_iwlwifi_iwl-dbg-tlv.c
/home/mpdesouza/kgr/livepatches/bsc_giuliano/ce/15.5u12/work_drivers_net_wireless_intel_iwlwifi_iwl-dbg-tlv.c/livepatch.c:1379: warning: "TRACE_EVENT" redefined
 1379 | #define TRACE_EVENT(name, proto, ...) \
      | 
In file included from /home/mpdesouza/kgr/livepatches/bsc_giuliano/ce/15.5u12/work_drivers_net_wireless_intel_iwlwifi_iwl-dbg-tlv.c/livepatch.c:1377:
/home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.14.21-150500.55.59/include/linux/tracepoint.h:557: note: this is the location of the previous definition
  557 | #define TRACE_EVENT(name, proto, args, struct, assign, print)   \
      |

Not issue for now, it can be addressed by the livepatch developer.

Another interesting result of this patchset is that it now adds multiple declarations of a function. For example:

/** clang-extract: from drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h:56:1  */                                     
void klpp__iwl_dbg_tlv_time_point(struct iwl_fw_runtime *fwrt,                                                        
                             enum iwl_fw_ini_time_point tp_id,                                                                                     union iwl_dbg_tlv_tp_data *tp_data,                                                                                   bool sync);

was found on line 758, then

/** clang-extract: from drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c:1347:1  */                                   
void klpp__iwl_dbg_tlv_time_point(struct iwl_fw_runtime *fwrt,                                                        
                             enum iwl_fw_ini_time_point tp_id,                                                                                     union iwl_dbg_tlv_tp_data *tp_data,                                                                                   bool sync)

was found on line 2027, but now implementing the function, and then we have:

extern void klpp__iwl_dbg_tlv_time_point(struct iwl_fw_runtime *, enum iwl_fw_ini_time_point, union iwl_dbg_tlv_tp_data *, bool);

On line 2122, which seems like what klp-ccp currently does.

Either way, now it generates code that compile, which is the most important part, but I would like to point to these details to have it documented somewhere. Merge it!

marcosps commented 1 month ago

Actually, on klp-ccp, instead of the extern void klpp__iwl_dbg_tlv_time_point, we have typeof(klpp__iwl_dbg_tlv_time_point) klpp__iwl_dbg_tlv_time_point;.

giulianobelinassi commented 1 month ago

Thanks, it fixes the issue! Some interesting notes here:

Like stated on #5, some macros can be redeclared:

make -C /home/mpdesouza/kgr/data/x86_64/lib/modules/5.14.21-150500.55.59-default/build modules M=/home/mpdesouza/kgr/livepatches/bsc_giuliano/ce/15.5u12/work_drivers_net_wireless_intel_iwlwifi_iwl-dbg-tlv.c
/home/mpdesouza/kgr/livepatches/bsc_giuliano/ce/15.5u12/work_drivers_net_wireless_intel_iwlwifi_iwl-dbg-tlv.c/livepatch.c:1379: warning: "TRACE_EVENT" redefined
 1379 | #define TRACE_EVENT(name, proto, ...) \
      | 
In file included from /home/mpdesouza/kgr/livepatches/bsc_giuliano/ce/15.5u12/work_drivers_net_wireless_intel_iwlwifi_iwl-dbg-tlv.c/livepatch.c:1377:
/home/mpdesouza/kgr/data/x86_64/usr/src/linux-5.14.21-150500.55.59/include/linux/tracepoint.h:557: note: this is the location of the previous definition
  557 | #define TRACE_EVENT(name, proto, args, struct, assign, print)   \
      |

This may be a bug in the PrettyPrint::Print_Macro_Undef, so it may be worth investigating.

Not issue for now, it can be addressed by the livepatch developer.

Another interesting result of this patchset is that it now adds multiple declarations of a function. For example:

/** clang-extract: from drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.h:56:1  */                                     
void klpp__iwl_dbg_tlv_time_point(struct iwl_fw_runtime *fwrt,                                                        
                             enum iwl_fw_ini_time_point tp_id,                                                                                     union iwl_dbg_tlv_tp_data *tp_data,                                                                                   bool sync);

was found on line 758, then

/** clang-extract: from drivers/net/wireless/intel/iwlwifi/iwl-dbg-tlv.c:1347:1  */                                   
void klpp__iwl_dbg_tlv_time_point(struct iwl_fw_runtime *fwrt,                                                        
                             enum iwl_fw_ini_time_point tp_id,                                                                                     union iwl_dbg_tlv_tp_data *tp_data,                                                                                   bool sync)

was found on line 2027, but now implementing the function, and then we have:

extern void klpp__iwl_dbg_tlv_time_point(struct iwl_fw_runtime *, enum iwl_fw_ini_time_point, union iwl_dbg_tlv_tp_data *, bool);

On line 2122, which seems like what klp-ccp currently does.

Either way, now it generates code that compile, which is the most important part, but I would like to point to these details to have it documented somewhere. Merge it!