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

libcextract: SymbolExternalizer: Drop klpe_ prefix if IBT is specified #72

Closed
wants to merge 2 commits into from

Conversation

marcosps
Copy link
Collaborator

The resulting code would be exactly the same as the original code, or at least as much as it can be the same.

@marcosps marcosps changed the title libcextract: SymbolExternalizer: Drop klpe_ prefix is IBT is specified libcextract: SymbolExternalizer: Drop klpe_ prefix if IBT is specified Jul 10, 2024
Copy link
Collaborator

@giulianobelinassi giulianobelinassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not that simple. The way you are doing this you are going to update every reference to the function or variable with a new name that is equal to the name of the old function. That is unnecessary and will result in unnecessary header expansions. You should instead skip the update of those symbols.

And please rebase it on top of the latest branch, as this conflicts with the LateExternalization :) .

@marcosps
Copy link
Collaborator Author

marcosps commented Jul 16, 2024

It is not that simple. The way you are doing this you are going to update every reference to the function or variable with a new name that is equal to the name of the old function. That is unnecessary and will result in unnecessary header expansions. You should instead skip the update of those symbols.

And please rebase it on top of the latest branch, as this conflicts with the LateExternalization :) .

But I still need to include the outstr, to add the KLP_RELOC_SYMBOL macro. Or can I just add these symbols at the end > like I'm doing for klp-ccp?

You will need to rewrite the declaration of the symbol, yes. But you do not need to change any use of the symbol.

Adding them at the end will most likely break the one-definition rule. If it doesn't, then yes, it would be just better to add them to the end of the file. That will remove any necessity of header expansion.

In not like this, how can I add the symbol to the final closure without rewriting it, and still adding the extert ...
KLP_RELOC_SYMBOL macro?

At the SymbolExternalizer we do not work with printing what is in the closure, but rather modifying the source file. That is why we provide the Insert_Text, Remove_Text and Replace_Text methods. If you want to append stuff at the end of the file, just Insert_Text with a SourceLocation that reflects it. See SourceManager::getLocForEndOfFile and SourceManager::getMainFileID

Now with regard to the closure, if the Decl you are externalizing is being referenced by the function you want to extract or any of its dependencies, then it will be dragged to the closure by consequence. We do not eliminate any redeclarations.

@marcosps
Copy link
Collaborator Author

@giulianobelinassi please take a look in this new version of the patch. I found some issues with this approach, because it seems that it trigger different header expansion to some LPs tested (thanks to my klp-build test.sh script now I can test if changes like this ripple over previously working LPs...)

I'm sure that I'm forgetting something...

Signed-off-by: Marcos Paulo de Souza <[email protected]>
@marcosps
Copy link
Collaborator Author

marcosps commented Aug 1, 2024

This one was superseded by #101

@marcosps marcosps closed this Aug 1, 2024
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