In D66274#1634895, @arichardson wrote:

In D66274#1634810, @ruiu wrote:

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

I had to do this for the CHERI fork of LLD since we added a few new entries. We currently always use a 128-bit mask (https://github.com/CTSRD-CHERI/llvm-project/blob/master/lld/ELF/Relocations.cpp#L139) but I guess we could reorder the enum to have a fast path for commonly checked values and do the second compare only for less common entries. I can submit a patch if you would like to use the 128-bit variant.

In rL366708#680830, @NikolaPrica wrote:

Actually it is not fixed. Bug is related to "-O2" linker optimizations. More precisely to stringpool optimization of merge-able sections. I've located bug in the linker but I didn't have time to get into the details. Here you can find binaries https://sourceware.org/bugzilla/show_bug.cgi?id=16794.

Updated for code review comment.

Add target triple to test.

@ilya-biryukov @hokein ping

In D66339#1633368, @greened wrote:

In D66339#1632975, @sdesmalen wrote:

I like your suggestion of using concat_iterator(_range), but I think grouping all fixed-width types together is sufficient for now. If there is ever a good reason to iterate through (all combined scalable and fixed-width fp vector types)or (all combined scalable and fixed-width integer vector types), we can always introduce the extra concat_iterators then.

I'm not sure what you're suggesting here. Are you asking Graham to keep all the fixed-width types contiguous (i.e. int and fp)? This patch does not do that. I don't have a strong preference either way. Given that the existing state is non-contiguous I see no compelling reason to change it.

Thanks for doing this. This was something on my radar too as an odd inconsistency.

In D66274#1634810, @ruiu wrote:

In D66274#1634792, @MaskRay wrote:

In D66274#1634786, @ruiu wrote:

Nice! Thank you for doing this.

The remaining bits of RelExpr are a scarce recourse now... R_RISCV_PC_INDIRECT == 61. @sidneym Do you need more than 3 bits?

It shouldn't be too hard to extend oneOf so that the function uses two words instead of one word as a bitmap, but yeah, we should first try to reduce the number of relocation types if possible.

LGTM, but please clang-format your patch before committing (esp. spacing around binary operators).

Actually it is not fixed. Bug is related to "-O2" linker optimizations. More precisely to stringpool optimization of merge-able sections. I've located bug in the linker but I didn't have time to get into the details. Here you can find binaries https://sourceware.org/bugzilla/show_bug.cgi?id=16794.

LGTM.

I'm concerned with the getHeader() call changes in ELFObjectFile, mostly because again it's introducing a very obvious point of failure. What if somebody added a new piece of functionality to the base class to get another piece of data from the header? There's a good change they'll call EF.getHeader(). Say the new function is called getFoo(): if another user then decides they want to call getFoo() on a MutableELFObject, then they'll end up using the unmodified header rather than the modified one, which could cause surprising bugs. I don't have an obvious answer to this, but I'm beginning to wonder if we should be using a different style of inheritance, one that only makes the functions available that we know to be safe from a MutableELFObject's point of view. This might be something like private inheritance. I'm not sure though, as it would mean providing shim functions to re-publicise the "safe" functions we care about. What are your thoughts?

LGTM, though @rupprecht should confirm too.

Addressed review comments.

LGTM