Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
May 31, 2024

Conversation

giulianobelinassi
Copy link
Collaborator

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.

Previously, if a struct was defined as:

struct S {
} __aligned(64);

whereas __aligned(x) is a macro, clang-extract would only copy
__aligned, thus misscompiling the code.

Signed-off-by: Giuliano Belinassi <[email protected]>
…mbol

Previously, FunctionDepsFinder::Find_Functions_Required sometimes only
analyzed the last declaration of a function, which breaks in the case
where the function has a new prototype defined later of its body.  The
reason for this is that DeclContext::lookup fails to find all symbols
of a given name.

If this is a result of a bug in LLVM this is yet to be determined.

Fixes SUSE#20.

Signed-off-by: Giuliano Belinassi <[email protected]>
Signed-off-by: Giuliano Belinassi <[email protected]>
@marcosps
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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!

@giulianobelinassi giulianobelinassi merged commit 2885d83 into SUSE:main May 31, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants