LLD: representation of a power of two value

It’s not a big deal, but it always annoyed me a bit when I hit it, so I’ll bring it up here.

LLD represents an alignment X as log2(X) in some places and just X in other places. It’s a bit confusing. Because I always think alignments in my mind in terms of 1, 2, 4, 8, …, instead of 2^1, 2^2, 2^3, …, I’d like to propose to always use real values.

Any objections?

I'm in favor of this change, not only for consistency but because I
feel the same way as you.
I've recently hit some alignment issues linking on FreeBSD and it will
be easier for me to reason about them if you make this change.

Thanks,

Can you give some examples?

The DefinedAtom class has struct:

    struct Alignment {
        uint16_t powerOf2;
        uint16_t modulus;
    };

That use seems clear. But, yes, if there is “int alignment” somewhere, that is ambiguous.

-Nick

At least in ELF, alignment values are represented not by exponents but by
just numbers in files, so we convert them back and force. "Alignment" is
used for both exponents and actual values there.

I agree. This is how we represent alignment everywhere in llvm
projects except a few places in lld.

- Michael Spencer

I agree. Every time I deal with alignment in the LLD code I have to
remember how we represent alignment in that case.

I believe you are mentioning about section alignments, that are being differentiated from atom alignments ?

If so are you planning to change the variable name (or) use Alignment even there ?

Shankar Easwaran

I guess I’m talking about atoms. Do we have notion of section in LLD?

Its only atoms. I was hinting at default output section alignments in the ELF writer.

I’ll update wherever we use log2 instead of a value itself.

It's not a big deal, but it always annoyed me a bit when I hit it, so I'll
bring it up here.

LLD represents an alignment X as log2(X) in some places and just X in
other places. It's a bit confusing. Because I always think alignments in my
mind in terms of 1, 2, 4, 8, ..., instead of 2^1, 2^2, 2^3, ..., I'd like
to propose to always use real values.

Any objections?

The only theoretical objection I can imagine is that you can use much less
storage for holding the log2(X). But right now that isn't causing us any
problems it seems.

-- Sean Silva

I submitted a series of patches to convert all uses of log2 values to non-log2 values (That was harder than I thought because the types of the two are the same. They only different in meaning. So it was not easy to distinguish them. I split up patches so that anyone can verify correctness of the conversion that I’ve done.)

From now on, please always use non-log2 alignment values exclusively in LLD. Thanks!

I was expecting that you would send the patches for review prior committing the patch.

We could follow this style for all future commits that things get reviewed before committing, so that its easier to follow a uniform policy.

For example you added a PowerOf2 class in DefinedAtom, for a simple reason that the class usage went away :slight_smile: I thought it would also have helped if you made few commits to cleanup, which IMO would be easier to review and get a overall sense of the code.

Shankar Easwaran

I requested you pre-commit code review several times recently because you
submitted them without any discussion or code review. This is not the case.
Also please note that the patches looked actually dubious. As the history
of this project shows, I'm probably the person who cares most about the
LLD's code healthiness, and probably the person who contributed to that
area most. I'm sorry to say that, but your coding practice sometimes seem
very odd to me (e.g. being against even removing completely-dead code, or
doing something too fancy with several levels of indirections), so I need
to review your code carefully.

As to the patches, I usually don't split a refactoring patch. This is an
exception -- this would actually help others understand code. If each step
of the patches look trivial, it served its purpose -- everything looks
obvious and mechanical translation from the old code to the new one. Had I
done that in one large patch, no one wouldn't have been able to verify that
I didn't miss something.