This is an archive of the discontinued LLVM Phabricator instance.

[Flang] -funderscoring bug fix
ClosedPublic

Authored by madanial on Jul 11 2023, 11:19 PM.

Details

Summary

There was a bug with the -funderscoring / -fno-underscoring options from (https://reviews.llvm.org/D140795) that prevented the driver option from controlling the underscoring behaviour and instead the behaviour could only be controlled by the pass option instead of the driver option. The driver test case did not catch the bug and also needed to be updated.

Diff Detail

Event Timeline

madanial created this revision.Jul 11 2023, 11:19 PM
madanial requested review of this revision.Jul 11 2023, 11:19 PM

Thanks for the fix!

flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp
146
161

What's the difference between appendUnderscore and appendUnderscores? Perhaps you could introduce more descriptive names?

flang/test/Driver/underscoring.f90
12–13

You could add this (and similar checks) to be stricter.

19–20

IIUC, this form does not appear in any of the cases? Then I would not include it - to reduce noise.

madanial updated this revision to Diff 539840.Jul 12 2023, 9:54 PM

Thanks for the review, addressing review comments.

madanial marked 4 inline comments as done.Jul 12 2023, 9:54 PM
awarzynski accepted this revision.Jul 13 2023, 7:27 AM

LGTM, thanks!

This revision is now accepted and ready to land.Jul 13 2023, 7:27 AM
This revision was automatically updated to reflect the committed changes.

FYI I just got this test failing on an unrelated change: https://lab.llvm.org/buildbot/#/builders/21/builds/78228
An earlier example that had none of my changes: https://lab.llvm.org/buildbot/#/builders/21/builds/78223

Not sure that it's this change in particular, but it's the most recent thing to point at :)

I looked briefly at Linaro's flang bots and don't see this happening. Would be worth scanning through the driver option for any uninitialised variables, maybe building with UBSAN (I don't know if there is a bot that does that for flang).

DavidSpickett added inline comments.Aug 15 2023, 10:24 AM
flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp
161

This changes the member variable, is that intentional? I guess that it doesn't make any difference unless we re-use the pass object, but it does look a bit strange.

usePassOpt looks to always be true, but I must be missing something there.

madanial added inline comments.Aug 16 2023, 8:38 AM
flang/lib/Optimizer/Transforms/ExternalNameConversion.cpp
161

Thanks for looking into this. The ExternalNameConversion pass is controlled by 2 options a pass option for running with tco and fir-opt and a driver option for flang-new. I think the intermittent failure in the test is caused by the uninitialization of usePassOpt, working on confirming that at the moment.