Add environment var as input for SSA CFG debug log level by matheusaaguiar · Pull Request #16581 · argotorg/solidity

matheusaaguiar · 2026-04-07T12:43:47Z

No description provided.

nikola-matic · 2026-04-07T14:45:23Z

solc/CommandLineParser.cpp

 		m_options.logLevel = *level;
 	}
+	// Check if env VAR is defined
+	else if(char const* levelValue = std::getenv("SOLC_LOG_LEVEL"))


Just curious - why an environment variable instead of doing this via cmake? Are we planning for this to be available to the end users as well (although in that case an actual CLI option would make more sense)?

Leaving this here for context: #16540 (comment)

TLDR if we do it via cmake and macro it's super easy to get drift. Compiling it in allows us to simply turn it on if we need it on any version if there's a problem somewhere. With the forced no-inline I think we have performance that is close to having it compiled out, so why not leave it in :)
As for cli arg vs environment var: Since the thing should be configurable down to components and individual levels, it's not entirely trivial (although Daniel might say so) and I'm sure Kamil has opinions on it. So env var seemed like the easiest for now to enable it for ssa cfg where it's currently needed.

Sure, makes sense.

So env var seemed like the easiest for now to enable it for ssa cfg where it's currently needed.

You already have both here though. I'm not sure we really need the var. It's just a sneaky way to introduce a global without declaring one :) A bit pointless if you need a singleton for the registry anyway.

thanks, I am acutely aware that we have both here :) not so sure about sneaky or not, it seemed to me there is some discussion to be had about having a proper cli concept for logging. to sidestep that for the time being, an environment variable could be used. or making the cli experimental. or just a bool flag somewhere in the code. or a cmake option. i don't care what it is in the end as long as it enables me to get some logging output if needed.

stackenbotten3000 · 2026-04-08T01:15:49Z

libsolutil/logging.cpp

