Skip to content

[UR] added filename and line number to logs #17684

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

Merged
merged 39 commits into from
May 8, 2025
Merged

Conversation

mateuszpn
Copy link
Contributor

@mateuszpn mateuszpn commented Mar 27, 2025

Added an option to print line number and filename in log file. When fileline:1 is specified in an environment variable describing log, e.g:

UR_LOG_LEVEL_ZERO="level:debug;flush:warning;fileline:1;output:file,juju-l0.log"

then every log entry will have source file and line added, like below:

ZE ---> zeInit(L0InitFlags) </home/user/llvm/unified-runtime/source/adapters/level_zero/common.cpp:141>

The significant changes are contained in the following files:

In the remaining files, the calls to the logger functions are replaced by macros (basically automatic change)

Co-authored-by: Łukasz Ślusarczyk <[email protected]>
Co-authored-by: Mateusz P. Nowak <[email protected]>
@mateuszpn mateuszpn marked this pull request as ready for review March 28, 2025 10:37
@mateuszpn mateuszpn requested review from a team as code owners March 28, 2025 10:37
Comment on lines 24 to 25
URLOG(ERR, "Not Implemented : {} - File : {} / Line : {}", __FUNCTION__, \
__FILE__, __LINE__); \
Copy link
Contributor

Choose a reason for hiding this comment

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

If the idea is that URLOG already includes the file name and line number, __FILE__ and __LINE__ should be removed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't quite look right now, if __FILE__ and __LINE__ are removed without also updating the format string to match, this cannot work, can it?

Copy link
Contributor

@lslusarczyk lslusarczyk Apr 1, 2025

Choose a reason for hiding this comment

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

I think double FILE LINE is fine if one will set fileline:1, because

  • default fileline is 0
  • FILE from URLOG shortens path relatively to root, while FILE is a full path and one may want to 'grep' over each of them
  • URLOG looks shorter in code and comply new standard of writing log statements

I would leave original code

    URLOG(ERR, "Not Implemented : {} - File : {} / Line : {}", __FUNCTION__,   \
          __FILE__, __LINE__);

@hvdijk ,
do we agree on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, File : {} / Line : {} was added to address a deficiency in the available logging, now that the standard logging will have it, there is no longer a need for it. If we want the ability to log full paths (I'm not sure that we do), we should implement that in URLOG, not work around it everywhere we use URLOG.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. OK. @mateuszpn , please just fix format string too (remove excesive {}).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I meant, it was my mistake to keep the braces. I am now running additional tests to find more problems like this.

@mateuszpn
Copy link
Contributor Author

@ldrumm

Put the full paths in the logs without trimming?

Yes

Done, please re-review

@mateuszpn mateuszpn requested a review from ldrumm April 30, 2025 09:24
@mateuszpn mateuszpn requested review from ProGTX and JackAKirk April 30, 2025 13:14
@mateuszpn
Copy link
Contributor Author

@intel/bindless-images-reviewers please review

@lslusarczyk
Copy link
Contributor

@intel/llvm-gatekeepers , could you please merge this ? It seems this PR got finally last approvals needed.

@kbenzie kbenzie merged commit 4ef071d into intel:sycl May 8, 2025
34 checks passed
@kbenzie
Copy link
Contributor

kbenzie commented May 8, 2025

This has caused post commit linux build and windows build failures. @mateuszpn is aware and working on a fix.

mateuszpn added a commit to mateuszpn/llvm that referenced this pull request May 8, 2025
mateuszpn added a commit to mateuszpn/llvm that referenced this pull request May 8, 2025
aelovikov-intel pushed a commit that referenced this pull request May 8, 2025
Fix of compilation errors caused by merge of
#17684
github-actions bot pushed a commit to oneapi-src/unified-runtime that referenced this pull request May 9, 2025
Fix of compilation errors caused by merge of
intel/llvm#17684
kbenzie pushed a commit to oneapi-src/unified-runtime that referenced this pull request May 9, 2025
Fix of compilation errors caused by merge of
intel/llvm#17684
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.