[llvm-dev] "[NFC]" Abuse

[David Blaikie via llvm-dev]

On Fri, Jun 18, 2021 at 4:00 AM Luke Drummond <luke.drummond at codeplay.com>
wrote:

> On Thu Jun 17, 2021 at 6:15 PM BST, David Blaikie wrote:
> > Got links to the reviews? Hard to discuss in the abstract.
>
> I was more intent on general discussion rather than singling people out,
> but
> here these are things that have caused me to have to modify our downstream
> users due to crashes or failed builds this week (these patches were not not
> committed in the last week, but I'm a bit behind):
>   1. [bc7d15c61da7](https://reviews.llvm.org/D101713) changed the format
> of the
>     IR and causes crashes during inlining. Yesterday tanother user
> commented on
>     this one on Phab saying it broke their downstream.
>

Fair point there - if a patch touches both production and test code that's
a pretty good sign it's not NFC. I should've caught that in review.


>   2. [c83cd8feef7e](https://reviews.llvm.org/D9918l2) changed the order of
>     parameters for a public function
>

(slight typo in the URL, here's a good one: https://reviews.llvm.org/D99182
)

LLVM doesn't really have a concept of a "public" API - as Nikita and Mehdi
have touched on - this sort of change is the bread and butter of NFC
changes in LLVM - refactoring APIs (renaming, adding/removing parameters,
etc) and simultaneously updating all callers in such a way that no
observable change in the LLVM command line utilities can be observed.


>   3. [738abfdbea2](https://github.com/llvm/llvm-project/commit/738abfdbea2
> )
>     Apparent followup to (1). No code review linked. Reverted this morning.
>

Looks like a reasonable "I thought this was NFC/that the invariant was
already enforced elsewhere so the checking would be redundant here" and it
didn't turn out to be true.

This change looks in line with what Arthur should be committing with
post-commit review. The opaque pointers work is going to be a lot of
cleanup like this and I'm happy to review it post-commit in many cases like
these small targeted changes - not sure if there's more scope for testing
that might've identified the reason it broke things in advance of
committing the change.

Perhaps it is NFC, if (1) stuck (regardless of whether (1) was classified
as NFC), I haven't looked closely at whether it was reverted for the patch
itself, or because it needed to be backed out if (1) was backed out.


> > But generally, if it is intended to change observable behavior of the
> LLVM
> > program (if you have to modify a lit test, that's not NFC, if one could
> craft
> > a test (that doesn't invoke UB, though that shouldn't be possible
> through the
> > command line interface, etc) that would need to be modified when the
> patch is
> > committed - then it's not NFC).
>
> I appreciate that downstream consumers aren't afforded the same
> expectations of
> API and ABI stability as an old-fashioned C library users, but it's not
> just the
> function of the programs like `opt` that is the set of all things
> functional:
> the API and the ABI are too.
>

I don't think that's how the LLVM project should/is going to classify
"functional change" - in part because there isn't a clear "public" API
boundary - there are wide interfaces across the whole project that external
users can call into.

We could introduce a separate term but I think most NFC patches would use
that term instead, so it probably wouldn't amount to much real change -
we'd end up using that new term ubiquitously instead.

> But I think it's important that NFC is about intent, not about the
> reality of
> > what the patch ends up doing - so we can't judge an NFC patch by whether
> it
> > causes a regression later - the NFC marker is there to say "I don't
> intend
> > this to cause any observable change, if you observe any change, that's a
> bug"
> > in the same way any patch description is a statement of intent and can't
> be
> > more than that.
> Sure. Nobody's perfect, and I'm not saying that *in-general* it's possible
> to
> prove that *any* change - however small - doesn't affect the observable
> behaviour of the program. But if we're just going to wave the NFC flag
> willy-nilly it completely loses its meaning.
>

I'm not proposing waving it around will-nilly, I think I (& others) have
described a fairly consistent understanding of the term.


> Since Jun 1st, there were ~130 commits in the llvm/ directory marked with
> /\bNFC\b/. Many of them are simply formatting or refactoring of the bodies
> of
> functions to clarify the codebase - and this seems appropriate usage to
> me. Some
> of them, however, change the observable program semantics: one removes a
> command
> line flag (breaks people's use of the llvm "program");


Sure, I probably would agree that shouldn't be NFC.


> and one changes a public
> function return type.


I'm generally OK with that being marked NFC - as a few folks have said on
this thread, cross-library API refactorings are generally understood to be
"NFC" in the way the project uses the term.


> Both these examples have the potential to break someone's
> use of llvm. Yet another is a revert, which suggests to me that the use of
> NFC
> in the original commit might have been less than judicious.
>

That the patch was reverted doesn't necessarily mean the original commit
was a problem - looks like it was reverted because a preceeding patch was
reverted that meant the follow on patch needed to be reverted too, without
that follow on patch necessarily being to blame.

If I'm debugging a new crash, build failure, or codegen change in my
> downstream,
>

Build failures (if you mean like Clang no longer compiling some code it did
before - not "a previously compiling use of LLVM libraries now doesn't
compile anymore") and codegen changes (if you mean LLVM produces different
IR/machine code for the same source code - not LLVM itself builds to a
different binary file) shouldn't be marked NFC - that's sort of the core of
the function of LLVM and is the definition of a functional change.

Crashes - they happen.


> it'd be nice if I could ignore messages marked "NFC" when trying to find
> the
> commit that introduced the new behaviour. That's *my* benchmark for
> non-functional.
>
> All the Best
>
> Luke
> --
> Codeplay Software Ltd.
> Company registered in England and Wales, number: 04567874
> Registered office: Regent House, 316 Beulah Hill, London, SE19 3HF
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210618/ddba9f78/attachment.html>