Page MenuHomePhabricator

Created MCTargetOptions which affects assembly instrumentations.
ClosedPublic

Authored by ygorshenin on Mar 18 2014, 1:29 AM.

Details

Summary

Created MCTargetOptions structure which affects instrumentation of inline assembly. Currently MCTargetOptions contains only type of assembly instrumentation (address for now) and is exposed to target asm parsers.

Diff Detail

Event Timeline

Unfortunately "-fsanitize=*" flag is not exposed to MCTargetAsmParser. Currently investigating how to do that...

Blacklist needs to live somewhere with a longer lifetime.
Also, we need a "more global" place anyway to pass down -fsanitize=(address|memory|thread) setting for top-level assembly.
I suggest looking at EmitAssemblyHelper::CreateTargetMachine() in clang.
We could add a field (or several fields) to llvm::TargetOptions to pass through the sanitizer to run (if any) and, potentially, all the information that is currently passes through *SanitizerPass constructor arguments, like blacklist pass, MSanTrackOrigins, etc.

MCCodeGenInfo also looks like a good place for blacklist to live in.

/// CodeGenInfo - Low level target information such as relocation model.
/// Non-const to allow resetting optimization level per-function.
lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
26

This would enable not yet well tested asm instrumentation by default.

I think it's better to keep the flag name positive, and off by default (for now).

lib/Target/X86/AsmParser/X86AsmParser.cpp
724

This would read and parse the blacklist anew for every machine function.

The whole AsmParser is created from scratch for every inline asm instruction, so it's not a good owner for the blacklist.

ygorshenin updated this revision to Unknown Object (????).Mar 21 2014, 7:09 AM
  1. Updating D3106: MachineFunction is exposed to X86AsmParser. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Fixes.

ygorshenin updated this revision to Unknown Object (????).Mar 21 2014, 7:16 AM
  1. Updating D3106: MachineFunction is exposed to X86AsmParser. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

Added const attribute to STI.

ygorshenin updated this revision to Unknown Object (????).Mar 21 2014, 7:18 AM
  1. Updating D3106: MachineFunction is exposed to X86AsmParser. #
  2. Enter a brief description of the changes included in this update.
  3. The first line is used as subject, next lines as comment. #
  4. If you intended to create a new revision, use:
  5. $ arc diff --create

s/blacklist/blacklisted/

PTAL

Value of "fsanitize" option + asan blacklist should be exposed in a separate CL, as it requires changes to clang.

lib/Target/X86/AsmParser/X86AsmInstrumentation.cpp
26

Done.

lib/Target/X86/AsmParser/X86AsmParser.cpp
724

Done.

