-
Notifications
You must be signed in to change notification settings - Fork 106
[Comgr] Create a Global Logger Class for Comgr #3116
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
base: amd-staging
Are you sure you want to change the base?
Changes from 3 commits
39d8e5e
678d708
cb37cdf
a72673a
a7623d9
983e6d5
ec8cb9f
c162e90
7a3a133
b426a0d
3daf40e
17af8a8
bcaa0fe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -84,6 +84,11 @@ bool shouldEmitVerboseLogs() { | |
| return VerboseLogs && StringRef(VerboseLogs) != "0"; | ||
| } | ||
|
|
||
| StringRef getLogLevel() { | ||
| static const char *LogLevel = getenv("AMD_COMGR_LOG_LEVEL"); | ||
| return LogLevel ? StringRef(LogLevel) : StringRef(); | ||
|
Comment on lines
+88
to
+89
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably want to be case-insensitive here ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, whats the rationale behind using strings over numbers to denote severity ? I would lean more to using numbers since thats what we currently do in runtime. Its also easier to think about IMO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Internally Comgr can map an integer to a severity level. But externally I think we should just expose them as numbers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the case insensitivity is handled in comgr-logger.cpp, are you saying to move that the case lower to here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose the sensitivity discussion is irrelevant if we're exposing numbers instead
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh sorry, when I posted that comment, I didn't see the more recent ones. Personally, I think that these labels make it more clear for both assigning LogLevels to a particular output and for toggling the threshold for what logs a user would like to see. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I disagree. There's a few reasons:
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That makes sense, thanks for the clarification. I'll do a 0-20 level logging and let the users have autonomy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We dont want to do However, we do want to perform numeric comparison rather than equality when checking against the parsed value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LogLevel should be an enum as @lamb-j mentioned. This function should return that enum There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check how Sam is trying to do something similar at #2478
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just to confirm, this means to revert LogLevel away from using numbers (suggested here #3116 (comment)) and back to using the original None, Error, Warning, Debug, Info enum, correct? |
||
| } | ||
|
|
||
| llvm::StringRef getLLVMPath() { | ||
| static const char *EnvLLVMPath = std::getenv("LLVM_PATH"); | ||
| return EnvLLVMPath; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,156 @@ | ||
| //===- comgr-logger.cpp - Global Comgr logging facility -------------------===// | ||
| // | ||
| // Part of Comgr, under the Apache License v2.0 with LLVM Exceptions. See | ||
| // amd/comgr/LICENSE.TXT in this repository for license information. | ||
| // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
| // | ||
| //===----------------------------------------------------------------------===// | ||
| /// | ||
| /// \file | ||
| /// This file implements COMGR::Logger. See comgr-logger.h for the design. | ||
| /// | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| #include "comgr-logger.h" | ||
| #include "comgr-env.h" | ||
|
|
||
| #include "llvm/ADT/SmallString.h" | ||
| #include "llvm/ADT/StringRef.h" | ||
| #include "llvm/Support/FileSystem.h" | ||
|
|
||
| using namespace llvm; | ||
|
|
||
| namespace COMGR { | ||
|
|
||
| namespace { | ||
|
|
||
| // The capture stream is per-thread so that a captured Action on one thread does | ||
| // not collect log output emitted by an unrelated API on another thread. | ||
| thread_local raw_ostream *ThreadCaptureStream = nullptr; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comgr doesnt support multithreaded execution yet. Do we need
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comgr can be run in multiple threads and is thread safe, via a mutex around LLVM calls that cause issues |
||
|
|
||
| // Resolve the configured level from the environment. Delegates the mapping to | ||
| // the testable parseLogLevel(); kept separate so the Logger constructor reads | ||
| // the (cached) environment exactly once. | ||
| LogLevel resolveLevel() { | ||
| return parseLogLevel(env::getLogLevel(), env::shouldEmitVerboseLogs()); | ||
| } | ||
|
|
||
| StringRef severityPrefix(LogLevel Severity) { | ||
| switch (Severity) { | ||
|
MixedMatched marked this conversation as resolved.
Outdated
|
||
| case LogLevel::Error: | ||
| return "comgr: error: "; | ||
| case LogLevel::Warning: | ||
| return "comgr: warning: "; | ||
| case LogLevel::Info: | ||
| return "comgr: info: "; | ||
| case LogLevel::Debug: | ||
| return "comgr: debug: "; | ||
| } | ||
| return "comgr: "; | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| LogLevel parseLogLevel(StringRef Requested, bool VerboseFallback) { | ||
| // When the variable is unset or unrecognized, default to Debug if verbose | ||
| // logs are requested for back-compat with AMD_COMGR_EMIT_VERBOSE_LOGS, | ||
| // otherwise to Error. An explicit, recognized value always wins (including | ||
| // "none", which silences logging even when verbose logs are requested). | ||
| LogLevel Fallback = VerboseFallback ? LogLevel::Debug : LogLevel::Error; | ||
| if (Requested.empty()) | ||
| return Fallback; | ||
|
|
||
| if (Requested.equals_insensitive("none")) | ||
|
MixedMatched marked this conversation as resolved.
Outdated
|
||
| return LogLevel::None; | ||
| if (Requested.equals_insensitive("error")) | ||
| return LogLevel::Error; | ||
| if (Requested.equals_insensitive("warning")) | ||
| return LogLevel::Warning; | ||
| if (Requested.equals_insensitive("info")) | ||
| return LogLevel::Info; | ||
| if (Requested.equals_insensitive("debug")) | ||
| return LogLevel::Debug; | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Commented out code should be removed |
||
| return Fallback; | ||
| } | ||
|
|
||
| Logger::Logger() : Level(resolveLevel()), Sink(nullptr) { | ||
| std::optional<StringRef> RedirectLogs = env::getRedirectLogs(); | ||
| if (!RedirectLogs) | ||
| return; | ||
|
|
||
| StringRef RedirectLog = *RedirectLogs; | ||
| if (RedirectLog == "stdout" || RedirectLog == "-") { | ||
| Sink = &outs(); | ||
| } else if (RedirectLog == "stderr") { | ||
| Sink = &errs(); | ||
| } else { | ||
| std::error_code EC; | ||
| SinkFile = std::make_unique<raw_fd_ostream>( | ||
| RedirectLog, EC, sys::fs::OF_Text | sys::fs::OF_Append); | ||
| if (EC) { | ||
| SinkFile.reset(); | ||
| // Record the failure rather than writing it to stderr here. The Logger is | ||
| // constructed before any action's log buffer exists; the action layer | ||
| // surfaces this message into the returned comgr.log via getSinkError(), | ||
| // restoring the pre-Logger behavior of reporting it to the caller. | ||
|
Comment on lines
+69
to
+72
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shorten? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 need to shorten more |
||
| SinkError = (Twine("unable to redirect log to file '") + RedirectLog + | ||
| "': " + EC.message()) | ||
| .str(); | ||
| } else { | ||
| Sink = SinkFile.get(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Logger::Logger(LogLevel Level, raw_ostream *Sink) : Level(Level), Sink(Sink) {} | ||
|
|
||
| void Logger::writeToSink(StringRef Data) { | ||
| if (!Sink) | ||
| return; | ||
|
|
||
| // Share the same mutex as emit() so teed output and Logger messages never | ||
| // interleave mid-write on the shared sink. The sink is intentionally left | ||
| // unflushed here; the caller flushes once when the action completes. | ||
| std::scoped_lock<std::mutex> Lock(Mutex); | ||
|
theSK2005 marked this conversation as resolved.
|
||
| *Sink << Data; | ||
| } | ||
|
|
||
| void Logger::emit(LogLevel Severity, const Twine &Message) { | ||
| if (!isEnabled(Severity)) | ||
| return; | ||
|
|
||
| SmallString<256> Buffer; | ||
| StringRef Text = Message.toStringRef(Buffer); | ||
| StringRef Prefix = severityPrefix(Severity); | ||
|
|
||
| std::scoped_lock<std::mutex> Lock(Mutex); | ||
|
|
||
| raw_ostream *Capture = ThreadCaptureStream; | ||
| if (Sink) { | ||
| *Sink << Prefix << Text << '\n'; | ||
| Sink->flush(); | ||
| } | ||
| // Guard against double-emission if a capture stream happens to alias the | ||
| // sink. | ||
| if (Capture && Capture != Sink) { | ||
| *Capture << Prefix << Text << '\n'; | ||
| Capture->flush(); | ||
| } | ||
| } | ||
|
|
||
| Logger &getLogger() { | ||
| static Logger TheLogger; | ||
| return TheLogger; | ||
| } | ||
|
|
||
| raw_ostream *getThreadCaptureStream() { return ThreadCaptureStream; } | ||
|
|
||
| LogCaptureScope::LogCaptureScope(raw_ostream &OS) | ||
| : Previous(ThreadCaptureStream) { | ||
| ThreadCaptureStream = &OS; | ||
| } | ||
|
|
||
| LogCaptureScope::~LogCaptureScope() { ThreadCaptureStream = Previous; } | ||
|
|
||
| } // namespace COMGR | ||
Uh oh!
There was an error while loading. Please reload this page.