[llvm-objcopy] [COFF] Add support for removing symbols

[jakehehrlich]

One broad comment is that we should probably implement these in different patches. Just pick one for now and we can build up to the others.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

52

Do these really all do the same thing?

[jakehehrlich]

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

132

The two styles by which removeSymbols and removeSections were written was a historical accident in ELF. We should pick one and stick to it here. I was going to say I don't really care which but I'll retract that statement say that using the style that was used in removeSymbols is to be preferred. I think it will both perform better and be less confusing to incoming devs.

140

I think supporters of checking empty() prior to is_contianed changed their minds. This should be removed in all checks.

tools/llvm-objcopy/COFF/Object.cpp

36

Can sections reference each other? Can a symbol reference a section? A significant portion of the complexity in removeSection in ELF comes from the fact that we handle those sorts of cases. If you remove a section then depending on circumstance we either remove the referencing object as well or throw an error because you weren't *also* removing the reference.

41

This seems like it is perhaps something the Writer should do? I'm not clear on the intention here.

49

Again don't follow the error handling that I used in ELF code. You don't need to pass the otherwise useless Filename if you return an error because you can add that information when you report the error in whatever handles this. Also sometimes it won't even be a file (see -I binary for instance)

59

I know I reportError or call error all of the place, but it's best to avoid that and instead propogate the Error back. I forgot to check for that in the previous patch. Doing this makes writing tools easy but makes converting this into a library hard.

71

Ditto on filename and reporting and comment about RawSymbolTable not belonging in general.

101

Why can't this be a loop over Sec.Relocs? The only blocker seems to be the use of Sec.RelocTargets[I] but it seems that's just a parallel array which is force you to write it like this.

tools/llvm-objcopy/COFF/Object.h

41

Depending on what you mean here, this makes me think this should not be necessary. Can you elaborate?

43

Aside from weather these should actually exist, (Idx probably should from my experience but I'm skeptical about RelocTargets) aren't you maintaining a parallel array array here? Why not just have a proper Reloc struct and put both the coff_relocation and the Target in it? That would improve some code I'm going to complain about below.

49

Yeah this was the sort of thing we eventually compromised on but we agreed that it was a result of poor structure in the ELF code. Ideally this sort of thing should not be in Symbol; we just didn't find a way around it that didn't require significant upheaval in the code. Since we're in an earlier stage with COFF code we can find a better way that storing this with the Symbol itself perhaps?

69

Seems like a field that only the writer should ever know about.

75

Can you elaborate on what the intended semantics of this is? I'd kind of prefer the interface of Object be that you can construct a blank object, build it up step by step having a valid object along the way as you go, and then write it when possible. The exception to this is when parts of an Object reference each other you need to allow for temporary invalidity of some form or another while the cycle is being closed.

76

If possible its most ideal to keep this true as you go rather than setting it all at once. We found that wasn't easily possible to do correctly in the ELF code 1) That might not be true here but even if it is 2) A more generic interface to this would be to use updateSymbols which is the sort of generic operation on an Object that I'd like to see used in favor of something specific like this.

78

ditto.

79

Can you describe what this does a bit more?

[mstorsjo]

Thanks for the thorough review!

In D55881#1336855, @jakehehrlich wrote:

One broad comment is that we should probably implement these in different patches. Just pick one for now and we can build up to the others.

I found them rather entangled, as removing sections also requires removing the corresponding symbols for those sections. But I can try to make this only removal of symbols, as an absolutely minimal first step.

Also, most of the comments seem to be related to the design of the data structure (or lack thereof). I'll try to restructure it according to the general direction I got from these comments, towards the goal of maintaining a correct state along the way as far as possible, moving more of the reassembly of correct state into the writer.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

52

As far as I've seen, yes, with respect to sections. With respect to symbols, they do slightly different things though. (In the original objcopy, with things abstracted via libbfd, they probably have slightly different logic, but when mapped down to COFF, this is what I've discovered so far at least.)

132

Ok, I'll try to rewrite that part (but if I get the patch split, that's in a later patch then).

140

Ok, will do gladly.

tools/llvm-objcopy/COFF/Object.cpp

36

