Page MenuHomePhabricator

[remark][diagnostics] [codegen] Fix PR44896
ClosedPublic

Authored by xur on Feb 19 2020, 3:33 PM.

Details

Summary

This patch fixes PR44896.

For IR input files, option fdiscard-value-names should be ignored as we need named values in loadModule().
Commit 60d3947922 sets this option after loadModule() where valued names already created. This creates an inconsistent state in setNameImpl() that leads to a seg fault.
This patch forces fdiscard-value-names to be false for IR input files.

Diff Detail

Event Timeline

xur created this revision.Feb 19 2020, 3:33 PM
hans added a subscriber: hans.Feb 20 2020, 5:03 AM

See also https://reviews.llvm.org/D74871
I don't know which is the better fix here.

This seems like the reasonable fix to me.

@lebedev.ri maybe we should at least warn the user if there was a flag at clang level to force discarding? That's what I've been doing in https://reviews.llvm.org/D74871?id=245594

xur added a comment.Feb 20 2020, 9:08 AM

@lebedev.ri maybe we should at least warn the user if there was a flag at clang level to force discarding? That's what I've been doing in https://reviews.llvm.org/D74871?id=245594

@serge-sans-paille
It seems to me that we are not dropping the flag of -fdiscard-value-names. It's just the implementation of setNameImpl() that assumes the names would not be generated under this option.
In this sense, your patch is more complete as it breaks this removes this assumption.

My patch will fall back to the old behavior before Commit 60d3947922.

It seems to me that we are not dropping the flag of -fdiscard-value-names.

I also reads this as overriding a cc1 flag / ignoring the flag, I don't know if we consistently warn in such cases though.

Overall LGTM for the fix, pending resolution for the warning, thanks!

Let's go that way then. @xur I leave it to you to add the appropriate warning at least on the clang side? Feel free to just use the code from https://reviews.llvm.org/D74871?id=245594.

xur updated this revision to Diff 245891.Feb 21 2020, 9:20 AM
xur added reviewers: mehdi_amini, serge-sans-paille.

Update the patch based on the reviews from Mehdi and Serge.
Reuse Sege's warning code in https://reviews.llvm.org/D74871?id=245594.

xur added a comment.Feb 24 2020, 8:16 AM

Gentle ping. Is the newest patch ok?

Will this also give a warning when passing a .ll file to a release clang, without explicitly passing the '-fdiscard-value-names' ? Is this what we want it to be ?

Will this also give a warning when passing a .ll file to a release clang, without explicitly passing the '-fdiscard-value-names' ? Is this what we want it to be ?

I think it does. @xur, we should probably explicitly check for -fdiscard-value-names before raising the warning (?)

xur updated this revision to Diff 246260.Feb 24 2020, 11:02 AM

check if the -fdiscard-value-names explicitly in args (suggested by Jeroen and Serge)

xur updated this revision to Diff 246265.Feb 24 2020, 11:16 AM

Uploaded a wrong patch in the last time.
This is the correct one.

Looks good to me.

This revision is now accepted and ready to land.Feb 25 2020, 3:05 AM

Nice work, LGTM too.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2020, 8:17 AM