Page MenuHomePhabricator

[FaultMaps] Add a parser for the __llvm__faultmaps section.
ClosedPublic

Authored by sanjoy on Jun 16 2015, 4:36 PM.

Details

Summary

The parser is exercised by llvm-objdump using -print-fault-maps. As is
probably obvious, the code itself was "heavily inspired" by
http://reviews.llvm.org/D10434.

Diff Detail

Repository
rL LLVM

Event Timeline

sanjoy updated this revision to Diff 27795.Jun 16 2015, 4:36 PM
sanjoy retitled this revision from to [FaultMaps] Add a parser for the __llvm__faultmaps section..
sanjoy updated this object.
sanjoy edited the test plan for this revision. (Show Details)
sanjoy added reviewers: reames, atrick, JosephTremoulet.
sanjoy added a subscriber: Unknown Object (MLST).
reames edited edge metadata.Jun 17 2015, 1:37 PM

The actual parser part is LGTM w/minor comments addressed. I don't feel comfortable signing off on the objdump parts.

include/llvm/CodeGen/FaultMaps.h
74 ↗(On Diff #27795)

Add some comments about compatibility guarantees. It's easier to preempt such things now than introduce them later.

118 ↗(On Diff #27795)

Minor style suggestion:
If you predeclared both inner classes, you could put the definition for the FunctionInfoAccessor first. When trying to understand the format, that's the piece you need first.

136 ↗(On Diff #27795)

I'm not a huge fan of having the types of each field repeated both in the offset code and reader code. Too much room for mismatches. I don't really have a good suggestion on how to do this otherwise though.

(What you have is fine, just airing a general frustration.)

150 ↗(On Diff #27795)

I really don't like this style of iterator, but it's hard to avoid if you want to do a flyweight parser in this style with variably sized entries.

199 ↗(On Diff #27795)

I think you can probably get away with these being inferred which would make the code cleaner.

212 ↗(On Diff #27795)

A clearer structure would be:
for(int i = 0; i < NumFunctions; i++) {

if (i == 0) FFI = getFirstFunctionInfo();
else FFI = getNextFunctionInfo();
OS << FFI;

}

tools/llvm-objdump/llvm-objdump.cpp
158 ↗(On Diff #27795)

The general option naming convention above would seem to call for a name like "-fault-map-section"

Description wise, maybe "Display contents of faultmap section"? I'm trying to get at the fact this is not a hex dump, but not be overly verbose.

1235 ↗(On Diff #27795)

The output should probably indicate there isn't a FaultMap table if that's the case.

1236 ↗(On Diff #27795)

Is this specific to ELF? Or should it work with any format of object file?

1249 ↗(On Diff #27795)

I don't believe StringRef has any backing storage. This looks highly suspicious to say the least. Should this be an std::string instead?

1252 ↗(On Diff #27795)

reinterpret_cast?! Couldn't this at least be a static_cast?

const char* -> const uint8_t* doesn't seem like it should need a reinterpret_cast

1255 ↗(On Diff #27795)

Why can't you just use begin(), end() on StringRef?

sanjoy added inline comments.Jun 17 2015, 4:00 PM
include/llvm/CodeGen/FaultMaps.h
74 ↗(On Diff #27795)

Done.

118 ↗(On Diff #27795)

Done.

199 ↗(On Diff #27795)

Do you mean Endianness? If so I tried that and it didn't work.

212 ↗(On Diff #27795)

Makes sense, fixed.

tools/llvm-objdump/llvm-objdump.cpp
158 ↗(On Diff #27795)

Fixed.

1235 ↗(On Diff #27795)

Fixed.

1236 ↗(On Diff #27795)

Actually, this worked only with Mach-O. I've added support (and a test) for ELF, and changed printFaultMaps to print an error for other file formats (currently only COFF).

1249 ↗(On Diff #27795)

This looks intentional -- as far as I can tell, the sections return a reference to memory they own.

1255 ↗(On Diff #27795)

That's a good idea, using bytes_begin() and bytes_end() lets me avoid casting altogether.

sanjoy updated this revision to Diff 27886.Jun 17 2015, 4:01 PM
sanjoy edited edge metadata.
  • Address Philip's review.

Hey Rafael,

Can you please take a look at the llvm-objdump portion of this patch?

Thanks,
Sanjoy

rafael edited edge metadata.Jun 18 2015, 6:04 AM

I have no objections (just small comments), but lhames should probably be the one to give the final OK.

test/CodeGen/X86/implicit-null-check.ll
2 ↗(On Diff #27886)

Do you really need to use llc? Can't you use llvm-mc or even check in a binary?

3 ↗(On Diff #27886)

These are both little endian. Your code is templated on endiannes, so you should probably include a big endian test.

LGTM, just some minor cosmetic feedback.

include/llvm/CodeGen/FaultMaps.h
75–76 ↗(On Diff #27886)

I think you have a typo or missing word(s) here (was it supposed to read "version of LLVM that includes it"?).

117 ↗(On Diff #27886)

I had the same reaction as Philip on seeing the uintnn_t types replicated in getFoo vs the FooOffset calculation. Did you consider introducing typedefs for each field (e.g. typedef uint64_t FunctionAddrType) so that the offset calculation and read template argument could each be in terms of the typedef, and there'd be just one place mapping each field to its type?

124 ↗(On Diff #27886)

Should "idx" be "Idx" instead?

132–135 ↗(On Diff #27886)

This could be size_t NextFunctionInfoOffset = FunctionFaultInfosOffset + getNumFaultingPCs() * FunctionFaultInfoSize; to avoid needing to duplicate the whole field list here.

sanjoy updated this revision to Diff 27975.Jun 18 2015, 4:46 PM
sanjoy edited edge metadata.
  • address Joseph's review comments
  • add an llvm-mc based test
include/llvm/CodeGen/FaultMaps.h
75–76 ↗(On Diff #27886)

Fixed.

117 ↗(On Diff #27886)

That's a good idea, done.

124 ↗(On Diff #27886)

Fixed.

132–135 ↗(On Diff #27886)

Fixed.

test/CodeGen/X86/implicit-null-check.ll
2 ↗(On Diff #27886)

Unless you have strong objections, I'd like to keep this test because I think it is valuable to check that the parser can parse the output of -enable-implicit-null-checks. I'll add a second test that works with llvm-mc.

3 ↗(On Diff #27886)

Right now the faultmaps section is generated only for x86_64.


rafael wrote:

Do you really need to use llc? Can't you use llvm-mc or even check in a binary?

Unless you have strong objections, I'd like to keep this test because I think it is valuable to check that the parser can parse the output of -enable-implicit-null-checks. I'll add a second test that works with llvm-mc.

In which case you should do

llc ... -o %t.s < %s
llvm-mc ... -o %t.o < %t.s

We really should only use "llc -filetype=obj" when there a code path
that is only ever used with that.

Right now the faultmaps section is generated only for x86_64.

Why the template then?

Cheers,
Rafael

LGTM, thanks.

tools/llvm-objdump/llvm-objdump.cpp
1241–1245 ↗(On Diff #27975)

What's missing that will need to be implemented to support this on COFF? (Not asking you to make any adjustments to this change; just saw this code and wondered about COFF.)

sanjoy updated this revision to Diff 27985.Jun 18 2015, 6:15 PM
  • address Rafaels' comment: change test to not pass -filetype=obj to llc
sanjoy updated this revision to Diff 28041.Jun 19 2015, 1:21 PM
  • address Rafael's review: remove templating

This is fine with me.
lhames?

ping!

Does it make sense to check in the parser now, and wait for Lang to comment on the objdump bits? I don't really need the objdump changes locally, they're just for testing.

rafael accepted this revision.Jun 22 2015, 10:43 AM
rafael edited edge metadata.

This is fine. I am sure any further comment can be addressed in post commit.

This revision is now accepted and ready to land.Jun 22 2015, 10:43 AM
This revision was automatically updated to reflect the committed changes.