+		bool matched = false;
+		for (auto const& [prefix, level] : presets) {
+			if (prefix.empty() || name == prefix || name.starts_with(prefix + ".")) {
+				if (!matched || prefix.size() >= best_match) {


Coding style error

cameel · 2026-04-08T19:52:56Z

solc/CommandLineInterface.cpp

 	m_options = parser.options();

+	if (m_options.logLevel)
+		Registry::instance().set_level("", *m_options.logLevel);


Registry of what? Why doesn't it have a more descriptive name?

because it's a draft. but yeah, should def be changed.

cameel · 2026-04-08T19:55:34Z

solc/CommandLineParser.cpp

+			solThrow(
+				CommandLineValidationError,
+				"Invalid option for --" + g_strLogLevel + ": " + levelStr +
+				". Valid options: trace, debug, info, warn, error, off."


There should be some way to get a list of those to avoid hard-coding them in multiple places.

cameel · 2026-04-08T20:00:11Z

solc/CommandLineParser.cpp


 	m_options.formatting.withErrorIds = m_args.count(g_strErrorIds);

+	if (m_args.contains(g_strLogLevel))


This should be processed way earlier. The values of some the options might actually be something we might want to log at some point.

cameel · 2026-04-08T20:08:26Z

libsolutil/logging.h

There's lot of stuff here that does not follow our style. Is it copied from somewhere?

Braces not on a new line

Abbreviated or even single-letter variable names

No _ prefix in function parameters

Whole enums squished into a single line

Function names with underscores rather than camel case

The file name starts with a lowercase letter (logging.cpp)

Tons of auto even where using the type directly is straightforward

No SPDX comment (or copyright boilerplace, though TBH I'd be glad to get rid of it at some point)

do you think it is productive to drive-by review this PR at that level especially considering in the state it is in right now? while it is being in draft and clearly not in a finalized state?

Sorry, but SSA CFG-related PRs tend to get merged very quickly, sometimes before I manage to even take a look at them so I learned that I have to act fast and leave a comment if something catches my eye. This bit wasn't my focus here, but it stood out and it wasn't very clear to me how close to completion this PR really was as I only skimmed through parts of it and it has no description (is it a normal initial version or just a messy final version?) so I decided it's better to comment on it regardless. Note that I intentionally wrapped it into a single comment rather than flood you with comments on every individual thing.

cameel · 2026-04-08T20:21:37Z

libyul/backends/evm/ssa/StackLayoutGenerator.cpp

+solidity::Logger const& log()
+{
+	static solidity::Logger const& instance = solidity::Registry::instance().get("yul.ssa.stacklayout");
+	return instance;
+}


Maybe we should have a macro for automatically declaring something like this?

DEFINE_LOGGER(log, "yul.ssa.stacklayout");

This is something that we'll be doing in almost every file eventually, so it would be best to be able to do this without much boilerplate.

Right. Or a static helper function. I am not particularly fond of macros and try to avoid them if possible. :)

cameel · 2026-04-08T20:24:16Z

libyul/backends/evm/ssa/CodeTransform.cpp

+	log().debug("--------------------\n");
+	log().debug("Running SSA CFG code transform\n");
+	log().debug("--------------------\n");


I think that a newline at the end should be the default. We'll want it in the vast majority of cases. For those few were we don't we could have a parameter to disable it or even just an alternative function.

yes I agree and this code is still extremely messy and not cleaned up. we should also add prefixes to the logs. perhaps ansi colors if we want to be fancy. but why don't we follow an incremental approach. default-adding newline at the end seems like a good thing to have for the initial draft though.

Sorry for jumping into it already. I did not realize how early of a draft this is meant to be. My bad.

Incremental approach is fine as long as it's clear it's incremental. What I'm really missing here is at least a short comment in the description about the intended scope and current status of this.

But overall I would have really preferred if this started with an issue or a design note so that the higher-level details can be hashed out separately from the implementation. There are things we can easily agree on at that level without being distracted by the unfinished details. Starting with the code actually always makes me more anxious because I know I have to fish out the things I care about from the PR and I have to be constantly on guard not to miss them. IMO incremental should mean that we create at least a rough design and implement it one step at a time, not that we incrementally design and implement as we go. The latter does not lead to a smooth review.

No worries at all and I'm sorry you were at the receiving end of my venty replies. A description to the PR really would have helped. We ought to do better there.
Regarding design discussion, I thought we were in agreement on how it should look like - at least roughly - when you brought up logging here #16540 (comment). If you think it helps we can set up an issue or so to track progress and discuss it more :)

If you think it helps we can set up an issue or so to track progress and discuss it more :)

What I'd really like to have is a separate artifact that constrains the implementation, describing what we're trying to achieve, why and the high-level decisions, like interfaces, backwards-compatibility, general structure. It could be an issue, but I feel that these usually encourage thinking about steps or tasks to be checked off, while what I'd prefer to end up with is more like a spec or docs.

That discussion we had is a good basis for that, but it lacked details I'd expect to see in such a design note (e.g. the specific logging levels, how we're changing the CLI/Standard JSON or how we'll make logging macros/functions ergonomic). I could have continued it, but it already felt like it was too detailed for a comment thread tucked away in a PR :) It's hard to iterate on it in that format.

Now that I think of it, maybe some day we could just feed these notes to an LLM and get an automatic review of the implementation based on that :)

Hehe. As long as it's not a mandatory quality gate and we have to bow to our AI overlords. :D

matheusaaguiar requested a review from clonker April 7, 2026 12:43

nikola-matic reviewed Apr 7, 2026

View reviewed changes

matheusaaguiar force-pushed the ssa-cfg-debug-stdout-logging-env-var branch from b8e744b to 55a795d Compare April 8, 2026 01:14

stackenbotten3000 reviewed Apr 8, 2026

View reviewed changes

matheusaaguiar changed the base branch from ssa-cfg-debug-stdout-logging to develop April 8, 2026 01:17

clonker and others added 10 commits April 8, 2026 10:20

add logging to libsolutil

885c2fe

logging

ad2c681

f

234b252

logging for ssa code transform

2bdfb2c

logging for ssa stack layout generator

a8facb2

fixup! logging for ssa stack

06c05f1

Add environment var as possible input for log level

4065c17

Update tests

6869430

logging cli

c10d465

[WIP] test expectation missing operations

e7e1338

matheusaaguiar force-pushed the ssa-cfg-debug-stdout-logging-env-var branch from 55a795d to e7e1338 Compare April 8, 2026 18:12


		m_options.formatting.withErrorIds = m_args.count(g_strErrorIds);

		if (m_args.contains(g_strLogLevel))

Conversation

matheusaaguiar commented Apr 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clonker Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clonker Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clonker Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clonker Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cameel Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

clonker Apr 9, 2026 •

edited

Loading

clonker Apr 9, 2026 •

edited

Loading

clonker Apr 9, 2026 •

edited

Loading

cameel Apr 9, 2026 •

edited

Loading

clonker Apr 9, 2026 •

edited

Loading

cameel Apr 9, 2026 •

edited

Loading

cameel Apr 9, 2026 •

edited

Loading