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

String may be the first string in a section #164

Open
wants to merge 3 commits into
base: hiperf
Choose a base branch
from

Conversation

ericharding
Copy link

When searching for the correct section for a string we should consider the case where the string is the very first string in a section.

Without this change sometimes a certain severity level string will be "found" in the wrong section header causing it to be garbage which falls though to the base case in severityFromString and turns into Severity::no_logs or "off" when printing to text.

@erenon
Copy link
Contributor

erenon commented Apr 13, 2023

Thank! It is a good find. I do not understand though, if it accidentally skips the correct section, why does it find the address in a different section, instead of throwing?

@spektrof this PR looks good to me, please merge. Thanks.

@ericharding
Copy link
Author

Unfortunately, I debugged though this quite a while ago and was just slow posting the merge request. I don't remember why it escaped the loop off the top of my head.

@spektrof spektrof self-requested a review April 14, 2023 07:02
@spektrof
Copy link

Hi,
The clang-tidy check failed on dependency installation. Can you check it please?

Thanks

@erenon
Copy link
Contributor

erenon commented Apr 14, 2023

To fix the CI, I backported changes from the main branch here: #165
However, Clang 14 tests fails, and I'm unable to repro with Clang 15. It needs some more work.

@ericharding
Copy link
Author

Seems like it might just be the apt remove command returning a non-zero exit code. I tweaked the ci to match master. I'm not sure if it'll re-run the job automatically of if you have to kick it.

@erenon
Copy link
Contributor

erenon commented Apr 14, 2023

The issue seen in this ci run is already fixed in main. However, there's a new problem with clang 14 on Linux that affects the hiperf branch. No resolution yet.

@erenon
Copy link
Contributor

erenon commented Apr 18, 2023

lld bug filed: llvm/llvm-project#62209

@erenon
Copy link
Contributor

erenon commented Apr 18, 2023

So lld uses --no-apply-dynamic-relocs, that means, instead of setting the pointers in the .binlog.esrc section, it creates relocations, that'll be applied during runtime, when the object is loaded. However, the current linux code still reads the section from the disk, not from memory -- I'll need to change that (perhaps by using dl_iterate_phdr).

@erenon
Copy link
Contributor

erenon commented Apr 20, 2023

Fixed in and superseded by #165

@spektrof
Copy link

#165 is now merged.

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.

3 participants