Use semantic analysis in lint-logs.py

vasild commented at 11:26 am on June 5, 2023: contributor

Please describe the feature you’d like to see added.

Currently test/lint/lint-logs.py tries to parse the C++ manually which is prone to flaws. For example it does not support expressions that span more than one line and would be happy if there is \n" anywhere (on the single line it checks). For example:

0        LogPrintLevel(BCLog::PROXY, 
1                      BCLog::Level::Debug, 
2                      "This is fine, but lint-logs.py will report error\n");

0        LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "Not ok\n"
1                      " but will be reported as ok");

0        LogPrintLevel(BCLog::PROXY, BCLog::Level::Debug, "even more /* Continued */ no newline, but is ok");

0        /* LogPrintLevel() inside comment causes an error */

lint-logs.py requires to either write everything on line line or add /* Continued */ comment.

Describe the solution you’d like

It should be possible to use libclang to properly parse the C++ code. Then the check should be something like:

0for each call to `LogPrintLevel()`:
1    get the third argument:
2        it should be a string
3        its last two chars should be `\` and `n`

Describe any alternatives you’ve considered

No response

Please leave any additional context

No response

vasild added the label Feature on Jun 5, 2023

theuni commented at 6:03 pm on June 9, 2023: member

Hah, I implemented this as a clang-tidy check a while back.

I’ll work on getting it hooked up.

theuni commented at 8:41 pm on June 12, 2023: member

Still working on getting the plugin cleaned up, but in the meantime it pointed out what seems to be a buggy case here. Will PR a trivial fix for that.

vasild commented at 5:10 am on June 15, 2023: contributor

That looks more native than the sh or py scripts, excellent!

theuni commented at 6:45 pm on June 16, 2023: member

Just wanted to document this somewhere before I forget…

I attempted to make an “ideal” plugin, which wouldn’t require us to hard-code parts of our code, but I ran into a few issues. @aaronballman was nice enough to explain the status quo to me on IRC:

<cfields> I’m working on a clang plugin that registers a custom attribute, and for parsing I’m finding myself copy/pasting several static functions from SemaDeclAttr.cpp (for example checkUInt32Argument). I’m now needing the equivalent of checkFunctionOrMethodParameterIndex, but that comes with lots of dependencies. Is there maybe something I’m missing? Some better supported api for checking attribute arguments from plugins? <AaronBallman> cfields: you’re not missing anything, attribute plugin support is relatively recent and so it still has quite a few sharp edges <cfields> AaronBallman: ah, ok, that alone is a huge hint. Thanks :) <AaronBallman> cfields: you’re welcome! We definitely could stand to make some of that stuff easier for folks writing plugins. <cfields> AaronBallman: anything I can do to help move it along? I’ve been waiting quite eager for this functionality so I’d be happy to contribute. .. <cfields> AaronBallman: for reference, my motivation: It would allow our codebase (bitcoin core) to use custom meaningfull annotations (the second parameter of this function must be of a certain subset of something something) as opposed to having plugins that contains hard-coded knowledge of our types/functions/etc. Imo the former is far more maintainable :) <AaronBallman> cfields: for this, I think it’s mostly a matter of figuring out where those functions should live and which ones to expose. They’re static to SemaDeclAttr.cpp and they might not all be reasonable to expose more widely (due to weird implementation details like how to index parameters, etc).

So I’ll just stick with the approach of hard-coding our function names for now.

theuni commented at 7:19 pm on July 10, 2023: member

See here for an example of this implemented as a clang plugin rather than clang-tidy. As noted above, it’s pretty rough because LLVM does not yet expose several useful helpers.

Advantages:

We can invent our own attributes, which is way nicer for future-proofing. See here for an example of the kind of synchronization problems we’ll have otherwise.
Use is a side-effect of typical compilation

Disadvantages:

More complicated to write
Jobs with a static-analysis clang-tidy step may be preferred

maflcko commented at 6:35 pm on July 11, 2023: member

See https://github.com/theuni/bitcoin-tidy-experiments/pull/1 for an example of the kind of synchronization problems we’ll have otherwise.

Wouldn’t it be easier to (have an option to) self-sanity-check the match? For example, assert that more than one result was found?

theuni commented at 10:49 pm on July 11, 2023: member

Sure. Now that I think about it, we could just keep|copy the test suite in Core rather than|in addition to the plugin repo.

vasild commented at 3:02 pm on August 17, 2023: contributor

Fixed by #26296, thanks!

vasild closed this on Aug 17, 2023

bitcoin locked this on Aug 16, 2024

Use semantic analysis in lint-logs.py #27825

Please describe the feature you’d like to see added.

Is your feature related to a problem, if so please describe it.

Describe the solution you’d like

Describe any alternatives you’ve considered

Please leave any additional context