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

[analyzer] Include arch subdirectories in LD_LIBRARY_PATH for logging #4394

Merged
merged 5 commits into from
Dec 10, 2024

Conversation

gamesh411
Copy link
Collaborator

@gamesh411 gamesh411 commented Dec 2, 2024

The issue with logging (see #4360 for details) came up on my Ubuntu24 machine, where only specifying the <data_files>/ldlogger/lib in the LD_LIBRARY_PATH was not enough for the dynamic loader to load the ldlogger.so inside <data_files>/ldlogger/lib/x86_64, so I observed there is no implicit architecture resolution in the logic of the loader. There were other tests on other distributions where this implicit architecture resolution seems to exist (rhel7, rhel8, ubuntu22, among others).

The solution implemented by this patch is adding all <data_files>/ldlogger/lib/* folders in addition to the current <data_files>/ldlogger/lib folder to the LD_LIBRARY_PATH.

I have tested it on Ubuntu24, and the multiple .so files that are possibly reachable due to all of them being added are not an issue. The logger works as expected, and if I remove only the relevant ldlogger.so (x86_64 for my machine) but keep the others, the CodeChecker log command correctly detects that I meet neither the ld-logger nor the intercept-build requirement.

@dkrupp
Copy link
Member

dkrupp commented Dec 3, 2024

sounds good. did you test this on older distros? (ubuntu 20, 22,rhel8,rhel9)

@dkrupp
Copy link
Member

dkrupp commented Dec 5, 2024

You may want to try out this implmentation on the following test

test.c:

int main(void){
  return 0;
}

builder.c:

#include<stdlib.h>
int main(void){
  const char* command="gcc ./test.c";
  system(command);
  return 0;
}

gcc -m32 ./builder.c -o builder32

CodeChecker log -b ./builder32 -o compile_command32.json

gcc ./builder.c -o builder64

CodeChecker log -b ./builder64 -o compile_command64.json

both compile_commands.json should be valid.

The architecture (32 or 64 bits) of the caller of the gcc what matters.

@gamesh411
Copy link
Collaborator Author

Hi, I have tested with the suggested method, and both logs have the correct content.
I tested the above approach but modified the builder to make it even more convoluted like this:

#include<stdlib.h>
int main(void) {
  system("/path/to/32bitcompiler/bin/clang -o test_build_with_clang32 test.c");
  system("/path/to/64bitcompiler/bin/clang -o test_build_with_clang64 test.c");
  return 0;
}

Then, build this to builder32 and builder64, and log them like this:

strace -fte trace=%process,%file CodeChecker log -b './builder32' -o ccdb32.json 2>&1 | tee strace_log32
strace -fte trace=%process,%file CodeChecker log -b './builder64' -o ccdb64.json 2>&1 | tee strace_log64

Then, the contents of BOTH ccdb32.json and ccdb64.json:

[
	{
		"directory": "...",
		"command": "/path/to/32bitcompiler/bin/clang -o test_build_with_clang32 test.c",
		"file": "test.c"
	}
	,
	{
		"directory": "...",
		"command": "/path/to/64bitcompiler/bin/clang -o test_build_with_clang64 test.c",
		"file": "test.c"
	}
]

All in all, I see this is the expected behaviour.

@gamesh411 gamesh411 marked this pull request as ready for review December 5, 2024 15:20
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

I like the proposed solution and I tested and it works on ubuntu 22 too
I think we can remove the i486, i586, i686 subdirs of ld_logger/lib
and just keep i386 dir.

Also could you please add the testcase which I proposed in my comment as functional test?

analyzer/codechecker_analyzer/analyzer_context.py Outdated Show resolved Hide resolved
@gamesh411 gamesh411 force-pushed the include-all-archs-in-ldlibrarypath branch from 6d0cdd6 to 7620288 Compare December 6, 2024 14:54
@gamesh411 gamesh411 force-pushed the include-all-archs-in-ldlibrarypath branch from 7620288 to 4817933 Compare December 6, 2024 18:02
@gamesh411
Copy link
Collaborator Author

@dkrupp Thanks for the suggestions. I implemented and verified all of them on Ubuntu24 and this Ubuntu20 test environment.

@gamesh411 gamesh411 requested a review from dkrupp December 6, 2024 18:22
Copy link
Member

@dkrupp dkrupp left a comment

Choose a reason for hiding this comment

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

LGTM

@dkrupp dkrupp merged commit 2f1aa32 into Ericsson:master Dec 10, 2024
8 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