This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] [COFF] Update symbol indices in weak externals
ClosedPublic

Authored by mstorsjo on Jan 21 2019, 2:04 AM.

Details

Summary

This is, as far as I know, the only other aux symbol whose contents we need to update based on how llvm-objcopy rewrites the files. (We already handle coff_aux_section_definition.)

Diff Detail

Event Timeline

mstorsjo created this revision.Jan 21 2019, 2:04 AM
jhenderson added inline comments.Jan 21 2019, 2:50 AM
test/tools/llvm-objcopy/COFF/weak-external.test
26–27

If these two fields aren't required, it would be nice to get rid of them.

tools/llvm-objcopy/COFF/Reader.cpp
155–156

Use createStringError instead of make_error<StringError>

159–160

Ditto.

tools/llvm-objcopy/COFF/Writer.cpp
78

This seems like a weird condition. Could you explain it please (e.g. with a comment).

83–85

Ditto. In this case, you can then take advantage of the printf-style overload to avoid the string concatenation here.

Also putting some quotes around the symbol name might be good, especially if it's theoretically possible to have spaces in your symbol names in COFF.

mstorsjo marked 4 inline comments as done.Jan 21 2019, 3:32 AM
mstorsjo added inline comments.
test/tools/llvm-objcopy/COFF/weak-external.test
26–27

Apparently the test works even without them - will remove.

tools/llvm-objcopy/COFF/Reader.cpp
155–156

Ok, I can change that.

I've used a lot of make_error<StringError> so far in the COFF backend, so there's a number of other occurrances of it that I could change in a separate patch if you prefer that as well? (And there's one instance of it in the ELF subdir.)

tools/llvm-objcopy/COFF/Writer.cpp
78

I can add a comment, sure. The NumberOfAuxSymbols check needs to be at least greater than or equal to one for the code not to write out of bounds, but I've made the check stricter since only exactly equal to one makes sense.

83–85

With printf-style formatting, I'd either need to make the StringRef a null terminated string, or use the %.*s formatting syntax with Sym.Name.size(), Sym.Name.data() or something similar, which kinda negates any convenience of printf-style formatting strings.

About quotes around the symbol names - oh, right, yes. It seems I've slipped in a few more of these before as well, I can push an NFC patch that fixes those, the next time I push something.

mstorsjo updated this revision to Diff 182772.Jan 21 2019, 3:46 AM
mstorsjo marked 5 inline comments as done.

Applied the suggested changes.

jhenderson accepted this revision.Jan 21 2019, 3:58 AM

LGTM. I'm happy with you updating the last createStringError call if you want, or leaving as is.

tools/llvm-objcopy/COFF/Reader.cpp
155–156

That would be good.

tools/llvm-objcopy/COFF/Writer.cpp
83–85

You could use StringRef::str() to avoid this.

Updating the other errors later would be good.

This revision is now accepted and ready to land.Jan 21 2019, 3:58 AM
mstorsjo marked an inline comment as done.Jan 21 2019, 4:02 AM
mstorsjo added inline comments.
tools/llvm-objcopy/COFF/Writer.cpp
83–85

Wouldn't that need something like Sym.Name.str().c_str() then? That's also a bit clunky but slightly leaner than the %.*s form.

jhenderson added inline comments.Jan 21 2019, 4:25 AM
tools/llvm-objcopy/COFF/Writer.cpp
83–85

Possibly. I can't remember exactly how the internals work. I think I'd prefer it to the current form, because it keeps the pattern simpler.

mstorsjo updated this revision to Diff 182776.Jan 21 2019, 4:29 AM

Going with %s together with StringRef::str() + .c_str() for formatting. Will use this form for later change to unify on createStringError() then.

This revision was automatically updated to reflect the committed changes.