search

morganstanley / binlog

A high performance C++ log library, producing structured binary logs
http://opensource.morganstanley.com/binlog/
Apache License 2.0
608 stars 72 forks source link

String may be the first string in a section #164

Open ericharding opened 1 year ago

ericharding commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Thanks

erenon commented 1 year ago

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

ericharding commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

lld bug filed: https://github.com/llvm/llvm-project/issues/62209

erenon commented 1 year ago

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 commented 1 year ago

Fixed in and superseded by https://github.com/morganstanley/binlog/pull/165

spektrof commented 1 year ago

165 is now merged.