A symbol references a section, and a relocation can reference a symbol. There are cases where sections can reference each other (associative comdat symbols) that I haven't implemented yet - I don't think GNU objcopy does either (it doesn't know about the associative comdat concept at all, afaik). Getting those right would of course be ideal, but they're a bit out of scope of what I'm aiming at initially (basic stripping functionality).

Most of the functionality in this patch also revolves around this - removing symbols that belong to a removed section, etc, but it's rather spread out, and the Object class API isn't easy to use for constructing things from scratch, and updates requires coordinating a few steps.

41

It initializes helper tables that currently only are used during the transformation stage. But I think the general remedy to most of the suggested changes is to make more of these permanent members of the data structures, updated on the go as far as possible, and moving more of the logic into the reader/writer.

49

Ok, that makes sense. I didn't copy this design from the ELF code actually, I independently made the same bad design...

101

Yes, moving the target into the Relocs vector would get rid of that.

tools/llvm-objcopy/COFF/Object.h

41

I mean that they're not essential for reading/writing, but necessary as intermediate helpers when remapping symbol indices. So strictly, this could also be kept in separate datastructure within COFFObjcopy, as it doesn't have to be persisted to the writing stage. But it's more convenient to have them go along with the Section struct here.

43

Sure, that should be doable.

49

I don't instinctively think of a better way of storing it - can you elaborate on what you would have in mind?

69

No, the write (as it currently stands) doesn't need to know about this. Currently, before invoking the writer, all relocations have correct symbol table indices set up.

A COFF symbol can have a number of auxillary symbol table entries following, with metadata about that preceding symbol. The Symbols array here only consists of logical symbols (so that when removing one symbol, the corresponding extra entries are gone along with it), and this allows looking up what Symbol a raw symbol table index (from a relocation) refers to. The writer currently just writes out the symbol table index in the relocation as is.

75

That sounds like a great goal for the Object class API, my design so far is too much of a plain container with the logic scattered outside of it.

[mstorsjo]

Split out code for removing sections to a later patch. Maintaining a map from symbol name to Symbol object within the Object class, and keeping the relocation target symbol name in a Reloc struct together with the coff_relocation struct. This hopefully addresses most of @jakehehrlich's feedback.

[mstorsjo]

Marked a few more comments done (for things that I moved to a later patch). The main thing I didn't change much is the Referenced field, which I update with a helper function before it's needed. I could make the Reader call it originally to avoid having to do it in COFFObjcopy.cpp as well, in order to try to keep it up to date at all times.

[jakehehrlich]

Head's up I'm about to go home for the evening and I'm pretty well off for until the new year after that. I think others are in a similar boat. I'm not ready to LGTM this yet and I don't have time to give more review so this might have to wait until January 2nd.

In D55881#1337277, @mstorsjo wrote:

I found them rather entangled, as removing sections also requires removing the corresponding symbols for those sections. But I can try to make this only removal of symbols, as an absolutely minimal first step.

Yeah I think given how simple section stripping seems to be for COFF relative to ELF this might make sense. I'm fine with just minimizing the number of options a bit. I think we can come back to minimizing handleArgs after the new years

tools/llvm-objcopy/COFF/Object.cpp

20

I was thinking of updateSymbols as more of an operation the user could take on each symbol. See updateSymbols in the ELF code for instance. This seems like something that can be done at finalization time.

36

Awesome! Good to hear COFF doesn't (at least yet) have that complexity.

49

I copied it from LLD so you're not alone.

tools/llvm-objcopy/COFF/Object.h

49

I don't have anything specifically in mind, I don't remember the review too well. It's kind of formats specific. For instance a symbol can be "referenced" by more than one section. It might be nice to have sections list all the symbols that they reference (except the symbol table which is the owner not the referencer) but this could cause O(n^2) running times. I was hoping that some quirk or insight you might have into the nature of COFF might lead us toward a means of avoiding storing weather this was referenced or not while maintaining something about as efficient. Had I done a complete redesign of the ELF code with this in mind we might wound up with something like a graph that could be walked to take out all the things that we actually wind up using and stripping would just reduce the set of initial nodes for instance; That's a crazy different direction but maybe something less extreme is possible here.

76

Oh other thing, I think we did this on the section level (which probably doesn't make sense here) but it was called "markSymbols" to mimic "mark and sweep" garbage collection. I'd prefer that name but it hardly matters.

[alexander-shaposhnikov]

test/tools/llvm-objcopy/COFF/X86/remove-local-symbols.s

1 ↗

(On Diff #179052)

so I'm confused - what decision has been made regarding tests (sorry, many people have commented, I'd like to understand the logic):
all the new tests will use assembly while yaml is deprecated, we will use both yaml and assembly, we will use yaml and assembly only in exceptional cases, or what ?
As far as I'm concerned - i want consistency + functionality, at least in the future.

[mstorsjo]

In D55881#1338420, @jakehehrlich wrote:

Head's up I'm about to go home for the evening and I'm pretty well off for until the new year after that. I think others are in a similar boat. I'm not ready to LGTM this yet and I don't have time to give more review so this might have to wait until January 2nd.

Ok, thanks for the reviews so far, let's pick it up again after new year's. I realized a few shortcomings in my approach so far (that had gone unnoticed while I've kept my patchset in use for a couple weeks but that just now cropped up) - the fact that I can't use symbol names as unique keys, and that this tool really does run on objects with associative comdats in the build setups I'm testing, so I'll try to revise it further until then.

test/tools/llvm-objcopy/COFF/X86/remove-local-symbols.s

1 ↗

(On Diff #179052)

I'm not aware of any other decision other than preferring yaml over binary object files.

I'd strongly prefer to be able to use both yaml and assembly for these tests; yaml is ideal for dumped output from a full toolchain, including debug info and everything that a real object file would contain. Assembly is better for the cases when I want to construct a specific setup to test certain cases, where this keeps the test much smaller and easier to read and understand. And for executables instead of object files, yaml is the only option as we can't rely on lld.

tools/llvm-objcopy/COFF/Object.cpp

20

Some of this can be done at finalization, but the map from symbol name to object pointer needs to be up to date when we do setReferencedSymbols or markSymbols if I rename it that way. With the current setup, of only supporting removing symbols and nothing else, it's enough if the Reader sets this up as well, but if we do multiple consecutive operations on the symbol vector (once we add removing of sections) we need to do it after each one of them.

However I just realized that there's fatal flaw with this in the current form; symbol names aren't necessarily unique within a COFF object file - there's a local symbol for each section, and there can be multiple sections with the same name. In the COFF files on disk, the symbol table index is the unique key that differentiates them, but when we rearrange the symbol table (e.g. on removals) I'll probably have to add some other unique id to be able to hook them up correctly back again after removals.

tools/llvm-objcopy/COFF/Object.h

49

Well the alternative, as I see it, would be to store a backreference in each symbol, indicating what relocations refer to it, and that pretty much doubles the storage needed for all relocations. It probably creates a nice walkable graph in all directions, but also feels rather overengineered.

76

Ok, I can rename it to markSymbols.

[mstorsjo]

In D55881#1338836, @mstorsjo wrote:

I realized a few shortcomings in my approach so far (that had gone unnoticed while I've kept my patchset in use for a couple weeks but that just now cropped up) - the fact that I can't use symbol names as unique keys

I'm updating the patch to work around this now, by adding a different unique key to each symbol, to avoid having to use the non-unique name for that. Incidentally I noticed that neither obj2yaml, llvm-readobj or llvm-objdump were capable of differentiating between two symbols with the same name when dumping relocations; I'm adding support for that in D56140 for llvm-readobj, and will amend the tests here a bit once that lands, to properly verify that aspect as well.

tools/llvm-objcopy/COFF/Object.cpp

36

Actually, associative comdats are a case where one section (indirectly, via a section definition symbol) references another section. Since this patch only handles symbol removal so far, I'm not touching that quite yet. Once I do, I'll probably first just let the removal do what it does and then error out at the end if there are dangling references - which should save quite a bit of complexity and be just fine for the basic stripping functionality (and shouldn't exclude implementing it differently later if desired).

[mstorsjo]

Did a few suggested changes, renaming setReferencedSymbols to markSymbols, and using a unique key instead of the non-unique symbol name. Will amend the tests for that case once D56140 lands.

[jhenderson]

test/tools/llvm-objcopy/COFF/X86/remove-local-symbols.s

1 ↗

(On Diff #179052)

Yes, to my knowledge, we haven't agreed anything formal re. YAML versus assembly, but I personally would prefer YAML wherever possible. From experience, at least for ELF, assembly often doesn't provide enough low-level control which we need to produce objects that have the properties we want in the input (e.g. what kinds of relocations get emitted etc). The comment about calling otherfunc not resulting in a relocation is a perfect example of this. It also removes the need for the "PRE" checks.

Also, I might be wrong, but wouldn't making this use YAML allow us to drop the X86-requirement here?

20–21 ↗

(On Diff #179739)

This will produce a false positive, if otherfunc is present, because you've got nothing checking the total number of symbols. I don't know if COFF has a convenient way of testing this, but I would recommend doing so. Otherwise, you should put a -NEXT suffix on this line (and similarly in the above block) - you should also show that there are no symbols before this list if doing things this way, since llvm-objcopy might have reordered the symbols to put otherfunc first instead of removing it.

Could we also test that two different local symbols are removed? That shows that we remove all locals not just the first.

test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s

23

My comments apply from the remove-local-symbols test apply equally well here. We need to more tightly show that there are no symbols in other places. Similarly, it might be nice to make this YAML based if possible.

test/tools/llvm-objcopy/COFF/X86/remove-symbol2.s

7–11

I don't think this check really gives us anything in this case.

13

It would be nice if this test checked that the file name appears in the error. The symbol name should probably also be quoted in some fashion. At least on ELF, it is possible to have symbol names with spaces in, so the error can be a bit ambiguous!

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

39

There seems to be an awful lot of code in this function that I don't think is tested. Perhaps it would make more sense to implement it in more patches (e.g. basic explicit symbol removal, then keep, then strip all or whatever etc), and test each part individually.

53

This isn't a parsing failure. Use a more appropriate error value, like invalid_argument.

tools/llvm-objcopy/COFF/Object.cpp

39–40

Referenced is initialised to false. Do you need to reset it here as well? I don't envisage a situation where we should need to do this more than once on a single object.

tools/llvm-objcopy/COFF/Object.h

27–28

Are these constructors really necessary? Won't they be generated by default?

46–47

Don't abbreviate unnecessarily (Idx -> Index). Do we really need both separately? If so, perhaps better names would be OriginalIndex and Index or similar, and then use the input symbol index as the key.

70

This comment seems to indicate an encapsulation issue to me. If this function has to be called any time the vector is updated, perhaps the vector should be private and accessed only through member functions?

[mstorsjo]

test/tools/llvm-objcopy/COFF/X86/remove-local-symbols.s

1 ↗

(On Diff #179052)

Yes, using YAML would remove the dependency on X86 being enabled, and I agree that it makes some things less ambiguous. As for writing (and reading) the tests I prefer assembly, but if there's strong opposition I can make them YAML instead and just use assembly as source locally when crafting the tests.

There is, however, at least one case in a test (that depends on D56140) which is impossible to make using YAML as input at the moment; the COFF YAML format doesn't distinguish between multiple symbols with the same name for relocations. To fix that, the COFF YAML format would need to be changed to use symbol indices instead of names for relocations (mirroring what actually is stored in the files).

20–21 ↗

(On Diff #179739)

Indeed, that's true. I somehow thought I tested this stricter than this, but apparently I misremembered.

Yes, some sort of stricter check would indeed be good. I think the current order actually is produced by llvm-nm and not dependent on the source file itself, but avoiding such assumptions is of course better.

Using -NEXT would indeed help, but is there anything that would force a match on the first line? llvm-nm doesn't print anything at all before the symbols. llvm-readobj -symbols is way too verbose for a neat test here (it prints 5+ lines per symbol), but llvm-objdump -t turns out to be very compact and detailed, so that's probably even better than this. That output also has got a nice start header that makes the use of -NEXT exact.

test/tools/llvm-objcopy/COFF/X86/remove-symbol2.s

7–11

Indeed, this part of the test is pretty pointless.

13

Ok, will make the code quote it and have the test look for a filename as well.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

39

Fair enough, I can do that.

53

Ok, will use a different error code.

tools/llvm-objcopy/COFF/Object.cpp

39–40

I'd rather have the method be self-contained enough to clear it here, in case the caller would call this method multiple times. If it doesn't, the method leaks the limitation that it can only be called once, which in itself also is rather encapsulation.

To make things safer perhaps the field should be initialized to true, to indicate that all symbols are referenced unless we've called this and actually know for sure?

tools/llvm-objcopy/COFF/Object.h

46–47

Ok, I'll expand the unnecessarily terse name.

As for Key vs OriginalIndex - yes, input symbol index would work just as well for key (and that's pretty much what I use right now anyway). But as @jakehehrlich wants to have the Object API usable even for constructing object files from scratch, naming it OriginalIndex feels a bit limiting.

70

Yes, that'd be a nicer abstraction. That leaves things a bit open wrt how to use it from the Reader though; I could have an add method which adds one symbol at a time and updates the map after each one. That ends up with O(n^2) for the map updates. Alternatively the Reader produces a vector locally and there'd be an addSymbols(vector<Symbol>) method here, which adds all at once and does one update of the map at the end.

What do you think?

[mstorsjo]

Updating with most of the clear issues fixed, removed code for all other stripping modes than removing explicitly named symbols. Using llvm-objdump -t instead of llvm-nm for printing symbol lists, using -NEXT to avoid the risk of any missed symbols. Using llvm::errc::invalid_argument in one place, renamed RawIdx to RawIndex, added quoting around the symbol name for the erorr message, extended the error test to check for this and the file name.

Other issues that still are under discussion are not yet changed.

[mstorsjo]

Updated and extended the testcase after llvm-readobj was improved. The testcase in remove-symbols1.s no longer works with yaml input, with current yaml2obj/obj2yaml tools/formats.

[jhenderson]

llvm-readobj -symbols is way too verbose for a neat test here (it prints 5+ lines per symbol)

Not that it's really relevant here, I think, but llvm-readelf (aka GNU output style for llvm-readobj) produces single line output for each symbol. I've not looked, but I'm guessing that llvm-readobj doesn't have an equivalent for COFF. Maybe that's worth taking a look at at some point? We actually tend to use the verbose output style for the llvm-objcopy ELF tests though.

Yes, using YAML would remove the dependency on X86 being enabled, and I agree that it makes some things less ambiguous. As for writing (and reading) the tests I prefer assembly, but if there's strong opposition I can make them YAML instead and just use assembly as source locally when crafting the tests.

I don't feel strongly about it, but I would prefer YAML, because of the extra control granted.

There is, however, at least one case in a test (that depends on D56140) which is impossible to make using YAML as input at the moment; the COFF YAML format doesn't distinguish between multiple symbols with the same name for relocations. To fix that, the COFF YAML format would need to be changed to use symbol indices instead of names for relocations (mirroring what actually is stored in the files).

Hmm... that's an interesting case. I think it accepting arbitrary indexes would be nice. IIRC, the ELF YAML format allows for some fields specifying either a name or index, and if an index, it doesn't do any checking, which allows for crafting invalid inputs to test error handling. Having a symmetry here would make a lot of sense.

test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s

12–13

Unless there's a strong need, I'd just drop the (16)/(15) bit at the end, as it allows folding these two lines together, and doesn't really give us anything, since there's only one "ptr" symbol.

48

Could you largely fold this and the PRE block together, to reduce risk of copy/paste issues, and make clear the difference?

You could do this by sharing the majority of the checks, but having a single difference for the removed symbol:

...
# SYMBOLS-NEXT: foo
# SYMBOLS-PRE-NEXT: otherfunc
# SYMBOLS-NEXT: otherfunc2
...

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

32

You might want to simplify this if in this first version to only check the SymbolsToRemove.

40–42

This comment doesn't really make sense in the patch's current form.

tools/llvm-objcopy/COFF/Object.cpp

39–40

Okay, I see your point, so it's fine what you have done. I have issues both with it being set to true and to false, because each would result in technically incorrect state for a period of time until the function is called. Perhaps it would be best to simply not initialise Referenced at all, until markSymbols is called, since the value can't be relied on to be safe without that call.

tools/llvm-objcopy/COFF/Object.h

46–47

Okay, no problem. I'd marginally prefer UniqueID to Key, I think, but I'm okay if you prefer it this way around.

70

I think an addSymbols function as you describe would make a lot of sense. A future change might also want a single addSymbol function too, but that's just a minor thing that can be added when required.

[mstorsjo]

In D55881#1344707, @jhenderson wrote:

There is, however, at least one case in a test (that depends on D56140) which is impossible to make using YAML as input at the moment; the COFF YAML format doesn't distinguish between multiple symbols with the same name for relocations. To fix that, the COFF YAML format would need to be changed to use symbol indices instead of names for relocations (mirroring what actually is stored in the files).

Hmm... that's an interesting case. I think it accepting arbitrary indexes would be nice. IIRC, the ELF YAML format allows for some fields specifying either a name or index, and if an index, it doesn't do any checking, which allows for crafting invalid inputs to test error handling. Having a symmetry here would make a lot of sense.

Ok, I'll try to have a look at that and whether it's possible to extend the COFF YAML format similarly.

test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s

12–13

The point I tried to make in keeping it explicit, is that llvm-objcopy updated the symbol index in this relocation. Granted, if it wouldn't have, the symbol name would have changed as well (or it would have ended up as a broken relocation), but I tried to show it clearly that this reloc actually does refer to a symbol that changed index.

48

Oh, right, yes - will do.

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

32

Right, yes, will fix.

40–42

Oops, will fix.

tools/llvm-objcopy/COFF/Object.cpp

39–40

Ok, I can leave it uninitialized, and then sanitizers and checkers can point out if it's ever used uninitialized when it shouldn't.

tools/llvm-objcopy/COFF/Object.h

27–28

Sorry, I forgot to reply on this one yesterday. They're indeed unnecessary if using the right syntax for initializing, I didn't think of the new C++11 syntax for doing that without explicit constructors.

46–47

I can change it to UniqueId, that's what I originally called it, but later changed it into Key because I thought it was better - I'll change it back.

[mstorsjo]

Updated the details discussed with @jhenderson, except for the still-discussed overly verbose relocation symbol index in the test, and still using assembly instead of yasm for the test, pending extending obj2yaml/yaml2obj.

[mstorsjo]

tools/llvm-objcopy/COFF/Object.h

74

Oops, I forgot to clang-format this one - done locally but won't repost a new patch just for that.

[jhenderson]

test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s

12–13

Okay, that seems reasonable. Might be worth a small comment to show that's what you're doing (i.e. showing that the index has been changed).

tools/llvm-objcopy/COFF/Object.cpp

21–24

Not sure it's actually any simpler, but you could use std::transform, to do this. Approximate untested code:

std::transform(NewSymbols.begin(),
  NewSymbols.end(),
  std::back_inserter(Symbols),
  [&NextSymbolUniqueID](Symbol&S){ S.UniqueId = NextSymbolUniqueId++;});

Okay, now that I type it out, this is probably less readable too, so feel free to completely ignore it!

tools/llvm-objcopy/COFF/Object.h

27–28

I'm happy that you changed this, but I now realise that I misread the original, and probably wouldn't have bothered commenting if I'd realised it was a conversion constructor, not a generic copy constructor!

66

Could this return an ArrayRef?

69

This can take an ArrayRef.

74

I'd move this method to be alongside getSymbols.

[mstorsjo]

test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s

12–13

Ok, adding a comment here.

tools/llvm-objcopy/COFF/Object.h

66

Yup, will change.

69

Done.

74

Ok, will do.

[mstorsjo]

Changed a few cases of std::vector to ArrayRef as suggested, moved the getMutableSymbols method next to getSymbols as suggested, added a comment about the verbose tests, and changed the test input to yaml (depending on D56294).

[jhenderson]

test/tools/llvm-objcopy/COFF/Inputs/remove-symbols.yaml

38 ↗

(On Diff #180315)

Similar to my comments in D56294, how much of this YAML is actually necessary to test symbol removal? We really only want the very minimum needed for this particular case (and we can add more for other tests or too this test as we add more cases).

test/tools/llvm-objcopy/COFF/remove-symbol1.test

1 ↗

(On Diff #180315)

Please rename this to strip-symbol.test, to match the long form of the switch name. Also, please add a case using the long form. This could be a second run of llvm-objcopy on the same input, and show that you get the same output. Finally, the general pattern in most ELF tests is to make the edits out-of-line (i.e. specify a different output file to the input file), and then to show that the input file was not itself modified by the operation. This last point is probably only important because of the second point re. the long form switch name etc.

test/tools/llvm-objcopy/COFF/remove-symbol2.test

1 ↗

(On Diff #180315)

Rename this test to something meaningful, e.g. strip-referenced-symbol.test

In fact, I wonder if we should mirror the equivalent ELF test (strip-reloc-symbol.test) and error message as much as possible? What do you think?

[mstorsjo]

Stripped down the yaml input, renamed tests as suggested (matching the ELF tests), adjusted the error message to match the one used in the ELF case.

[jhenderson]

LGTM, but please wait for the others to confirm they're happy.

[mstorsjo]

@jakehehrlich - Any further comments, or can this go in?

[alexander-shaposhnikov]

tools/llvm-objcopy/COFF/Writer.h

25

btw, i forgot to ask it earlier - why do we need this base class + protected fields ?
maybe just roll it into COFFWriter ? (the hierarchy of Readers (in ELF and MachO) basically facilitates handling different kinds of input (object file vs binary input (not a regular object file)), but why is this base class necessary ?

[alexander-shaposhnikov]

tools/llvm-objcopy/COFF/Object.h

68

i was thinking about these 3 guys for some time,
i don't have a strong opinion - however, for what it's worth, they all should be in some consistent state, maybe there should be an extra abstraction which would encapsulate them ? (and Object would own that abstraction (for example)). There is some interference with relocations, but we can expose some methods to enable that "communication".

[mstorsjo]

tools/llvm-objcopy/COFF/Object.h

68

Well they're private at least, to me that's at least some sort of encapsulation. I'd rather not overengineer that part at this point, in case later patches turn out to need things wrt these I haven't thought of yet.

tools/llvm-objcopy/COFF/Writer.h

25

They're not really necessary - I brought them along while copying the general structure from ELF, and I tried to ask for opinions during review whether I should keep or remove them, but I don't think anybody commented on it. I can do an NFC patch to remove them.

[alexander-shaposhnikov]

tools/llvm-objcopy/COFF/Writer.h

25

yeah, sounds good, thanks

[rupprecht]

Just some nits, but I think this is good to commit.

tools/llvm-objcopy/COFF/Object.cpp

21

Symbol& S?

32

Given the number of symbols may be large and you know the exact size you want to add, you might want to avoid reallocation by changing SymbolMap.clear() with SymbolMap = DenseMap<xxx>(Symbols.size()).

tools/llvm-objcopy/COFF/Reader.cpp

87

Similarly you can set an initial size of COFFObj.getRawNumberOfSymbols() here

[mstorsjo]

In D55881#1353276, @rupprecht wrote:

Just some nits, but I think this is good to commit.

Ok - should I still wait for @jakehehrlich, or do you happen to know about his whereabouts?

tools/llvm-objcopy/COFF/Object.cpp

21

No, intentionally not a reference here. The point is that we have an ArrayRef<Symbol> from the caller, which essentially is read-only, and we mutate the Symbol objects as we insert them into the Object class' internal vector.

32

Ok, I'll try that.

tools/llvm-objcopy/COFF/Reader.cpp

87

Ok, will do. That'd be Symbols.reserve(COFFObj.getRawNumberOfSymbols()) right, as it can't be done via the constructor directly?

[rupprecht]

In D55881#1353296, @mstorsjo wrote:

In D55881#1353276, @rupprecht wrote:

Just some nits, but I think this is good to commit.

Ok - should I still wait for @jakehehrlich, or do you happen to know about his whereabouts?

IMO you can go ahead and submit now -- Jake (or anyone else) can always do a post-commit review if there are issues with this patch.

tools/llvm-objcopy/COFF/Reader.cpp

87

Sounds right to me

test/tools/llvm-objcopy/COFF/X86/lit.local.cfg

+# These tests require a registered x86 backend.
+
+if not 'X86' in config.root.targets:
+    config.unsupported = True

test/tools/llvm-objcopy/COFF/X86/remove-symbol1.s

+# RUN: llvm-mc -triple=x86_64-windows-gnu %s -filetype=obj -o %t.o
+
+# RUN: llvm-readobj -relocations %t.o | FileCheck %s --check-prefix=RELOCS
+# RUN: llvm-objdump -t %t.o | FileCheck %s --check-prefix=SYMBOLS-PRE
+# RUN: llvm-objcopy -N otherfunc %t.o
+# RUN: llvm-readobj -relocations %t.o | FileCheck %s --check-prefix=RELOCS
+# RUN: llvm-objdump -t %t.o | FileCheck %s --check-prefix=SYMBOLS-POST
+
+# RELOCS:      Relocations [
+# RELOCS-NEXT:   Section (1) .text {
+# RELOCS-NEXT:     0x2 IMAGE_REL_AMD64_REL32 ptr
+# RELOCS-NEXT:   }
+# RELOCS-NEXT: ]
+
+# SYMBOLS-PRE: SYMBOL TABLE:
+# SYMBOLS-PRE-NEXT: .text
+# SYMBOLS-PRE-NEXT: AUX scnlen
+# SYMBOLS-PRE-NEXT: .data
+# SYMBOLS-PRE-NEXT: AUX scnlen
+# SYMBOLS-PRE-NEXT: .bss
+# SYMBOLS-PRE-NEXT: AUX scnlen
+# SYMBOLS-PRE-NEXT: mainfunc
+# SYMBOLS-PRE-NEXT: ptr
+# SYMBOLS-PRE-NEXT: otherfunc
+# SYMBOLS-PRE-NEXT: otherfunc2
+# SYMBOLS-PRE-EMPTY:
+
+# SYMBOLS-POST: SYMBOL TABLE:
+# SYMBOLS-POST-NEXT: .text
+# SYMBOLS-POST-NEXT: AUX scnlen
+# SYMBOLS-POST-NEXT: .data
+# SYMBOLS-POST-NEXT: AUX scnlen
+# SYMBOLS-POST-NEXT: .bss
+# SYMBOLS-POST-NEXT: AUX scnlen
+# SYMBOLS-POST-NEXT: mainfunc
+# SYMBOLS-POST-NEXT: ptr
+# SYMBOLS-POST-NEXT: otherfunc2
+# SYMBOLS-POST-EMPTY:
+
+
+        .text
+        .globl  mainfunc
+mainfunc:
+        movl    ptr(%rip), %eax
+        call    otherfunc
+        ret
+        .globl  otherfunc
+otherfunc:
+        ret
+        .globl  otherfunc2
+otherfunc2:
+        ret
+
+        .data
+ptr:
+        .long   42

test/tools/llvm-objcopy/COFF/X86/remove-symbol2.s

+# RUN: llvm-mc -triple=x86_64-windows-gnu %s -filetype=obj -o %t.o
+
+# RUN: not llvm-objcopy -N ptr %t.o 2>&1 | FileCheck %s --check-prefix=ERROR
+
+# ERROR: error: '{{.*}}/remove-symbol2.s.tmp.o': Can't remove referenced symbol 'ptr'
+
+        .text
+        .globl  mainfunc
+mainfunc:
+        movl    ptr(%rip), %eax
+        call    otherfunc
+        ret
+        .globl  otherfunc
+otherfunc:
+        ret
+        .globl  otherfunc2
+otherfunc2:
+        ret
+
+        .data
+ptr:
+        .long   42

tools/llvm-objcopy/CMakeLists.txt

 tablegen(LLVM StripOpts.inc -gen-opt-parser-defs)
 add_public_tablegen_target(StripOptsTableGen)
 
 add_llvm_tool(llvm-objcopy
   Buffer.cpp
   CopyConfig.cpp
   llvm-objcopy.cpp
   COFF/COFFObjcopy.cpp
+  COFF/Object.cpp
   COFF/Reader.cpp
   COFF/Writer.cpp
   ELF/ELFObjcopy.cpp
   ELF/Object.cpp
   DEPENDS
   ObjcopyOptsTableGen
   StripOptsTableGen
   )
 
 add_llvm_tool_symlink(llvm-strip llvm-objcopy)
 
 if(LLVM_INSTALL_BINUTILS_SYMLINKS)
   add_llvm_tool_symlink(objcopy llvm-objcopy)
   add_llvm_tool_symlink(strip llvm-objcopy)
 endif()

tools/llvm-objcopy/COFF/COFFObjcopy.cpp

 #include "CopyConfig.h"
 #include "Object.h"
 #include "Reader.h"
 #include "Writer.h"
 #include "llvm-objcopy.h"
 
 #include "llvm/Object/Binary.h"
 #include "llvm/Object/COFF.h"
+#include "llvm/Support/Errc.h"
 #include <cassert>
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
+static Error handleArgs(const CopyConfig &Config, Object &Obj) {
+  // If we need to do per-symbol removals, initialize the Referenced field.
+  if (Config.StripUnneeded || Config.DiscardAll ||
+      !Config.SymbolsToRemove.empty())
+    if (Error E = Obj.markSymbols())
+      return E;
+
+  // Actually do removals of symbols.
+  Obj.removeSymbols([&](const Symbol &Sym) {
+    if (is_contained(Config.SymbolsToRemove, Sym.Name)) {
+      // Explicitly removing a referenced symbol is an error, while
+      // implicitly removing them as part of a blanket removal above is
+      // accepted.
+      if (Sym.Referenced)
+        reportError(Config.OutputFilename,
+                    make_error<StringError>("Can't remove referenced symbol '" +
+                                                Sym.Name + "'",
+                                            llvm::errc::invalid_argument));
+      return true;
+    }
+
+    return false;
+  });
+  return Error::success();
+}
+
 void executeObjcopyOnBinary(const CopyConfig &Config,
                             object::COFFObjectFile &In, Buffer &Out) {
   COFFReader Reader(In);
   Expected<std::unique_ptr<Object>> ObjOrErr = Reader.create();
   if (!ObjOrErr)
     reportError(Config.InputFilename, ObjOrErr.takeError());
   Object *Obj = ObjOrErr->get();
   assert(Obj && "Unable to deserialize COFF object");
+  if (Error E = handleArgs(Config, *Obj))
+    reportError(Config.InputFilename, std::move(E));
   COFFWriter Writer(*Obj, Out);
   if (Error E = Writer.write())
     reportError(Config.OutputFilename, std::move(E));
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

tools/llvm-objcopy/COFF/Object.h

 //===- Object.h -------------------------------------------------*- C++ -*-===//
 //
 //                      The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_TOOLS_OBJCOPY_COFF_OBJECT_H
 #define LLVM_TOOLS_OBJCOPY_COFF_OBJECT_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/BinaryFormat/COFF.h"
 #include "llvm/Object/COFF.h"
 #include <cstddef>
 #include <cstdint>
 #include <vector>
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
+struct Relocation {
+  Relocation() {}
+  Relocation(const object::coff_relocation &R) : Reloc(R) {}
+
+  object::coff_relocation Reloc;
+  size_t Target;
+  StringRef TargetName; // Used for diagnostics only
+};
+
 struct Section {
   object::coff_section Header;
   ArrayRef<uint8_t> Contents;
-  std::vector<object::coff_relocation> Relocs;
+  std::vector<Relocation> Relocs;
   StringRef Name;
 };
 
 struct Symbol {
   object::coff_symbol32 Sym;
   StringRef Name;
   ArrayRef<uint8_t> AuxData;
+  size_t Key;
+  size_t RawIndex;
+  bool Referenced = false;
 };
 
 struct Object {
   bool IsPE = false;
 
   object::dos_header DosHeader;
   ArrayRef<uint8_t> DosStub;
 
   object::coff_file_header CoffFileHeader;
 
   bool Is64 = false;
   object::pe32plus_header PeHeader;
   uint32_t BaseOfData = 0; // pe32plus_header lacks this field.
 
   std::vector<object::data_directory> DataDirectories;
   std::vector<Section> Sections;
   std::vector<Symbol> Symbols;
+  DenseMap<size_t, Symbol *> SymbolMap;
+  size_t NextSymbolKey = 0;
+
+  // Update SymbolMap and RawIndex in each Symbol. This must be called
+  // after updating the Symbols vector from outside of this class.
+  void updateSymbols();
+
+  void removeSymbols(function_ref<bool(const Symbol &)> ToRemove);
+
+  // Set the Referenced field on all Symbols, based on relocations in
+  // all sections.
+  Error markSymbols();
 };
 
 // Copy between coff_symbol16 and coff_symbol32.
 // The source and destination files can use either coff_symbol16 or
 // coff_symbol32, while we always store them as coff_symbol32 in the
 // intermediate data structure.
 template <class Symbol1Ty, class Symbol2Ty>
 void copySymbol(Symbol1Ty &Dest, const Symbol2Ty &Src) {
   static_assert(sizeof(Dest.Name.ShortName) == sizeof(Src.Name.ShortName),
                 "Mismatched name sizes");

tools/llvm-objcopy/COFF/Object.cpp

+//===- Object.cpp ---------------------------------------------------------===//
+//
+//                      The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Object.h"
+#include <algorithm>
+
+namespace llvm {
+namespace objcopy {
+namespace coff {
+
+using namespace object;
+
+void Object::updateSymbols() {
+  SymbolMap.clear();
+  size_t RawSymIndex = 0;
+  for (Symbol &Sym : Symbols) {
+    SymbolMap[Sym.Key] = &Sym;
+    Sym.RawIndex = RawSymIndex;
+    RawSymIndex += 1 + Sym.Sym.NumberOfAuxSymbols;
+  }
+}
+
+void Object::removeSymbols(function_ref<bool(const Symbol &)> ToRemove) {
+  Symbols.erase(
+      std::remove_if(std::begin(Symbols), std::end(Symbols),
+                     [ToRemove](const Symbol &Sym) { return ToRemove(Sym); }),
+      std::end(Symbols));
+  updateSymbols();
+}
+
+Error Object::markSymbols() {
+  for (Symbol &Sym : Symbols)
+    Sym.Referenced = false;
+  for (const Section &Sec : Sections) {
+    for (const Relocation &R : Sec.Relocs) {
+      auto It = SymbolMap.find(R.Target);
+      if (It == SymbolMap.end())
+        return make_error<StringError>("Relocation target " + Twine(R.Target) +
+                                           " not found",
+                                       object_error::invalid_symbol_index);
+      It->second->Referenced = true;
+    }
+  }
+  return Error::success();
+}
+
+} // end namespace coff
+} // end namespace objcopy
+} // end namespace llvm

tools/llvm-objcopy/COFF/Reader.h

 };
 
 class COFFReader : public Reader {
   const COFFObjectFile &COFFObj;
 
   Error readExecutableHeaders(Object &Obj) const;
   Error readSections(Object &Obj) const;
   Error readSymbols(Object &Obj, bool IsBigObj) const;
+  Error setRelocTargets(Object &Obj) const;
 
 public:
   explicit COFFReader(const COFFObjectFile &O) : COFFObj(O) {}
   Expected<std::unique_ptr<Object>> create() const override;
 };
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm
 
 #endif // LLVM_TOOLS_OBJCOPY_COFF_READER_H

tools/llvm-objcopy/COFF/Reader.cpp

for (size_t I = 1, E = COFFObj.getNumberOfSections(); I <= E; I++) {
     if (Sec->hasExtendedRelocations())
       return make_error<StringError>("Extended relocations not supported yet",
                                      object_error::parse_failed);
   }
   return Error::success();
 }
 
 Error COFFReader::readSymbols(Object &Obj, bool IsBigObj) const {
   for (uint32_t I = 0, E = COFFObj.getRawNumberOfSymbols(); I < E;) {
     Expected<COFFSymbolRef> SymOrErr = COFFObj.getSymbol(I);
     if (!SymOrErr)
       return SymOrErr.takeError();
     COFFSymbolRef SymRef = *SymOrErr;
 
     Obj.Symbols.push_back(Symbol());
     Symbol &Sym = Obj.Symbols.back();
     // Copy symbols from the original form into an intermediate coff_symbol32.
     if (IsBigObj)
       copySymbol(Sym.Sym,
                  *reinterpret_cast<const coff_symbol32 *>(SymRef.getRawPtr()));
     else
       copySymbol(Sym.Sym,
                  *reinterpret_cast<const coff_symbol16 *>(SymRef.getRawPtr()));
     if (auto EC = COFFObj.getSymbolName(SymRef, Sym.Name))
       return errorCodeToError(EC);
     Sym.AuxData = COFFObj.getSymbolAuxData(SymRef);
+    Sym.Key = Obj.NextSymbolKey++;
     assert((Sym.AuxData.size() %
             (IsBigObj ? sizeof(coff_symbol32) : sizeof(coff_symbol16))) == 0);
     I += 1 + SymRef.getNumberOfAuxSymbols();
   }
   return Error::success();
 }
 
+Error COFFReader::setRelocTargets(Object &Obj) const {
+  std::vector<Symbol *> RawSymbolTable;
+  for (Symbol &Sym : Obj.Symbols) {
+    RawSymbolTable.push_back(&Sym);
+    for (size_t I = 0; I < Sym.Sym.NumberOfAuxSymbols; I++)
+      RawSymbolTable.push_back(nullptr);
+  }
+  for (Section &Sec : Obj.Sections) {
+    for (Relocation &R : Sec.Relocs) {
+      if (R.Reloc.SymbolTableIndex >= RawSymbolTable.size())
+        return make_error<StringError>("SymbolTableIndex out of range",
+                                       object_error::parse_failed);
+      Symbol *Sym = RawSymbolTable[R.Reloc.SymbolTableIndex];
+      if (Sym == nullptr)
+        return make_error<StringError>("Invalid SymbolTableIndex",
+                                       object_error::parse_failed);
+      R.Target = Sym->Key;
+      R.TargetName = Sym->Name;
+    }
+  }
+  return Error::success();
+}
+
 Expected<std::unique_ptr<Object>> COFFReader::create() const {
   auto Obj = llvm::make_unique<Object>();
 
   const coff_file_header *CFH = nullptr;
   const coff_bigobj_file_header *CBFH = nullptr;
   COFFObj.getCOFFHeader(CFH);
   COFFObj.getCOFFBigObjHeader(CBFH);
   bool IsBigObj = false;
Expected<std::unique_ptr<Object>> COFFReader::create() const {
   }
 
   if (Error E = readExecutableHeaders(*Obj))
     return std::move(E);
   if (Error E = readSections(*Obj))
     return std::move(E);
   if (Error E = readSymbols(*Obj, IsBigObj))
     return std::move(E);
+  if (Error E = setRelocTargets(*Obj))
+    return std::move(E);
+  Obj->updateSymbols();
 
   return std::move(Obj);
 }
 
 } // end namespace coff
 } // end namespace objcopy
 } // end namespace llvm

tools/llvm-objcopy/COFF/Writer.h

 #include <utility>
 
 namespace llvm {
 namespace objcopy {
 namespace coff {
 
 struct Object;
 
 class Writer {
 protected:
   Object &Obj;
   Buffer &Buf;
 
 public:
   virtual ~Writer();
   virtual Error write() = 0;
 
   Writer(Object &O, Buffer &B) : Obj(O), Buf(B) {}
 };
 
 class COFFWriter : public Writer {
   size_t FileSize;
   size_t FileAlignment;
   size_t SizeOfInitializedData;
   StringTableBuilder StrTabBuilder;
 
+  Error finalizeRelocTargets();
   void layoutSections();
   size_t finalizeStringTable();
   template <class SymbolTy> std::pair<size_t, size_t> finalizeSymbolTable();
 
-  void finalize(bool IsBigObj);
+  Error finalize(bool IsBigObj);
 
   void writeHeaders(bool IsBigObj);
   void writeSections();
   template <class SymbolTy> void writeSymbolStringTables();
 
   Error write(bool IsBigObj);
 
   Error patchDebugDirectory();

tools/llvm-objcopy/COFF/Writer.cpp

 namespace objcopy {
 namespace coff {
 
 using namespace object;
 using namespace COFF;
 
 Writer::~Writer() {}
 
+Error COFFWriter::finalizeRelocTargets() {
+  for (Section &Sec : Obj.Sections) {
+    for (Relocation &R : Sec.Relocs) {
+      auto It = Obj.SymbolMap.find(R.Target);
+      if (It == Obj.SymbolMap.end())
+        return make_error<StringError>("Relocation target " + R.TargetName +
+                                       " (" + Twine(R.Target) + ") not found",
+                                       object_error::invalid_symbol_index);
+      R.Reloc.SymbolTableIndex = It->second->RawIndex;
+    }
+  }
+  return Error::success();
+}
+
 void COFFWriter::layoutSections() {
   for (auto &S : Obj.Sections) {
     if (S.Header.SizeOfRawData > 0)
       S.Header.PointerToRawData = FileSize;
     FileSize += S.Header.SizeOfRawData; // For executables, this is already
                                         // aligned to FileAlignment.
     if (S.Header.NumberOfRelocations > 0)
       S.Header.PointerToRelocations = FileSize;

 template <class SymbolTy>
 std::pair<size_t, size_t> COFFWriter::finalizeSymbolTable() {
   size_t SymTabSize = Obj.Symbols.size() * sizeof(SymbolTy);
   for (const auto &S : Obj.Symbols)
     SymTabSize += S.AuxData.size();
   return std::make_pair(SymTabSize, sizeof(SymbolTy));
 }
 
-void COFFWriter::finalize(bool IsBigObj) {
+Error COFFWriter::finalize(bool IsBigObj) {
+  if (Error E = finalizeRelocTargets())
+    return E;
+
   size_t SizeOfHeaders = 0;
   FileAlignment = 1;
   size_t PeHeaderSize = 0;
   if (Obj.IsPE) {
     Obj.DosHeader.AddressOfNewExeHeader =
         sizeof(Obj.DosHeader) + Obj.DosStub.size();
     SizeOfHeaders += Obj.DosHeader.AddressOfNewExeHeader + sizeof(PEMagic);
 
if (Obj.IsPE)
       StrTabSize = 0;
   }
 
   size_t NumRawSymbols = SymTabSize / SymbolSize;
   Obj.CoffFileHeader.PointerToSymbolTable = PointerToSymbolTable;
   Obj.CoffFileHeader.NumberOfSymbols = NumRawSymbols;
   FileSize += SymTabSize + StrTabSize;
   FileSize = alignTo(FileSize, FileAlignment);
+
+  return Error::success();
 }
 
 void COFFWriter::writeHeaders(bool IsBigObj) {
   uint8_t *Ptr = Buf.getBufferStart();
   if (Obj.IsPE) {
     memcpy(Ptr, &Obj.DosHeader, sizeof(Obj.DosHeader));
     Ptr += sizeof(Obj.DosHeader);
     memcpy(Ptr, Obj.DosStub.data(), Obj.DosStub.size());
for (const auto &S : Obj.Sections) {
     // For executable sections, pad the remainder of the raw data size with
     // 0xcc, which is int3 on x86.
     if ((S.Header.Characteristics & IMAGE_SCN_CNT_CODE) &&
         S.Header.SizeOfRawData > S.Contents.size())
       memset(Ptr + S.Contents.size(), 0xcc,
              S.Header.SizeOfRawData - S.Contents.size());
 
     Ptr += S.Header.SizeOfRawData;
-    std::copy(S.Relocs.begin(), S.Relocs.end(),
-              reinterpret_cast<coff_relocation *>(Ptr));
+    for (const auto &R : S.Relocs) {
+      memcpy(Ptr, &R.Reloc, sizeof(R.Reloc));
+      Ptr += sizeof(R.Reloc);
+    }
   }
 }
 
 template <class SymbolTy> void COFFWriter::writeSymbolStringTables() {
   uint8_t *Ptr = Buf.getBufferStart() + Obj.CoffFileHeader.PointerToSymbolTable;
   for (const auto &S : Obj.Symbols) {
     // Convert symbols back to the right size, from coff_symbol32.
     copySymbol<SymbolTy, coff_symbol32>(*reinterpret_cast<SymbolTy *>(Ptr),
                                         S.Sym);
     Ptr += sizeof(SymbolTy);
     std::copy(S.AuxData.begin(), S.AuxData.end(), Ptr);
     Ptr += S.AuxData.size();
   }
   if (StrTabBuilder.getSize() > 4 || !Obj.IsPE) {
     // Always write a string table in object files, even an empty one.
     StrTabBuilder.write(Ptr);
     Ptr += StrTabBuilder.getSize();
   }
 }
 
 Error COFFWriter::write(bool IsBigObj) {
-  finalize(IsBigObj);
+  if (Error E = finalize(IsBigObj))
+    return E;
 
   Buf.allocate(FileSize);
 
   writeHeaders(IsBigObj);
   writeSections();
   if (IsBigObj)
     writeSymbolStringTables<coff_symbol32>();
   else