This is an archive of the discontinued LLVM Phabricator instance.

Move getenv for AS_SECURE_LOG_FILE to clang
ClosedPublic

Authored by benlangmuir on Oct 27 2022, 2:40 PM.

Details

Summary

Avoid calling getenv in the MC layer and let the clang driver do it so that it is reflected in the command-line as an -mllvm option.

rdar://101558354

Diff Detail

Event Timeline

benlangmuir created this revision.Oct 27 2022, 2:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 27 2022, 2:40 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
benlangmuir requested review of this revision.Oct 27 2022, 2:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptOct 27 2022, 2:40 PM
benlangmuir added inline comments.Oct 27 2022, 2:41 PM
llvm/lib/MC/MCContext.cpp
61–62

Interestingly, opt<char*> doesn't have a parser; this code only worked with the default value before.

If we have to fix it, I slightly prefer just give it a proper option in CodeGenOption and a cc1 and cc1as flag.

llvm/lib/MC/MCContext.cpp
61–62

ha, maybe we should just deprecate the directive since this hasn't been working for more than 6 years!

benlangmuir added inline comments.Oct 27 2022, 3:36 PM
llvm/lib/MC/MCContext.cpp
61–62

It works if you use the environment variable because that feeds directly into the default value for the cl::opt. It's only if you actually tried to pass -mllvm -as-secure-log-file-name ... it would fail to parse the option.

  • Updated to use cc1 option in clang
  • Moved the llvm-mc option to a central location so it can be used when initialized MCTargetOptions
  • Added tests to cover cc1 and cc1as since each one passes in the string to MC in a slightly different way (CodeGenOpts vs. AssemblerInvocation)
MaskRay added inline comments.Oct 27 2022, 7:00 PM
clang/test/Driver/AS_SECURE_LOG_FILE.s
1

--target= for new tests

remove unused -o %t.o

llvm/lib/MC/MCContext.cpp
77

When is TargetOptions null? When it is TargetOptions, Does a value of "" matter?

steven_wu accepted this revision.Oct 28 2022, 10:00 AM

LGTM except the comments from @MaskRay

This revision is now accepted and ready to land.Oct 28 2022, 10:00 AM
  • --target=, remove -o per review
  • Use "log_file" as path to try to avoid Windows path issue in driver test
llvm/lib/MC/MCContext.cpp
77

When is TargetOptions null?

null is the default value for the parameter, and happens in various places that construct MCOptions. None of them require a secure log file as far as I can tell.

Does a value of "" matter?

Like the other file path in MCTargetOptions (SplitDwarfFile) empty indicates no file specified.

benlangmuir added inline comments.Oct 28 2022, 11:41 AM
llvm/lib/MC/MCContext.cpp
77

@MaskRay was this just a question, or did you want me to change something here? Wanted to check before I merge this.

MaskRay accepted this revision.Oct 28 2022, 3:05 PM
MaskRay added inline comments.
llvm/lib/MC/MCContext.cpp
77

Just a question. This looks good.

The environment variable use is a bit odd. I do not know whether it is a legacy feature...

This revision was landed with ongoing or failed builds.Oct 28 2022, 4:08 PM
This revision was automatically updated to reflect the committed changes.