AdaCore / ada-spark-rfcs

Platform to submit RFCs for the Ada & SPARK languages
63 stars 28 forks source link

New aspect to prevent default values #53

Open gusthoff opened 4 years ago

gusthoff commented 4 years ago

When creating overlays with the Address attribute, default values might become an issue. For example, let's say we have the following type declaration:

    type Unsigned_8 is mod 2 ** 8 with Default_Value => 0;

    type Byte_Field is array (Natural range <>) of Unsigned_8;

If we create an overlay, the default value will be used to initialize the overlay. For example:

    procedure Use_Overlay (V : in out Integer) is
       BF : Byte_Field (1 .. V'Size / 8)
         with Address => V'Address, Volatile;
         --  BF is initialized with Default_Value of Unsigned_8
    begin
       null;
    end Use_Overlay;

We may use the Import aspect to solve this problem:

    procedure Use_Overlay (V : in out Integer) is
       BF : Byte_Field (1 .. V'Size / 8)
         with Address => V'Address, Import => True, Volatile;

There are, however, two problems with the Import aspect:

We could, however, introduce a distinct aspect for this case. For example:

    procedure Use_Overlay (V : in out Integer) is
       BF : Byte_Field (1 .. V'Size / 8)
         with Address => V'Address, Use_Default_Value => False, Volatile;

What do you think? Is it worth creating an RFC for this new aspect?

sttaft commented 4 years ago

Interesting suggestion. But this situation seems like a bit of a corner case to me, since allowing programmers to use an Address clause but not pragma Import seems like an odd combination. As far as whether Import is "non-intuitive," I don't think you necessarily help things by adding a redundant aspect, because the programmer then needs to know about this new aspect. In any case, anyone who uses an Address clause should know what they are doing, and should understand the purpose of pragma Import. ;-)

Take care, -Tuck

On Fri, Aug 7, 2020 at 6:50 AM Gustavo A. Hoffmann notifications@github.com wrote:

When creating overlays with the Address attribute, default values might become an issue. For example, let's say we have the following type declaration:

type Unsigned_8 is mod 2 ** 8 with Default_Value => 0;

type Byte_Field is array (Natural range <>) of Unsigned_8;

If we create an overlay, the default value will be used to initialize the overlay. For example:

procedure Use_Overlay (V : in out Integer) is

   BF : Byte_Field (1 .. V'Size / 8)

     with Address => V'Address, Volatile;

     --  BF is initialized with Default_Value of Unsigned_8

begin

   null;

end Use_Overlay;

We may use the Import aspect to solve this problem:

procedure Use_Overlay (V : in out Integer) is

   BF : Byte_Field (1 .. V'Size / 8)

     with Address => V'Address, Import => True, Volatile;

There are, however, two problems with the Import aspect:

-

As soon as we restrict this aspect in our configuration — using pragma Restrictions (No_Use_Of_Pragma => Import); —, the solution above won't compile anymore.

It's non-intuitive: nothing in its name is saying that the aspect is meant to prevent default values from being used.

We could, however, introduce a distinct aspect for this case. For example:

procedure Use_Overlay (V : in out Integer) is

   BF : Byte_Field (1 .. V'Size / 8)

     with Address => V'Address, Use_Default_Value => False, Volatile;

What do you think? Is it worth creating an RFC for this new aspect?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/AdaCore/ada-spark-rfcs/issues/53, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANZ4FPWBYY3OMUHEVZL25TR7PL7PANCNFSM4PXQIUCQ .

QuentinOchem commented 4 years ago

Thanks for submitting this one @gusthoff. I fully agree with the need and have seen very common cases where this would have been handy. Using pragma import feels like a hack here. We've even seen this pragma completely forbidden by coding standard for security reasons, while overlays and deactivating initialization was a need. Not to mention situations where one may want to deactivate initialization without even the need of an overlay.

Strong support on this one from my end.

briot commented 4 years ago

The default_value aspect cannot be applied to a subtype, but maybe that would be a cleaner approach. Something like:

type Uninitialized is mod 2 ** 8; subtype Unsigned_8 is Uninitialized with Default_Value => 0; A : Unsigned_8; B : Uninitialized;

then A is 0 and B is some random value, which you can use in an array for which you want to use Address.

The original suggestion looks a bit like “I want default values” followed with “ah, but not quite, something I do not want that”.

Now I understand that applying the aspect to a subtype might be confusing in some cases, but perhaps there’s a possible way, I don’t know

Emmanuel

QuentinOchem commented 4 years ago

@briot The issue is that there are situations where you can't control default values, for example with access types. It's also much harder to control if you have default for a record type components (which themselves may be access types).

clairedross commented 4 years ago

Sorry for the stupid question. Why do coding standard forbid Import? is it to avoid having dangling references to subprograms or is it also dangerous for objects?

raph-amiard commented 4 years ago

I also think this is a worthy proposal (not to mention the implementation price seems really small)

I wonder if it wouldn't be better named No_Initialization or something.

I also think using Import for this is a hack.

Robert-Tice commented 4 years ago

An argument here is that Import is an overloaded aspect. To me, it has two uses (maybe there's more):

1.) When not combined with 'Address => Tell the compiler that the object implementation is external and will be resolved at link time (equivalent to C's extern) 2.) When combined with 'Address => This is POD (plain old data) and we will read the values from the address rather than apply a default initialization, even if default values are provided.

Here's the embedded use case: This is the Main Oscillator register for the ARM SAMV71 BSP.

You will notice that all of these record components have default values applied, BUT, we are also using 'Address. Here is that record as a component of the full register definition. And here is the address overlay.

The behavior here is that the default values are ignored because of the Import aspect. However, the default values are necessary so that I can perform operations like this.

Here we are setting values in the register and using others => <> to utilize the specified default values for the other record components. We have to set this register this way because of the KEY field.

This processor is hardened against spurious bit flips. The technique that is used is to disallow writes to certain registers is: the KEY field must be written with the correct bit sequence on every write. This means that we cannot do this: PMC_Periph.CKGR_MOR.MOSCXTEN := 1;

We must do:

PMC_Periph.CKGR_MOR := (MOSCXTEN => 1, KEY => Passwd, others => <>);

That will be rejected because we didn't also write the KEY field with the correct bit sequence.

So in this use case, we are required to use default values, 'Address, and 'Import on this register in order to get this to work. So I guess the question is: does it make sense to have two use cases for 'Import?

QuentinOchem commented 4 years ago

@clairedross Import may have additional consequences, for example at link time where it can lead to importing symbols from other objects that you wouldn't normally want to. I'm not entirely clear on all the ramifications in our technology, but this is a reason why we forbid it on our learn website for example.

There may be way to work around this (e.g. allow import only on objects if there's no external name provided...) but that start to be a lot of work and analysis to workaround a piece of the capability which could come with a cleaner design.

sttaft commented 4 years ago

On Fri, Aug 7, 2020 at 10:01 AM Robert Tice notifications@github.com wrote:

An argument here is that Import is an overloaded aspect. To me, it has two uses (maybe there's more):

1.) When not combined with 'Address => Tell the compiler that the object implementation is external and will be resolved at link time (equivalent to C's extern) 2.) When combined with 'Address => This is POD (plain old data) and we will read the values from the address rather than apply a default initialization, even if default values are provided.

Here's the embedded use case: This is the Main Oscillator register for the ARM SAMV71 BSP. https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/svd/i-sam-pmc.ads#L383

You will notice that all of these record components have default values applied, BUT, we are also using 'Address. Here is that record as a component of the full register definition. https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/svd/i-sam-pmc.ads#L2469 And here is the address overlay. https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/svd/i-sam-pmc.ads#L2581

The behavior here is that the default values are ignored because of the Import aspect. However, the default values are necessary so that I can perform operations like this https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/setup_pll.adb#L66 .

If you use 'Address without Import, then default initialization is performed, initializing the object at the address you specify. If you apply a pragma Import, you are saying some external process or device is doing the initialization, so there is no need for your program to do so.

Here we are setting values in the register and using others => <> to utilize the specified default values for the other record components. We have to set this register this way because of the KEY field.

This processor is hardened against spurious bit flips. The technique that is used is to disallow writes to certain registers is: the KEY field must be written with the correct bit sequence on every write. This means that we cannot do this: PMC_Periph.CKGR_MOR.MOSCXTEN := 1;

We must do:

PMC_Periph.CKGR_MOR := (MOSCXTEN => 1, KEY => Passwd, others => <>);

That will be rejected because we didn't also write the KEY field with the correct bit sequence.

So in this use case, we are required to use default values, 'Address, and 'Import on this register in order to get this to work. So I guess the question is: does it make sense to have two use cases for 'Import?

What is the reason for using Import on this register? Would 'Address without Import do what you want?

-Tuck

clairedross commented 4 years ago

ok, so a workaround would be for style checkers to not blindly forbid Import but only when no address clause is given. And possibly a restriction could be designed to do that to replace no_use_of_pragma => import. Whether this is prettier than adding yet another aspect or not, I honestly don't know.

Robert-Tice commented 4 years ago

We have to use 'Import on these registers because they are hardware registers. We don't want to Initialize the registers when the Ada runtime starts because we would overwrite the current processor state/settings that were setup in the start scripts.

For example: In the start scripts for the SAMV71, we setup all of the necessary hardware stuff (I and D caches, FPUs, TCMs) and then we call the setup_pll sequence (which is in Ada) to set up the main clock controllers before we even enter the Ada main. If we didn't use 'Import, and the default values were applied, we would essentially disable the clocks and put the chip into reset as soon as we entered the Ada code.

sttaft commented 4 years ago

One subtlety with default initialization is that there are really (at least) two sorts of default initialization, one which is to some extent "optional" and one which is "fundamental" to correct operation. For example, the tag and discriminants, if not initialized properly, will almost certainly result in erroneous execution. Also, at least some Ada compilers have additional "offset" fields in certain records with multiple dynamically-sized components, to reduce computational overhead in "reaching" such a component. Such fields also need to be initialized for the object to be usable in any way.

Pragma "Import" is really saying that the object already exists and is properly initialized by some external process, and neither the "optional" nor the "fundamental" initialization should be performed by the current process. Any sort of "No_Initialization" aspect will need to address this distinction, and decide what parts of default initialization are to be omitted.

-Tuck

On Fri, Aug 7, 2020 at 10:38 AM Tucker Taft tucker@yaletaft.com wrote:

On Fri, Aug 7, 2020 at 10:01 AM Robert Tice notifications@github.com wrote:

An argument here is that Import is an overloaded aspect. To me, it has two uses (maybe there's more):

1.) When not combined with 'Address => Tell the compiler that the object implementation is external and will be resolved at link time (equivalent to C's extern) 2.) When combined with 'Address => This is POD (plain old data) and we will read the values from the address rather than apply a default initialization, even if default values are provided.

Here's the embedded use case: This is the Main Oscillator register for the ARM SAMV71 BSP. https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/svd/i-sam-pmc.ads#L383

You will notice that all of these record components have default values applied, BUT, we are also using 'Address. Here is that record as a component of the full register definition. https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/svd/i-sam-pmc.ads#L2469 And here is the address overlay. https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/svd/i-sam-pmc.ads#L2581

The behavior here is that the default values are ignored because of the Import aspect. However, the default values are necessary so that I can perform operations like this https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/samv71/setup_pll.adb#L66 .

If you use 'Address without Import, then default initialization is performed, initializing the object at the address you specify. If you apply a pragma Import, you are saying some external process or device is doing the initialization, so there is no need for your program to do so.

Here we are setting values in the register and using others => <> to utilize the specified default values for the other record components. We have to set this register this way because of the KEY field.

This processor is hardened against spurious bit flips. The technique that is used is to disallow writes to certain registers is: the KEY field must be written with the correct bit sequence on every write. This means that we cannot do this: PMC_Periph.CKGR_MOR.MOSCXTEN := 1;

We must do:

PMC_Periph.CKGR_MOR := (MOSCXTEN => 1, KEY => Passwd, others => <>);

That will be rejected because we didn't also write the KEY field with the correct bit sequence.

So in this use case, we are required to use default values, 'Address, and 'Import on this register in order to get this to work. So I guess the question is: does it make sense to have two use cases for 'Import?

What is the reason for using Import on this register? Would 'Address without Import do what you want?

-Tuck

sttaft commented 4 years ago

On Fri, Aug 7, 2020 at 11:27 AM Robert Tice notifications@github.com wrote:

We have to use 'Import on these registers because they are hardware registers. We don't want to Initialize the registers when the Ada runtime starts because we would overwrite the current processor state/settings that were setup in the start scripts.

For example: In the start scripts for the SAMV71, we setup all of the necessary hardware stuff (I and D caches, FPUs, TCMs) and then we call the setup_pll sequence https://github.com/AdaCore/bb-runtimes/blob/50db90e35100ddb46df2a51748698521d7562121/arm/sam/start-rom.S#L76 (which is in Ada) to set up the main clock controllers before we even enter the Ada main. If we didn't use 'Import, and the default values were applied, we would essentially disable the clocks and put the chip into reset as soon as we entered the Ada code.

That makes perfect sense. But you said that "default initialization is necessary" and I didn't understand what you meant. Looking at your example I guess I realize you want to be able to use "others => <>" which implies some default initialization of fields of the aggregate. I don't think anyone was intending to turn off that sort of "explicit" (?) default initialization. And this aspect presumably applies only to a single object, and only affects what happens at the point of its declaration. It doesn't affect how aggregates are created and initialized, and doesn't affect subsequent assignments.

-Tuck

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/AdaCore/ada-spark-rfcs/issues/53#issuecomment-670572989, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANZ4FIH3THI62DMOMIYCNLR7QMNJANCNFSM4PXQIUCQ .