This is an archive of the discontinued LLVM Phabricator instance.

Add Clang driver flags equivalent to cl's /MD, /MT, /MDd, /MTd.
ClosedPublic

Authored by akhuang on Sep 7 2022, 2:58 PM.

Details

Summary

This will allow selecting the microsoft C runtime library without having to use cc1 flags.

Diff Detail

Event Timeline

akhuang created this revision.Sep 7 2022, 2:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 2:58 PM
akhuang requested review of this revision.Sep 7 2022, 2:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2022, 2:58 PM
akhuang retitled this revision from Add Clang driver flags equivalent to cl's /MD, /MT, /MDd, /MTd. This will allow selecting the MS C runtime library without having to use cc1 flags. to Add Clang driver flags equivalent to cl's /MD, /MT, /MDd, /MTd..Sep 8 2022, 4:58 PM
akhuang edited the summary of this revision. (Show Details)
akhuang added a reviewer: hans.
hans added inline comments.Sep 9 2022, 5:19 AM
clang/include/clang/Driver/Options.td
2215

Could this also list the allowed options? And maybe it could also have a DocBrief that explains how to use it?

clang/lib/Driver/ToolChains/Clang.cpp
6563

Could we somehow re-use the logic in Clang::AddClangCLArgs()? Perhaps the main switch from that could be extracted to a function that could be called from here?

clang/test/Driver/cl-runtime-flags.c
107

If we want these to behave as the /MD and /MT flags, maybe we can re-use the tests for those, so e.g. we'd do:

// RUN: %clang_cl -### /MD -- %s 2>&1 | FileCheck -check-prefix=CHECK-MD %s
// RUN: %clang -### -target ... -fms-runtime-lib=dll -- %s 2>&1 | FileCheck -check-prefix=CHECK-MD %s
// CHECK-MD-NOT: "-D_DEBUG"
// CHECK-MD: "-D_MT"
// CHECK-MD: "-D_DLL"
...
akhuang updated this revision to Diff 459577.Sep 12 2022, 3:53 PM
akhuang marked 2 inline comments as done.

Clean up test, add doc brief to new flag, try to put the flag logic in a separate function

akhuang added inline comments.Sep 12 2022, 3:55 PM
clang/lib/Driver/ToolChains/Clang.cpp
6563

Put it in a separate function; I guess it's a bit messy as some clang-cl flags aren't implemented for clang.

hans added inline comments.Sep 13 2022, 6:20 PM
clang/lib/Driver/ToolChains/Clang.cpp
4442

Could this be an enum, or reuse existing values like the /Mx option ids?

4443

The IsClangCL && part seems redundant?

4445

Should this use HasLDdFlag?

4461

Is there a getLastArg() variant that allows getting the last argument of either the /Mx options or -fms-runtime-lib, so we don't need to check IsClangCL?

akhuang marked 3 inline comments as done.Sep 14 2022, 10:51 AM
akhuang added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4442

Oh, right, resuing the options::OPT__SLASH_ flags makes more sense.

4461

Actually, I think I can just remove the IsClangCL option since this function now runs either in AddClangCLArgs or under !IsCLMode().

akhuang updated this revision to Diff 460154.Sep 14 2022, 10:51 AM
akhuang marked an inline comment as done.

Address comments, more cleanup

hans accepted this revision.Sep 14 2022, 1:11 PM

lgtm, nice!

clang/lib/Driver/ToolChains/Clang.cpp
4451

This could use llvm::StringSwitch

4492

leftover IsClangCL comment?

This revision is now accepted and ready to land.Sep 14 2022, 1:11 PM
mstorsjo added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
6563

Should this be Triple.isWindowsMSVCEnvironment()? We don't want to do this at least for the mingw/cygwin environments.

akhuang updated this revision to Diff 460271.Sep 14 2022, 5:28 PM
akhuang marked 3 inline comments as done.

more comments

LGTM from my perspective now too.

clang/lib/Driver/ToolChains/Clang.cpp
4492

If I understand correctly, we still don't have any corresponding gcc style driver flag for this? Would you consider adding that in a separate, later patch?

akhuang added inline comments.Sep 15 2022, 10:45 AM
clang/lib/Driver/ToolChains/Clang.cpp
4492

Yep, I can add that too.

akhuang updated this revision to Diff 460456.Sep 15 2022, 10:45 AM

ran clang-format

This revision was landed with ongoing or failed builds.Sep 15 2022, 10:52 AM
This revision was automatically updated to reflect the committed changes.

@akhuang I noticed that https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/MSVC.cpp#L199-L200 has got an explicit check for Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd) - I think that this would need to be amended to handle these cases too - or is that inferred from here somehow?

@akhuang I noticed that https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/MSVC.cpp#L199-L200 has got an explicit check for Args.hasArg(options::OPT__SLASH_MD, options::OPT__SLASH_MDd) - I think that this would need to be amended to handle these cases too - or is that inferred from here somehow?

You're right, those cases aren't being handled right now. I'll upload a patch for that.