eugenis added inline comments.Mar 21 2014, 7:34 AM
include/llvm/Transforms/Utils/SpecialCaseList.h
97 ↗(On Diff #8012)

Please call it isSourceFileIn.

lib/Target/X86/AsmParser/X86AsmInstrumentation.h
32

This is an unrelated change, right? It should be a separate CL.
If you want, I could commit this chunk right now.

ygorshenin updated this revision to Unknown Object (????).Mar 28 2014, 3:11 AM

isFileNameIn is renamed to isSourceFileIn.

ygorshenin updated this revision to Unknown Object (????).Apr 2 2014, 6:43 AM

Asm instrumentation mode is exposed to MCTargetOptions. Blacklist is in progress...

eugenis added inline comments.Apr 2 2014, 7:52 AM
include/llvm/MC/MCTargetOptions.h
19

SanitizeAddress

I still think that bitfields work better here, even though there may be incompatible instrumentation modes.

25

s/sed/set/
I'd rather explain what this flag does, not how it gets set. Like "enables AddressSanitizer instrumentation at machine level".

32

Where is the definition of this constructor?

What happens to MCTargetOptions without the corresponding clang patch. They should default to no instrumentation.

include/llvm/Target/TargetMachine.h
111 ↗(On Diff #8298)

Why? Options is a public member.

ygorshenin updated this revision to Unknown Object (????).Apr 2 2014, 11:04 AM

SpecialCaseList is exposed to MCContext.

Investigating why compilation of llvm-c-test fails.

include/llvm/MC/MCTargetOptions.h
19

Done.

25

Done.

32

Sorry, I forget to upload a source file. Fixed.

include/llvm/Target/TargetMachine.h
111 ↗(On Diff #8298)

Done.

lib/Target/X86/AsmParser/X86AsmInstrumentation.h
32

Deleted, thanks. This class don't have an internal state now, so it's ok to omit void copy ctor and copy operator.

eugenis added inline comments.Apr 3 2014, 1:40 AM
lib/MC/MCContext.cpp
13 ↗(On Diff #8310)
386 ↗(On Diff #8310)

Looks like we need a setter method to support -fsanitize-backlist= in clang.
Then probably ClAsmInstrumentationBlacklist flag should live somewhere else, and use the setter method, too.

ygorshenin updated this revision to Unknown Object (????).Apr 16 2014, 4:18 AM

Fix.

PTAL

lib/MC/MCContext.cpp
13 ↗(On Diff #8310)

Done.

So, this change now does 2 things:

  • adds SanitizeAddress flag to TargetOptions (wrapped in MCTargetOptions)
  • passes current MachineFunction to X86AsmInstrumentation

This effectively lets us take into account sanitize_address function attribute when instrumenting inline asm.
This also lets us apply blacklist when instrumenting .s files by runnning SpecialCaseList in cc1as and setting the bit in TargetOptions. That would be done in a future patch.

This patch looks OK to me.
Does anyone have any objections to the TargetOptions/MCTargetAsmParser changes?

ygorshenin updated this revision to Unknown Object (????).Apr 16 2014, 7:11 AM

MCTargetOptions is exposed to assembly instrumentation instead of MachineFunction.

rafael added inline comments.Apr 21 2014, 7:35 AM
include/llvm/MC/MCTargetAsmParser.h
102

Can this go on the asm streamer?

include/llvm/MC/MCTargetOptions.h
16

Do you expect this class to grow? Why have a class with only one field?

lib/MC/MCTargetOptions.cpp
22

We normally don't want to have command line options showing up in a binary just because it it links with MC. Can this be added to just llc and llvm-mc? Maybe add the option to CommandFlags.h or create another similar file for MC?

ygorshenin updated this revision to Unknown Object (????).Apr 22 2014, 6:52 AM

Created an individual header with flags for MCTargetOptions.

Rafael, Eugene, could you please take another look?

include/llvm/MC/MCTargetOptions.h
16

Yes, we're expecting that this class will grow. For instance, for memory sanitizer there will be additional bit about whether we need to track origins or not. Another reason is that in tools like llvm-mc this class is initialized independently from other TargetOptions's options.

lib/MC/MCTargetOptions.cpp
22

Done, I've created a separate header with flag and function for initialization from command line.

rafael added inline comments.Apr 22 2014, 7:03 AM
include/llvm/MC/MCTargetAsmParser.h
102

The class small and would be easy to copy. The existence of the onMCTargetOptionSet also suggests value semantics, don't you want to store just a "MCTargetOptions MCOptions"?

122

This can be private. Even better, can't you also just remove setMCTargetOptions method completely and add an argument to createMCAsmParser?

ygorshenin updated this revision to Diff 8735.Apr 22 2014, 10:10 AM
ygorshenin edited edge metadata.

Added MCTargetOptions arg to the ctors of target asm parsers.

PTAL

include/llvm/MC/MCTargetAsmParser.h
102

Done.

122

Done, but it required changes for all target parsers.

The phabricator login doesn't seem to be working, so I will just put
the comments on the email.

setMCTargetOptions is now unused, please delete it.
onMCTargetOptionsSet is also unused, please delete it.

AsmPrinterInlineAsm.cpp:147 is now just a white space change now.

/// Current MCTargetOptions.
MCTargetOptions MCOptions;

Without a set method the comment is out of date.

Nit: maybe even better than a copy would be a const reference if that
is possible.

LGTM with that.

ygorshenin updated this revision to Diff 8736.Apr 22 2014, 11:25 AM
ygorshenin edited edge metadata.

Fixes.

Done, many thanks, Rafael and Eugene!

ygorshenin retitled this revision from MachineFunction is exposed to X86AsmParser. to Created MCTargetOptions which affects assembly instrumentations..Apr 23 2014, 4:00 AM
ygorshenin updated this object.
ygorshenin updated this object.Apr 23 2014, 4:02 AM
eugenis accepted this revision.Apr 23 2014, 4:23 AM
eugenis edited edge metadata.

Committed in r206971.

This revision is now accepted and ready to land.Apr 23 2014, 4:23 AM
eugenis closed this revision.Apr 23 2014, 4:23 AM